diff options
author | Carlos IL <carlosil@chromium.org> | 2023-01-13 23:44:56 +0000 |
---|---|---|
committer | Michal Klocek <michal.klocek@qt.io> | 2023-02-02 10:33:48 +0000 |
commit | c67083d788a5c6d663ea8c0466b38ddc324e31a9 (patch) | |
tree | ad25518b4630efb28ccc5c471ed20db9d9fc7dc0 | |
parent | 1c46e13f96bb6b51950f35675f26b404f23beb08 (diff) | |
download | qtwebengine-chromium-c67083d788a5c6d663ea8c0466b38ddc324e31a9.tar.gz |
[Backport] Security bug 1404811
Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4167220:
Fix include_subdomains handling with PKI Metadata pins list
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 ca9cbe0aad4e4070b1e3c1ca530ede8a8a52cc4f)
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/+/4167220
Commit-Queue: David Benjamin <davidben@chromium.org>
Auto-Submit: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/branch-heads/5359@{#1333}
Cr-Branched-From: 27d3765d341b09369006d030f83f582a29eb57ae-refs/heads/main@{#1058933}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/456880
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
-rw-r--r-- | chromium/net/http/transport_security_state.cc | 78 | ||||
-rw-r--r-- | chromium/net/http/transport_security_state.h | 3 | ||||
-rw-r--r-- | chromium/net/http/transport_security_state_unittest.cc | 229 |
3 files changed, 284 insertions, 26 deletions
diff --git a/chromium/net/http/transport_security_state.cc b/chromium/net/http/transport_security_state.cc index 68d362d5fdc..8a02acf1837 100644 --- a/chromium/net/http/transport_security_state.cc +++ b/chromium/net/http/transport_security_state.cc @@ -639,8 +639,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; @@ -1184,32 +1183,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) diff --git a/chromium/net/http/transport_security_state.h b/chromium/net/http/transport_security_state.h index 464cbd182e1..c7a88437b8b 100644 --- a/chromium/net/http/transport_security_state.h +++ b/chromium/net/http/transport_security_state.h @@ -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_; diff --git a/chromium/net/http/transport_security_state_unittest.cc b/chromium/net/http/transport_security_state_unittest.cc index 905bbb70ba5..a54ac648eb4 100644 --- a/chromium/net/http/transport_security_state_unittest.cc +++ b/chromium/net/http/transport_security_state_unittest.cc @@ -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) { @@ -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( |