Skip to content

Commit

Permalink
Fix include_subdomains handling with PKI Metadata pins list
Browse files Browse the repository at this point in the history 
Previously include_subdomains was ignored once the pinning list was
loaded from a component (due to the find() call for the map using the
full domain). This fixes this by calling it again with the url substring
starting at the next . if the full domain is not found.

(cherry picked from commit ca9cbe0)

Bug: 1404811
Change-Id: I2d487401711a8a44e9cedb4b91c84ae5553c6932
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4137286
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1091464}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4166623
Auto-Submit: Carlos IL <carlosil@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/branch-heads/5414@{#1366}
Cr-Branched-From: 4417ee5-refs/heads/main@{#1070088}
  • Loading branch information
@carlosjoan91
carlosjoan91 authored and Chromium LUCI CQ committed Jan 13, 2023
1 parent 2037789 commit 11f1111
Show file tree
Hide file tree
Showing 3 changed files with 284 additions and 26 deletions.
78 changes: 53 additions & 25 deletions net/http/transport_security_state.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,7 @@ void TransportSecurityState::UpdatePinList(
base::Time update_time) {
pinsets_ = pinsets;
key_pins_list_last_update_time_ = update_time;
host_pins_ = absl::make_optional(
std::map<std::string, std::pair<PinSet const*, bool>>());
host_pins_.emplace();
std::map<std::string, PinSet const*> pinset_names_map;
for (const auto& pinset : pinsets_) {
pinset_names_map[pinset.name()] = &pinset;
Expand Down Expand Up @@ -1182,32 +1181,61 @@ bool TransportSecurityState::GetStaticPKPState(const std::string& host,

PreloadResult result;
if (host_pins_.has_value()) {
auto iter = host_pins_->find(host);
if (iter != host_pins_->end()) {
pkp_result->domain = host;
pkp_result->last_observed = key_pins_list_last_update_time_;
pkp_result->include_subdomains = iter->second.second;
const PinSet* pinset = iter->second.first;
if (!pinset->report_uri().empty()) {
pkp_result->report_uri = GURL(pinset->report_uri());
}
for (auto hash : pinset->static_spki_hashes()) {
// If the update is malformed, it's preferable to skip the hash than
// crash.
if (hash.size() == 32) {
AddHash(reinterpret_cast<const char*>(hash.data()),
&pkp_result->spki_hashes);
// Ensure that |host| is a valid hostname before processing.
if (CanonicalizeHost(host).empty()) {
return false;
}
// Normalize any trailing '.' used for DNS suffix searches.
std::string normalized_host = host;
size_t trailing_dot_found = normalized_host.find_last_not_of('.');
if (trailing_dot_found == std::string::npos) {
// Hostname is either empty or all dots
return false;
}
normalized_host.erase(trailing_dot_found + 1);
normalized_host = base::ToLowerASCII(normalized_host);

base::StringPiece search_hostname = normalized_host;
while (true) {
auto iter = host_pins_->find(search_hostname);
// Only consider this a match if either include_subdomains is set, or
// this is an exact match of the full hostname.
if (iter != host_pins_->end() &&
(iter->second.second || search_hostname == normalized_host)) {
pkp_result->domain = std::string(search_hostname);
pkp_result->last_observed = key_pins_list_last_update_time_;
pkp_result->include_subdomains = iter->second.second;
const PinSet* pinset = iter->second.first;
if (!pinset->report_uri().empty()) {
pkp_result->report_uri = GURL(pinset->report_uri());
}
}
for (auto hash : pinset->bad_static_spki_hashes()) {
// If the update is malformed, it's preferable to skip the hash than
// crash.
if (hash.size() == 32) {
AddHash(reinterpret_cast<const char*>(hash.data()),
&pkp_result->bad_spki_hashes);
for (auto hash : pinset->static_spki_hashes()) {
// If the update is malformed, it's preferable to skip the hash than
// crash.
if (hash.size() == 32) {
AddHash(reinterpret_cast<const char*>(hash.data()),
&pkp_result->spki_hashes);
}
}
for (auto hash : pinset->bad_static_spki_hashes()) {
// If the update is malformed, it's preferable to skip the hash than
// crash.
if (hash.size() == 32) {
AddHash(reinterpret_cast<const char*>(hash.data()),
&pkp_result->bad_spki_hashes);
}
}
return true;
}
return true;
auto dot_pos = search_hostname.find(".");
if (dot_pos == std::string::npos) {
// If this was not a match, and there are no more dots in the string,
// there are no more domains to try.
return false;
}
// Try again in case this is a subdomain of a pinned domain that includes
// subdomains.
search_hostname = search_hostname.substr(dot_pos + 1);
}
} else if (DecodeHSTSPreload(host, &result) && result.has_pins) {
if (result.pinset_id >= g_hsts_source->pinsets_count)
Expand Down
3 changes: 2 additions & 1 deletion net/http/transport_security_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,8 @@ class NET_EXPORT TransportSecurityState {

// The values in host_pins_ maps are references to PinSet objects in the
// pinsets_ vector.
absl::optional<std::map<std::string, std::pair<const PinSet*, bool>>>
absl::optional<
std::map<std::string, std::pair<const PinSet*, bool>, std::less<>>>
host_pins_;
base::Time key_pins_list_last_update_time_;
std::vector<PinSet> pinsets_;
Expand Down
229 changes: 229 additions & 0 deletions net/http/transport_security_state_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4053,6 +4053,23 @@ TEST_F(TransportSecurityStateTest, UpdateKeyPinsListNotValidPin) {
host_port_pair, true, good_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Hashes should also be rejected if the hostname has a trailing dot.
host_port_pair = HostPortPair("example.test.", kPort);
EXPECT_EQ(TransportSecurityState::PKPStatus::VIOLATED,
state.CheckPublicKeyPins(
host_port_pair, true, good_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Hashes should also be rejected if the hostname has different
// capitalization.
host_port_pair = HostPortPair("ExAmpLe.tEsT", kPort);
EXPECT_EQ(TransportSecurityState::PKPStatus::VIOLATED,
state.CheckPublicKeyPins(
host_port_pair, true, good_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));
}

TEST_F(TransportSecurityStateTest, UpdateKeyPinsEmptyList) {
Expand Down Expand Up @@ -4099,6 +4116,218 @@ TEST_F(TransportSecurityStateTest, UpdateKeyPinsEmptyList) {
network_anonymization_key, &unused_failure_log));
}

TEST_F(TransportSecurityStateTest, UpdateKeyPinsIncludeSubdomains) {
base::test::ScopedFeatureList scoped_feature_list_;
scoped_feature_list_.InitAndEnableFeature(
net::features::kStaticKeyPinningEnforcement);
HostPortPair host_port_pair("example.sub.test", kPort);
GURL report_uri(kReportUri);
NetworkAnonymizationKey network_anonymization_key =
NetworkAnonymizationKey::CreateTransient();
// Two dummy certs to use as the server-sent and validated chains. The
// contents don't matter.
scoped_refptr<X509Certificate> cert1 =
ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem");
ASSERT_TRUE(cert1);
scoped_refptr<X509Certificate> cert2 =
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
ASSERT_TRUE(cert2);

// unpinned_hashes is a set of hashes that (after the update) won't match the
// expected hashes for the tld of this domain. kGoodPath is used here because
// it's a path that is accepted prior to any updates, and this test will
// validate it is rejected afterwards.
HashValueVector unpinned_hashes;
for (size_t i = 0; kGoodPath[i]; i++) {
EXPECT_TRUE(AddHash(kGoodPath[i], &unpinned_hashes));
}

TransportSecurityState state;
EnableStaticPins(&state);
std::string unused_failure_log;

// Prior to updating the list, unpinned_hashes should be accepted
EXPECT_EQ(TransportSecurityState::PKPStatus::OK,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Update the pins list, adding kBadPath to the accepted hashes for this
// host, relying on include_subdomains for enforcement. The contents of the
// hashes don't matter as long as they are different from unpinned_hashes,
// kBadPath is used for convenience.
std::vector<std::vector<uint8_t>> accepted_hashes;
for (size_t i = 0; kBadPath[i]; i++) {
HashValue hash;
ASSERT_TRUE(hash.FromString(kBadPath[i]));
accepted_hashes.emplace_back(hash.data(), hash.data() + hash.size());
}
TransportSecurityState::PinSet test_pinset(
/*name=*/"test",
/*static_spki_hashes=*/{accepted_hashes},
/*bad_static_spki_hashes=*/{},
/*report_uri=*/kReportUri);
// The host used in the test is "example.sub.test", so this pinset will only
// match due to include subdomains.
TransportSecurityState::PinSetInfo test_pinsetinfo(
/*hostname=*/"sub.test", /* pinset_name=*/"test",
/*include_subdomains=*/true);
state.UpdatePinList({test_pinset}, {test_pinsetinfo}, base::Time::Now());

// The path that was accepted before updating the pins should now be rejected.
EXPECT_EQ(TransportSecurityState::PKPStatus::VIOLATED,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));
}

TEST_F(TransportSecurityStateTest, UpdateKeyPinsIncludeSubdomainsTLD) {
base::test::ScopedFeatureList scoped_feature_list_;
scoped_feature_list_.InitAndEnableFeature(
net::features::kStaticKeyPinningEnforcement);
HostPortPair host_port_pair(kHost, kPort);
GURL report_uri(kReportUri);
NetworkAnonymizationKey network_anonymization_key =
NetworkAnonymizationKey::CreateTransient();
// Two dummy certs to use as the server-sent and validated chains. The
// contents don't matter.
scoped_refptr<X509Certificate> cert1 =
ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem");
ASSERT_TRUE(cert1);
scoped_refptr<X509Certificate> cert2 =
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
ASSERT_TRUE(cert2);

// unpinned_hashes is a set of hashes that (after the update) won't match the
// expected hashes for the tld of this domain. kGoodPath is used here because
// it's a path that is accepted prior to any updates, and this test will
// validate it is rejected afterwards.
HashValueVector unpinned_hashes;
for (size_t i = 0; kGoodPath[i]; i++) {
EXPECT_TRUE(AddHash(kGoodPath[i], &unpinned_hashes));
}

TransportSecurityState state;
EnableStaticPins(&state);
std::string unused_failure_log;

// Prior to updating the list, unpinned_hashes should be accepted
EXPECT_EQ(TransportSecurityState::PKPStatus::OK,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Update the pins list, adding kBadPath to the accepted hashes for this
// host, relying on include_subdomains for enforcement. The contents of the
// hashes don't matter as long as they are different from unpinned_hashes,
// kBadPath is used for convenience.
std::vector<std::vector<uint8_t>> accepted_hashes;
for (size_t i = 0; kBadPath[i]; i++) {
HashValue hash;
ASSERT_TRUE(hash.FromString(kBadPath[i]));
accepted_hashes.emplace_back(hash.data(), hash.data() + hash.size());
}
TransportSecurityState::PinSet test_pinset(
/*name=*/"test",
/*static_spki_hashes=*/{accepted_hashes},
/*bad_static_spki_hashes=*/{},
/*report_uri=*/kReportUri);
// The host used in the test is "example.test", so this pinset will only match
// due to include subdomains.
TransportSecurityState::PinSetInfo test_pinsetinfo(
/*hostname=*/"test", /* pinset_name=*/"test",
/*include_subdomains=*/true);
state.UpdatePinList({test_pinset}, {test_pinsetinfo}, base::Time::Now());

// The path that was accepted before updating the pins should now be rejected.
EXPECT_EQ(TransportSecurityState::PKPStatus::VIOLATED,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));
}

TEST_F(TransportSecurityStateTest, UpdateKeyPinsDontIncludeSubdomains) {
base::test::ScopedFeatureList scoped_feature_list_;
scoped_feature_list_.InitAndEnableFeature(
net::features::kStaticKeyPinningEnforcement);
HostPortPair host_port_pair(kHost, kPort);
GURL report_uri(kReportUri);
NetworkAnonymizationKey network_anonymization_key =
NetworkAnonymizationKey::CreateTransient();
// Two dummy certs to use as the server-sent and validated chains. The
// contents don't matter.
scoped_refptr<X509Certificate> cert1 =
ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem");
ASSERT_TRUE(cert1);
scoped_refptr<X509Certificate> cert2 =
ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
ASSERT_TRUE(cert2);

// unpinned_hashes is a set of hashes that (after the update) won't match the
// expected hashes for the tld of this domain. kGoodPath is used here because
// it's a path that is accepted prior to any updates, and this test will
// validate it is accepted or rejected afterwards depending on whether the
// domain is an exact match.
HashValueVector unpinned_hashes;
for (size_t i = 0; kGoodPath[i]; i++) {
EXPECT_TRUE(AddHash(kGoodPath[i], &unpinned_hashes));
}

TransportSecurityState state;
EnableStaticPins(&state);
std::string unused_failure_log;

// Prior to updating the list, unpinned_hashes should be accepted
EXPECT_EQ(TransportSecurityState::PKPStatus::OK,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Update the pins list, adding kBadPath to the accepted hashes for the
// tld of this host, but without include_subdomains set. The contents of the
// hashes don't matter as long as they are different from unpinned_hashes,
// kBadPath is used for convenience.
std::vector<std::vector<uint8_t>> accepted_hashes;
for (size_t i = 0; kBadPath[i]; i++) {
HashValue hash;
ASSERT_TRUE(hash.FromString(kBadPath[i]));
accepted_hashes.emplace_back(hash.data(), hash.data() + hash.size());
}
TransportSecurityState::PinSet test_pinset(
/*name=*/"test",
/*static_spki_hashes=*/{accepted_hashes},
/*bad_static_spki_hashes=*/{},
/*report_uri=*/kReportUri);
// The host used in the test is "example.test", so this pinset will not match
// due to include subdomains not being set.
TransportSecurityState::PinSetInfo test_pinsetinfo(
/*hostname=*/"test", /* pinset_name=*/"test",
/*include_subdomains=*/false);
state.UpdatePinList({test_pinset}, {test_pinsetinfo}, base::Time::Now());

// Hashes that were accepted before the update should still be accepted since
// include subdomains is not set for the pinset, and this is not an exact
// match.
EXPECT_EQ(TransportSecurityState::PKPStatus::OK,
state.CheckPublicKeyPins(
host_port_pair, true, unpinned_hashes, cert1.get(), cert2.get(),
TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));

// Hashes should be rejected for an exact match of the hostname.
HostPortPair exact_match_host("test", kPort);
EXPECT_EQ(TransportSecurityState::PKPStatus::VIOLATED,
state.CheckPublicKeyPins(
exact_match_host, true, unpinned_hashes, cert1.get(),
cert2.get(), TransportSecurityState::ENABLE_PIN_REPORTS,
network_anonymization_key, &unused_failure_log));
}

TEST_F(TransportSecurityStateTest, UpdateKeyPinsListTimestamp) {
base::test::ScopedFeatureList scoped_feature_list_;
scoped_feature_list_.InitAndEnableFeature(
Expand Down

0 comments on commit 11f1111

Please sign in to comment.