Removing pin-sha1 from HPKP to match RFC

Since draft-ietf-websec-key-pinning-10, SHA1 has been an invalid hash for
pinning. This change brings Chromium's implementation in line with the
released RFC 7469. In order to maintain TOFU for now, if there is known
SHA1 HPKP on disk, we still honor them until expiration.

BUG=448501

Review URL: https://codereview.chromium.org/1303343002

Cr-Commit-Position: refs/heads/master@{#345085}

net/data/url_request_unittest/hpkp-headers-report-only.html.mock-http-headers

@@ -3,4 +3,4 @@ Cache-Control: private
 Content-Type: text/html; charset=ISO-8859-1
 X-Multiple-Entries: a
 X-Multiple-Entries: b
-Public-Key-Pins-Report-Only: max-age=50000; pin-sha1="K9e3/nFL5j90GuVJOJBv6WXpvcs="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; pin-sha1="PshSs8WOjC7qwaYMv0T3rJDwKS4="; report-uri="https://hpkp-report.test"
+Public-Key-Pins-Report-Only: max-age=50000; pin-sha256="9999999999999999999999999999999999999999999="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; report-uri="https://hpkp-report.test"

net/data/url_request_unittest/hpkp-headers.html.mock-http-headers

@@ -3,4 +3,4 @@ Cache-Control: private
 Content-Type: text/html; charset=ISO-8859-1
 X-Multiple-Entries: a
 X-Multiple-Entries: b
-Public-Key-Pins: max-age=50000; pin-sha1="K9e3/nFL5j90GuVJOJBv6WXpvcs="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; pin-sha1="PshSs8WOjC7qwaYMv0T3rJDwKS4="; report-uri="https://hpkp-report.test"
+Public-Key-Pins: max-age=50000; pin-sha256="9999999999999999999999999999999999999999999="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; report-uri="https://hpkp-report.test"

net/data/url_request_unittest/hsts-and-hpkp-headers.html.mock-http-headers

@@ -5,4 +5,4 @@ X-Multiple-Entries: a
 X-Multiple-Entries: b
 Strict-Transport-Security: max-age=12300
 Strict-Transport-Security: max-age=12300; includeSubdomains
-Public-Key-Pins: max-age=50000; pin-sha1="Wws2/Z7YhKlX73v3rYHBBxO4OLE="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="
+Public-Key-Pins: max-age=50000; pin-sha256="9999999999999999999999999999999999999999999="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="

net/data/url_request_unittest/hsts-and-hpkp-headers2.html.mock-http-headers

@@ -4,5 +4,5 @@ Content-Type: text/html; charset=ISO-8859-1
 X-Multiple-Entries: a
 X-Multiple-Entries: b
 Strict-Transport-Security: max-age=12300; includeSubdomains
-Public-Key-Pins: max-age=50000; pin-sha1="K9e3/nFL5j90GuVJOJBv6WXpvcs="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; pin-sha1="PshSs8WOjC7qwaYMv0T3rJDwKS4="
-Public-Key-Pins: max-age=50000; pin-sha1="K9e3/nFL5j90GuVJOJBv6WXpvcs="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="; pin-sha1="PshSs8WOjC7qwaYMv0T3rJDwKS4="
+Public-Key-Pins: max-age=50000; pin-sha256="9999999999999999999999999999999999999999999="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="
+Public-Key-Pins: max-age=50000; pin-sha256="9999999999999999999999999999999999999999999="; pin-sha256="+TTrWvvJdM9gwuHiLTApo/2DBT2xb4hBPRJDI9pebXY="

net/http/http_response_headers_unittest.cc

@@ -467,8 +467,8 @@ const struct PersistData persistence_tests[] = {
      "Strict-Transport-Security: max-age=1576800\n"
      "Bar: 1\n"
      "Public-Key-Pins: max-age=100000; "
-     "pin-sha1=\"ObT42aoSpAqWdY9WfRfL7i0HsVk=\";"
-     "pin-sha1=\"7kW49EVwZG0hSNx41ZO/fUPN0ek=\"",
+     "pin-sha256=\"1111111111111111111111111111111111111111111=\";"
+     "pin-sha256=\"2222222222222222222222222222222222222222222=\"",
 
      "HTTP/1.1 200 OK\n"
      "Bar: 1\n"},

net/http/http_security_headers.cc

@@ -147,17 +147,6 @@ bool ParseHPKPHeaderImpl(const std::string& value,
         return false;
       }
       parsed_max_age = true;
-    } else if (base::LowerCaseEqualsASCII(
-                   base::StringPiece(name_value_pairs.name_begin(),
-                                     name_value_pairs.name_end()),
-                   "pin-sha1")) {
-      // Pins are always quoted.
-      if (!name_value_pairs.value_is_quoted() ||
-          !ParseAndAppendPin(name_value_pairs.value_begin(),
-                             name_value_pairs.value_end(), HASH_VALUE_SHA1,
-                             &pins)) {
-        return false;
-      }
     } else if (base::LowerCaseEqualsASCII(
                    base::StringPiece(name_value_pairs.name_begin(),
                                      name_value_pairs.name_end()),

net/http/http_security_headers_unittest.cc

@@ -6,7 +6,6 @@
 #include <algorithm>
 
 #include "base/base64.h"
-#include "base/sha1.h"
 #include "base/strings/string_piece.h"
 #include "crypto/sha2.h"
 #include "net/base/host_port_pair.h"
@@ -36,9 +35,6 @@ std::string GetTestPinImpl(uint8 label, HashValueTag tag, bool quoted) {
 
   std::string ret;
   switch (hash_value.tag) {
-    case HASH_VALUE_SHA1:
-      ret = "pin-sha1=";
-      break;
     case HASH_VALUE_SHA256:
       ret = "pin-sha256=";
       break;
@@ -645,18 +641,10 @@ static void TestValidPKPHeaders(HashValueTag tag) {
   EXPECT_EQ(expect_report_uri, report_uri);
 }
 
-TEST_F(HttpSecurityHeadersTest, BogusPinsHeadersSHA1) {
-  TestBogusPinsHeaders(HASH_VALUE_SHA1);
-}
-
 TEST_F(HttpSecurityHeadersTest, BogusPinsHeadersSHA256) {
   TestBogusPinsHeaders(HASH_VALUE_SHA256);
 }
 
-TEST_F(HttpSecurityHeadersTest, ValidPKPHeadersSHA1) {
-  TestValidPKPHeaders(HASH_VALUE_SHA1);
-}
-
 TEST_F(HttpSecurityHeadersTest, ValidPKPHeadersSHA256) {
   TestValidPKPHeaders(HASH_VALUE_SHA256);
 }
@@ -675,10 +663,10 @@ TEST_F(HttpSecurityHeadersTest, UpdateDynamicPKPOnly) {
   HashValueVector saved_hashes = static_pkp_state.spki_hashes;
 
   // Add a header, which should only update the dynamic state.
-  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA1);
-  HashValue backup_hash = GetTestHashValue(2, HASH_VALUE_SHA1);
-  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA1);
-  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA1);
+  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA256);
+  HashValue backup_hash = GetTestHashValue(2, HASH_VALUE_SHA256);
+  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA256);
+  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA256);
   GURL report_uri("http://google.com");
   std::string header = "max-age = 10000; " + good_pin + "; " + backup_pin +
                        ";report-uri=\"" + report_uri.spec() + "\"";
@@ -756,9 +744,9 @@ TEST_F(HttpSecurityHeadersTest, UpdateDynamicPKPMaxAge0) {
   HashValueVector saved_hashes = static_pkp_state.spki_hashes;
 
   // Add a header, which should only update the dynamic state.
-  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA1);
-  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA1);
-  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA1);
+  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA256);
+  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA256);
+  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA256);
   std::string header = "max-age = 10000; " + good_pin + "; " + backup_pin;
 
   // Construct a fake SSLInfo that will pass AddHPKPHeader's checks.
@@ -858,9 +846,9 @@ TEST_F(HttpSecurityHeadersTest, NoClobberPins) {
       TransportSecurityState::DISABLE_PIN_REPORTS, &failure_log));
 
   // Add an HPKP header, which should only update the dynamic state.
-  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA1);
-  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA1);
-  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA1);
+  HashValue good_hash = GetTestHashValue(1, HASH_VALUE_SHA256);
+  std::string good_pin = GetTestPin(1, HASH_VALUE_SHA256);
+  std::string backup_pin = GetTestPin(2, HASH_VALUE_SHA256);
   std::string header = "max-age = 10000; " + good_pin + "; " + backup_pin;
 
   // Construct a fake SSLInfo that will pass AddHPKPHeader's checks.

net/http/transport_security_persister.cc

@@ -189,6 +189,11 @@ bool TransportSecurityPersister::SerializeData(std::string* output) {
     serialized->SetDouble(kDynamicSPKIHashesExpiry,
                           pkp_state.expiry.ToDoubleT());
 
+    // TODO(svaldez): Historically, both SHA-1 and SHA-256 hashes were
+    // accepted in pins. Per spec, only SHA-256 is accepted now, however
+    // existing serialized pins are still processed. Migrate historical pins
+    // with SHA-1 hashes properly, either by dropping just the bad hashes or
+    // the entire pin. See https://crbug.com/448501.
     if (now < pkp_state.expiry) {
       serialized->Set(kDynamicSPKIHashes,
                       SPKIHashesToListValue(pkp_state.spki_hashes));

net/http/transport_security_persister_unittest.cc

@@ -90,9 +90,9 @@ TEST_F(TransportSecurityPersisterTest, SerializeData2) {
 TEST_F(TransportSecurityPersisterTest, SerializeData3) {
   const GURL report_uri(kReportUri);
   // Add an entry.
-  HashValue fp1(HASH_VALUE_SHA1);
+  HashValue fp1(HASH_VALUE_SHA256);
   memset(fp1.data(), 0, fp1.size());
-  HashValue fp2(HASH_VALUE_SHA1);
+  HashValue fp2(HASH_VALUE_SHA256);
   memset(fp2.data(), 1, fp2.size());
   base::Time expiry =
       base::Time::Now() + base::TimeDelta::FromSeconds(1000);
@@ -193,13 +193,13 @@ TEST_F(TransportSecurityPersisterTest, PublicKeyPins) {
   std::string failure_log;
   EXPECT_FALSE(pkp_state.CheckPublicKeyPins(hashes, &failure_log));
 
-  HashValue sha1(HASH_VALUE_SHA1);
-  memset(sha1.data(), '1', sha1.size());
-  pkp_state.spki_hashes.push_back(sha1);
+  HashValue sha256(HASH_VALUE_SHA256);
+  memset(sha256.data(), '1', sha256.size());
+  pkp_state.spki_hashes.push_back(sha256);
 
   EXPECT_FALSE(pkp_state.CheckPublicKeyPins(hashes, &failure_log));
 
-  hashes.push_back(sha1);
+  hashes.push_back(sha256);
   EXPECT_TRUE(pkp_state.CheckPublicKeyPins(hashes, &failure_log));
 
   hashes[0].data()[0] = '2';
@@ -219,9 +219,9 @@ TEST_F(TransportSecurityPersisterTest, PublicKeyPins) {
   TransportSecurityState::PKPState new_pkp_state;
   EXPECT_TRUE(state_.GetDynamicPKPState(kTestDomain, &new_pkp_state));
   EXPECT_EQ(1u, new_pkp_state.spki_hashes.size());
-  EXPECT_EQ(sha1.tag, new_pkp_state.spki_hashes[0].tag);
-  EXPECT_EQ(
-      0, memcmp(new_pkp_state.spki_hashes[0].data(), sha1.data(), sha1.size()));
+  EXPECT_EQ(sha256.tag, new_pkp_state.spki_hashes[0].tag);
+  EXPECT_EQ(0, memcmp(new_pkp_state.spki_hashes[0].data(), sha256.data(),
+                      sha256.size()));
   EXPECT_EQ(report_uri, new_pkp_state.report_uri);
 }
 

net/http/transport_security_state_unittest.cc

@@ -49,19 +49,29 @@ const char kReportUri[] = "http://example.test/test";
 
 // kGoodPath is blog.torproject.org.
 const char* const kGoodPath[] = {
-    "sha1/m9lHYJYke9k0GtVZ+bXSQYE8nDI=", "sha1/o5OZxATDsgmwgcIfIWIneMJ0jkw=",
-    "sha1/wHqYaI2J+6sFZAwRfap9ZbjKzE4=", NULL,
+    "sha1/Yz4vayd/83rQfDXkDPn2yhzIScw=",
+    "sha1/3lKvjNsfmrn+WmfDhvr2iVh/yRs=",
+    "sha1/gzF+YoVCU9bXeDGQ7JGQVumRueM=",
+    "sha256/4osU79hfY3P2+WJGlT2mxmSL+5FIwLEVxTQcavyBNgQ=",
+    "sha256/k2v657xBsOVe1PQRwOsHsw3bsGT2VzIqz5K+59sNQws=",
+    "sha256/WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18=",
+    nullptr,
 };
 
-const char kGoodPin1[] = "m9lHYJYke9k0GtVZ+bXSQYE8nDI=";
-const char kGoodPin2[] = "o5OZxATDsgmwgcIfIWIneMJ0jkw=";
-const char kGoodPin3[] = "wHqYaI2J+6sFZAwRfap9ZbjKzE4=";
+const char kGoodPin1[] = "4osU79hfY3P2+WJGlT2mxmSL+5FIwLEVxTQcavyBNgQ=";
+const char kGoodPin2[] = "k2v657xBsOVe1PQRwOsHsw3bsGT2VzIqz5K+59sNQws=";
+const char kGoodPin3[] = "WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18=";
 
 // kBadPath is plus.google.com via Trustcenter, which is utterly wrong for
 // torproject.org.
 const char* const kBadPath[] = {
-    "sha1/4BjDjn8v2lWeUFQnqSs0BgbIcrU=", "sha1/gzuEEAB/bkqdQS3EIjk2by7lW+k=",
-    "sha1/SOZo+SvSspXXR9gjIBBPM5iQn9Q=", NULL,
+    "sha1/111111111111111111111111111=",
+    "sha1/222222222222222222222222222=",
+    "sha1/333333333333333333333333333=",
+    "sha256/1111111111111111111111111111111111111111111=",
+    "sha256/2222222222222222222222222222222222222222222=",
+    "sha256/3333333333333333333333333333333333333333333=",
+    nullptr,
 };
 
 // A mock ReportSender that just remembers the latest report
@@ -183,7 +193,7 @@ class TransportSecurityStateTest : public testing::Test {
 
   static HashValueVector GetSampleSPKIHashes() {
     HashValueVector spki_hashes;
-    HashValue hash(HASH_VALUE_SHA1);
+    HashValue hash(HASH_VALUE_SHA256);
     memset(hash.data(), 0, hash.size());
     spki_hashes.push_back(hash);
     return spki_hashes;
@@ -554,11 +564,11 @@ TEST_F(TransportSecurityStateTest, NewPinsOverride) {
   TransportSecurityState::PKPState pkp_state;
   const base::Time current_time(base::Time::Now());
   const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
-  HashValue hash1(HASH_VALUE_SHA1);
+  HashValue hash1(HASH_VALUE_SHA256);
   memset(hash1.data(), 0x01, hash1.size());
-  HashValue hash2(HASH_VALUE_SHA1);
+  HashValue hash2(HASH_VALUE_SHA256);
   memset(hash2.data(), 0x02, hash1.size());
-  HashValue hash3(HASH_VALUE_SHA1);
+  HashValue hash3(HASH_VALUE_SHA256);
   memset(hash3.data(), 0x03, hash1.size());
 
   state.AddHPKP("example.com", expiry, true, HashValueVector(1, hash1),
@@ -1310,8 +1320,8 @@ TEST_F(TransportSecurityStateTest, HPKPReportOnly) {
   // Check that a report is not sent for a Report-Only header with no
   // violation.
   std::string header =
-      "pin-sha1=\"" + std::string(kGoodPin1) + "\";pin-sha1=\"" +
-      std::string(kGoodPin2) + "\";pin-sha1=\"" + std::string(kGoodPin3) +
+      "pin-sha256=\"" + std::string(kGoodPin1) + "\";pin-sha256=\"" +
+      std::string(kGoodPin2) + "\";pin-sha256=\"" + std::string(kGoodPin3) +
       "\";report-uri=\"" + report_uri.spec() + "\";includeSubdomains";
   SSLInfo ssl_info;
   ssl_info.is_issued_by_known_root = true;
@@ -1356,8 +1366,8 @@ TEST_F(TransportSecurityStateTest, HPKPReportOnlyOnLocalRoot) {
   ASSERT_TRUE(cert2);
 
   std::string header =
-      "pin-sha1=\"" + std::string(kGoodPin1) + "\";pin-sha1=\"" +
-      std::string(kGoodPin2) + "\";pin-sha1=\"" + std::string(kGoodPin3) +
+      "pin-sha256=\"" + std::string(kGoodPin1) + "\";pin-sha256=\"" +
+      std::string(kGoodPin2) + "\";pin-sha256=\"" + std::string(kGoodPin3) +
       "\";report-uri=\"" + report_uri.spec() + "\";includeSubdomains";
 
   TransportSecurityState state;
@@ -1392,9 +1402,9 @@ TEST_F(TransportSecurityStateTest, HPKPReportOnlyParseErrors) {
   ASSERT_TRUE(cert1);
   ASSERT_TRUE(cert2);
 
-  std::string header = "pin-sha1=\"" + std::string(kGoodPin1) +
-                       "\";pin-sha1=\"" + std::string(kGoodPin2) +
-                       "\";pin-sha1=\"" + std::string(kGoodPin3) + "\"";
+  std::string header = "pin-sha256=\"" + std::string(kGoodPin1) +
+                       "\";pin-sha256=\"" + std::string(kGoodPin2) +
+                       "\";pin-sha256=\"" + std::string(kGoodPin3) + "\"";
 
   TransportSecurityState state;
   MockCertificateReportSender mock_report_sender;

net/url_request/url_request_unittest.cc

@@ -5462,8 +5462,10 @@ TEST_F(URLRequestTestHTTP, MAYBE_ProcessPKPAndSendReport) {
   HashValue hash2;
   // The values here don't matter, as long as they are different from
   // the mocked CertVerifyResult below.
-  ASSERT_TRUE(hash1.FromString("sha1/111111111111111111111111111="));
-  ASSERT_TRUE(hash2.FromString("sha1/222222222222222222222222222="));
+  ASSERT_TRUE(
+      hash1.FromString("sha256/1111111111111111111111111111111111111111111="));
+  ASSERT_TRUE(
+      hash2.FromString("sha256/2222222222222222222222222222222222222222222="));
   hashes.push_back(hash1);
   hashes.push_back(hash2);
   security_state.AddHPKP(test_server_hostname, expiry,
@@ -5483,7 +5485,8 @@ TEST_F(URLRequestTestHTTP, MAYBE_ProcessPKPAndSendReport) {
   verify_result.verified_cert = cert;
   verify_result.is_issued_by_known_root = true;
   HashValue hash3;
-  ASSERT_TRUE(hash3.FromString("sha1/333333333333333333333333333="));
+  ASSERT_TRUE(
+      hash3.FromString("sha256/3333333333333333333333333333333333333333333="));
   verify_result.public_key_hashes.push_back(hash3);
   cert_verifier.AddResultForCert(cert.get(), verify_result, OK);
 
@@ -5545,7 +5548,8 @@ TEST_F(URLRequestTestHTTP, MAYBE_ProcessPKPReportOnly) {
   HashValue hash;
   // This value doesn't matter, as long as it is different from the pins
   // for the request to hpkp-headers-report-only.html.
-  ASSERT_TRUE(hash.FromString("sha1/111111111111111111111111111="));
+  ASSERT_TRUE(
+      hash.FromString("sha256/1111111111111111111111111111111111111111111="));
   verify_result.public_key_hashes.push_back(hash);
   cert_verifier.AddResultForCert(cert.get(), verify_result, OK);