Revert "Revert "Reject certificates that are valid for too long.""

This reverts commit 7a8351f. Broke net_unittests on mac and linux Cr-Commit-Position: refs/heads/master@{#303519}
280723148 · Nov 10, 2014 · 96fcb48 · 96fcb48
1 parent b35d1e1
commit 96fcb48
Show file tree

Hide file tree

Showing 18 changed files with 831 additions and 61 deletions.
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
@@ -2667,6 +2667,12 @@ Even if you have downloaded files from this website before, the website might ha
       <message name="IDS_CERT_ERROR_NAME_CONSTRAINT_VIOLATION_DESCRIPTION" desc="Description of the error page for a certificate that contains a name outside of its scope">
         Server's certificate violates name constraints.
       </message>
+      <message name="IDS_CERT_ERROR_VALIDITY_TOO_LONG_DETAILS" desc="Details of the error page for a certificate whose validity period is too long">
+        You attempted to reach <ph name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but the server presented a certificate for which the period is too long.
+      </message>
+      <message name="IDS_CERT_ERROR_VALIDITY_TOO_LONG_DESCRIPTION" desc="Description of the error page for a certificate whose validity period is too long">
+        The server certificate has a validity period that is too long.
+      </message>
 
       <message name="IDS_CERT_ERROR_UNKNOWN_ERROR_DETAILS" desc="Details of the error page for an unknown ssl error">
         An unknown error has occurred.

diff --git a/chrome/browser/ssl/ssl_error_info.cc b/chrome/browser/ssl/ssl_error_info.cc
@@ -145,6 +145,13 @@ SSLErrorInfo SSLErrorInfo::CreateError(ErrorType error_type,
       short_description = l10n_util::GetStringUTF16(
           IDS_CERT_ERROR_NAME_CONSTRAINT_VIOLATION_DESCRIPTION);
       break;
+    case CERT_VALIDITY_TOO_LONG:
+      details =
+          l10n_util::GetStringFUTF16(IDS_CERT_ERROR_VALIDITY_TOO_LONG_DETAILS,
+                                     UTF8ToUTF16(request_url.host()));
+      short_description = l10n_util::GetStringUTF16(
+          IDS_CERT_ERROR_VALIDITY_TOO_LONG_DESCRIPTION);
+      break;
     case CERT_PINNED_KEY_MISSING:
       details = l10n_util::GetStringUTF16(
           IDS_ERRORPAGES_SUMMARY_PINNING_FAILURE);
@@ -191,6 +198,8 @@ SSLErrorInfo::ErrorType SSLErrorInfo::NetErrorToErrorType(int net_error) {
       return CERT_WEAK_KEY;
     case net::ERR_CERT_NAME_CONSTRAINT_VIOLATION:
       return CERT_NAME_CONSTRAINT_VIOLATION;
+    case net::ERR_CERT_VALIDITY_TOO_LONG:
+      return CERT_VALIDITY_TOO_LONG;
     case net::ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY:
       return CERT_WEAK_KEY_DH;
     case net::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN:
@@ -207,29 +216,31 @@ int SSLErrorInfo::GetErrorsForCertStatus(int cert_id,
                                          const GURL& url,
                                          std::vector<SSLErrorInfo>* errors) {
   const net::CertStatus kErrorFlags[] = {
-    net::CERT_STATUS_COMMON_NAME_INVALID,
-    net::CERT_STATUS_DATE_INVALID,
-    net::CERT_STATUS_AUTHORITY_INVALID,
-    net::CERT_STATUS_NO_REVOCATION_MECHANISM,
-    net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION,
-    net::CERT_STATUS_REVOKED,
-    net::CERT_STATUS_INVALID,
-    net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM,
-    net::CERT_STATUS_WEAK_KEY,
-    net::CERT_STATUS_NAME_CONSTRAINT_VIOLATION,
+      net::CERT_STATUS_COMMON_NAME_INVALID,
+      net::CERT_STATUS_DATE_INVALID,
+      net::CERT_STATUS_AUTHORITY_INVALID,
+      net::CERT_STATUS_NO_REVOCATION_MECHANISM,
+      net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION,
+      net::CERT_STATUS_REVOKED,
+      net::CERT_STATUS_INVALID,
+      net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM,
+      net::CERT_STATUS_WEAK_KEY,
+      net::CERT_STATUS_NAME_CONSTRAINT_VIOLATION,
+      net::CERT_STATUS_VALIDITY_TOO_LONG,
   };
 
   const ErrorType kErrorTypes[] = {
-    CERT_COMMON_NAME_INVALID,
-    CERT_DATE_INVALID,
-    CERT_AUTHORITY_INVALID,
-    CERT_NO_REVOCATION_MECHANISM,
-    CERT_UNABLE_TO_CHECK_REVOCATION,
-    CERT_REVOKED,
-    CERT_INVALID,
-    CERT_WEAK_SIGNATURE_ALGORITHM,
-    CERT_WEAK_KEY,
-    CERT_NAME_CONSTRAINT_VIOLATION,
+      CERT_COMMON_NAME_INVALID,
+      CERT_DATE_INVALID,
+      CERT_AUTHORITY_INVALID,
+      CERT_NO_REVOCATION_MECHANISM,
+      CERT_UNABLE_TO_CHECK_REVOCATION,
+      CERT_REVOKED,
+      CERT_INVALID,
+      CERT_WEAK_SIGNATURE_ALGORITHM,
+      CERT_WEAK_KEY,
+      CERT_NAME_CONSTRAINT_VIOLATION,
+      CERT_VALIDITY_TOO_LONG,
   };
   DCHECK(arraysize(kErrorFlags) == arraysize(kErrorTypes));
 
@@ -243,9 +254,10 @@ int SSLErrorInfo::GetErrorsForCertStatus(int cert_id,
             cert_id, &cert);
         DCHECK(r);
       }
-      if (errors)
+      if (errors) {
         errors->push_back(
             SSLErrorInfo::CreateError(kErrorTypes[i], cert.get(), url));
+      }
     }
   }
   return count;

diff --git a/chrome/browser/ssl/ssl_error_info.h b/chrome/browser/ssl/ssl_error_info.h
@@ -33,6 +33,7 @@ class SSLErrorInfo {
     CERT_WEAK_SIGNATURE_ALGORITHM,
     CERT_WEAK_KEY,
     CERT_NAME_CONSTRAINT_VIOLATION,
+    CERT_VALIDITY_TOO_LONG,
     UNKNOWN,
     CERT_WEAK_KEY_DH,
     CERT_PINNED_KEY_MISSING,

diff --git a/content/browser/ssl/ssl_policy.cc b/content/browser/ssl/ssl_policy.cc
@@ -56,6 +56,7 @@ void SSLPolicy::OnCertError(SSLCertErrorHandler* handler) {
     case net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM:
     case net::ERR_CERT_WEAK_KEY:
     case net::ERR_CERT_NAME_CONSTRAINT_VIOLATION:
+    case net::ERR_CERT_VALIDITY_TOO_LONG:
       if (!handler->fatal())
         options_mask |= OVERRIDABLE;
       else

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
@@ -440,13 +440,16 @@ NET_ERROR(CERT_WEAK_KEY, -211)
 // The certificate claimed DNS names that are in violation of name constraints.
 NET_ERROR(CERT_NAME_CONSTRAINT_VIOLATION, -212)
 
+// The certificate's validity period is too long.
+NET_ERROR(CERT_VALIDITY_TOO_LONG, -213)
+
 // Add new certificate error codes here.
 //
 // Update the value of CERT_END whenever you add a new certificate error
 // code.
 
 // The value immediately past the last certificate error code.
-NET_ERROR(CERT_END, -213)
+NET_ERROR(CERT_END, -214)
 
 // The URL is invalid.
 NET_ERROR(INVALID_URL, -300)

diff --git a/net/cert/cert_status_flags.cc b/net/cert/cert_status_flags.cc
@@ -49,6 +49,8 @@ CertStatus MapNetErrorToCertStatus(int error) {
       return CERT_STATUS_PINNED_KEY_MISSING;
     case ERR_CERT_NAME_CONSTRAINT_VIOLATION:
       return CERT_STATUS_NAME_CONSTRAINT_VIOLATION;
+    case ERR_CERT_VALIDITY_TOO_LONG:
+      return CERT_STATUS_VALIDITY_TOO_LONG;
     default:
       return 0;
   }
@@ -81,6 +83,8 @@ int MapCertStatusToNetError(CertStatus cert_status) {
     return ERR_CERT_WEAK_KEY;
   if (cert_status & CERT_STATUS_DATE_INVALID)
     return ERR_CERT_DATE_INVALID;
+  if (cert_status & CERT_STATUS_VALIDITY_TOO_LONG)
+    return ERR_CERT_VALIDITY_TOO_LONG;
 
   // Unknown status.  Give it the benefit of the doubt.
   if (cert_status & CERT_STATUS_UNABLE_TO_CHECK_REVOCATION)

diff --git a/net/cert/cert_status_flags_list.h b/net/cert/cert_status_flags_list.h
@@ -24,6 +24,7 @@ CERT_STATUS_FLAG(WEAK_KEY, 1 << 11)
 // 1 << 12 was used for CERT_STATUS_WEAK_DH_KEY
 CERT_STATUS_FLAG(PINNED_KEY_MISSING, 1 << 13)
 CERT_STATUS_FLAG(NAME_CONSTRAINT_VIOLATION, 1 << 14)
+CERT_STATUS_FLAG(VALIDITY_TOO_LONG, 1 << 15)
 
 // Bits 16 to 31 are for non-error statuses.
 CERT_STATUS_FLAG(IS_EV, 1 << 16)

diff --git a/net/cert/cert_verify_proc.cc b/net/cert/cert_verify_proc.cc
@@ -4,10 +4,13 @@
 
 #include "net/cert/cert_verify_proc.h"
 
+#include <stdint.h>
+
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
@@ -33,7 +36,6 @@
 #error Implement certificate verification.
 #endif
 
-
 namespace net {
 
 namespace {
@@ -276,6 +278,13 @@ int CertVerifyProc::Verify(X509Certificate* cert,
     // now treat it as a warning and do not map it to an error return value.
   }
 
+  // Flag certificates using too long validity periods.
+  if (verify_result->is_issued_by_known_root && HasTooLongValidity(*cert)) {
+    verify_result->cert_status |= CERT_STATUS_VALIDITY_TOO_LONG;
+    if (rv == OK)
+      rv = MapCertStatusToNetError(verify_result->cert_status);
+  }
+
   return rv;
 }
 
@@ -614,4 +623,41 @@ bool CertVerifyProc::HasNameConstraintsViolation(
   return false;
 }
 
+// static
+bool CertVerifyProc::HasTooLongValidity(const X509Certificate& cert) {
+  const base::Time& start = cert.valid_start();
+  const base::Time& expiry = cert.valid_expiry();
+  if (start.is_max() || start.is_null() || expiry.is_max() ||
+      expiry.is_null() || start > expiry) {
+    return true;
+  }
+
+  base::Time::Exploded exploded_start;
+  base::Time::Exploded exploded_expiry;
+  cert.valid_start().UTCExplode(&exploded_start);
+  cert.valid_expiry().UTCExplode(&exploded_expiry);
+
+  if (exploded_expiry.year - exploded_start.year > 10)
+    return true;
+  int month_diff = (exploded_expiry.year - exploded_start.year) * 12 +
+                   (exploded_expiry.month - exploded_start.month);
+
+  // Add any remainder as a full month.
+  if (exploded_expiry.day_of_month > exploded_start.day_of_month)
+    ++month_diff;
+
+  static const base::Time time_2015_04_01 =
+      base::Time::FromInternalValue(INT64_C(1427871600));
+  static const base::Time time_2012_07_01 =
+      base::Time::FromInternalValue(INT64_C(1341126000));
+  static const base::Time time_2019_07_01 =
+      base::Time::FromInternalValue(INT64_C(1561964400));
+
+  if (start >= time_2015_04_01)
+    return month_diff > 39;
+  if (start >= time_2012_07_01)
+    return month_diff > 60;
+  return month_diff > 120 || expiry > time_2019_07_01;
+}
+
 }  // namespace net
diff --git a/net/cert/cert_verify_proc.h b/net/cert/cert_verify_proc.h
@@ -73,6 +73,7 @@ class NET_EXPORT CertVerifyProc
  private:
   friend class base::RefCountedThreadSafe<CertVerifyProc>;
   FRIEND_TEST_ALL_PREFIXES(CertVerifyProcTest, DigiNotarCerts);
+  FRIEND_TEST_ALL_PREFIXES(CertVerifyProcTest, TestHasTooLongValidity);
 
   // Performs the actual verification using the desired underlying
   // cryptographic library.
@@ -99,6 +100,18 @@ class NET_EXPORT CertVerifyProc
       const std::vector<std::string>& dns_names,
       const std::vector<std::string>& ip_addrs);
 
+  // The CA/Browser Forum's Baseline Requirements specify maximum validity
+  // periods (https://cabforum.org/Baseline_Requirements_V1.pdf):
+  //
+  // For certificates issued after 1 July 2012: 60 months.
+  // For certificates issued after 1 April 2015: 39 months.
+  //
+  // For certificates issued before the BRs took effect, there were no
+  // guidelines, but clamp them at a maximum of 10 year validity, with the
+  // requirement they expire within 7 years after the effective date of the BRs
+  // (i.e. by 1 July 2019).
+  static bool HasTooLongValidity(const X509Certificate& cert);
+
   DISALLOW_COPY_AND_ASSIGN(CertVerifyProc);
 };
 

diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc
@@ -615,16 +615,36 @@ TEST_F(CertVerifyProcTest, NameConstraintsFailure) {
             verify_result.cert_status & CERT_STATUS_NAME_CONSTRAINT_VIOLATION);
 }
 
+TEST_F(CertVerifyProcTest, TestHasTooLongValidity) {
+  base::FilePath certs_dir = GetTestCertsDirectory();
+
+  scoped_refptr<X509Certificate> twitter =
+      ImportCertFromFile(certs_dir, "twitter-chain.pem");
+  EXPECT_FALSE(CertVerifyProc::HasTooLongValidity(*twitter));
+
+  scoped_refptr<X509Certificate> eleven_years =
+      ImportCertFromFile(certs_dir, "11_year_validity.pem");
+  EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*eleven_years));
+
+  scoped_refptr<X509Certificate> forty_months =
+      ImportCertFromFile(certs_dir, "40_months_after_2015_04.pem");
+  EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*forty_months));
+
+  scoped_refptr<X509Certificate> sixty_one_months =
+      ImportCertFromFile(certs_dir, "61_months_after_2012_07.pem");
+  EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*sixty_one_months));
+}
+
 TEST_F(CertVerifyProcTest, TestKnownRoot) {
   if (!SupportsDetectingKnownRoots()) {
-    LOG(INFO) << "Skipping this test in this platform.";
+    LOG(INFO) << "Skipping this test on this platform.";
     return;
   }
 
   base::FilePath certs_dir = GetTestCertsDirectory();
   CertificateList certs = CreateCertificateListFromFile(
-      certs_dir, "satveda.pem", X509Certificate::FORMAT_AUTO);
-  ASSERT_EQ(2U, certs.size());
+      certs_dir, "twitter-chain.pem", X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(3U, certs.size());
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(certs[1]->os_cert_handle());
@@ -635,20 +655,18 @@ TEST_F(CertVerifyProcTest, TestKnownRoot) {
 
   int flags = 0;
   CertVerifyResult verify_result;
-  // This will blow up, May 24th, 2019. Sorry! Please disable and file a bug
+  // This will blow up, May 9th, 2016. Sorry! Please disable and file a bug
   // against agl. See also PublicKeyHashes.
   int error = Verify(cert_chain.get(),
-                     "satveda.com",
+                     "twitter.com",
                      flags,
                      NULL,
                      empty_cert_list_,
                      &verify_result);
   EXPECT_EQ(OK, error);
-  EXPECT_EQ(CERT_STATUS_SHA1_SIGNATURE_PRESENT, verify_result.cert_status);
   EXPECT_TRUE(verify_result.is_issued_by_known_root);
 }
 
-// The certse.pem certificate has been revoked. crbug.com/259723.
 TEST_F(CertVerifyProcTest, PublicKeyHashes) {
   if (!SupportsReturningVerifiedChain()) {
     LOG(INFO) << "Skipping this test in this platform.";
@@ -657,8 +675,8 @@ TEST_F(CertVerifyProcTest, PublicKeyHashes) {
 
   base::FilePath certs_dir = GetTestCertsDirectory();
   CertificateList certs = CreateCertificateListFromFile(
-      certs_dir, "satveda.pem", X509Certificate::FORMAT_AUTO);
-  ASSERT_EQ(2U, certs.size());
+      certs_dir, "twitter-chain.pem", X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(3U, certs.size());
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(certs[1]->os_cert_handle());
@@ -669,28 +687,27 @@ TEST_F(CertVerifyProcTest, PublicKeyHashes) {
   int flags = 0;
   CertVerifyResult verify_result;
 
-  // This will blow up, May 24th, 2019. Sorry! Please disable and file a bug
+  // This will blow up, May 9th, 2016. Sorry! Please disable and file a bug
   // against agl. See also TestKnownRoot.
   int error = Verify(cert_chain.get(),
-                     "satveda.com",
+                     "twitter.com",
                      flags,
                      NULL,
                      empty_cert_list_,
                      &verify_result);
   EXPECT_EQ(OK, error);
-  EXPECT_EQ(CERT_STATUS_SHA1_SIGNATURE_PRESENT, verify_result.cert_status);
-  ASSERT_LE(2U, verify_result.public_key_hashes.size());
+  ASSERT_LE(3U, verify_result.public_key_hashes.size());
 
   HashValueVector sha1_hashes;
   for (size_t i = 0; i < verify_result.public_key_hashes.size(); ++i) {
     if (verify_result.public_key_hashes[i].tag != HASH_VALUE_SHA1)
       continue;
     sha1_hashes.push_back(verify_result.public_key_hashes[i]);
   }
-  ASSERT_LE(2u, sha1_hashes.size());
+  ASSERT_LE(3u, sha1_hashes.size());
 
-  for (size_t i = 0; i < 2; ++i) {
-    EXPECT_EQ(HexEncode(kSatvedaSPKIs[i], base::kSHA1Length),
+  for (size_t i = 0; i < 3; ++i) {
+    EXPECT_EQ(HexEncode(kTwitterSPKIs[i], base::kSHA1Length),
               HexEncode(sha1_hashes[i].data(), base::kSHA1Length));
   }
 
@@ -700,10 +717,10 @@ TEST_F(CertVerifyProcTest, PublicKeyHashes) {
       continue;
     sha256_hashes.push_back(verify_result.public_key_hashes[i]);
   }
-  ASSERT_LE(2u, sha256_hashes.size());
+  ASSERT_LE(3u, sha256_hashes.size());
 
-  for (size_t i = 0; i < 2; ++i) {
-    EXPECT_EQ(HexEncode(kSatvedaSPKIsSHA256[i], crypto::kSHA256Length),
+  for (size_t i = 0; i < 3; ++i) {
+    EXPECT_EQ(HexEncode(kTwitterSPKIsSHA256[i], crypto::kSHA256Length),
               HexEncode(sha256_hashes[i].data(), crypto::kSHA256Length));
   }
 }
@@ -810,7 +827,7 @@ TEST_F(CertVerifyProcTest, IntranetHostsRejected) {
   }
 
   CertificateList cert_list = CreateCertificateListFromFile(
-      GetTestCertsDirectory(), "ok_cert.pem",
+      GetTestCertsDirectory(), "reject_intranet_hosts.pem",
       X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1U, cert_list.size());
   scoped_refptr<X509Certificate> cert(cert_list[0]);