Backed out changeset d4a6f5cb9b3f (bug 1747320) for breaking connecti…

…vity with many https sites (bug 1750188) a=backout

modules/libpref/init/all.js

@@ -161,6 +161,10 @@ pref("security.xfocsp.errorReporting.automatic", false);
 // 2: Enable and enforce revocations via CRLite
 pref("security.pki.crlite_mode", 1);
 
+// Represents the expected certificate transparency log merge delay (including
+// the time to generate a CRLite filter). Currently 28 hours in seconds.
+pref("security.pki.crlite_ct_merge_delay_seconds", 100800);
+
 // Issuer we use to detect MitM proxies. Set to the issuer of the cert of the
 // Firefox update service. The string format is whatever NSS uses to print a DN.
 // This value is set and cleared automatically.

security/certverifier/CertVerifier.cpp

@@ -109,6 +109,7 @@ CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc,
                            NetscapeStepUpPolicy netscapeStepUpPolicy,
                            CertificateTransparencyMode ctMode,
                            CRLiteMode crliteMode,
+                           uint64_t crliteCTMergeDelaySeconds,
                            const Vector<EnterpriseCert>& thirdPartyCerts)
     : mOCSPDownloadConfig(odc),
       mOCSPStrict(osc == ocspStrict),
@@ -119,7 +120,8 @@ CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc,
       mNameMatchingMode(nameMatchingMode),
       mNetscapeStepUpPolicy(netscapeStepUpPolicy),
       mCTMode(ctMode),
-      mCRLiteMode(crliteMode) {
+      mCRLiteMode(crliteMode),
+      mCRLiteCTMergeDelaySeconds(crliteCTMergeDelaySeconds) {
   LoadKnownCTLogs();
   for (const auto& root : thirdPartyCerts) {
     EnterpriseCert rootCopy;
@@ -578,9 +580,10 @@ Result CertVerifier::VerifyCert(
           trustEmail, defaultOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft,
           mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS_WEAK,
           ValidityCheckingMode::CheckingOff, SHA1Mode::Allowed,
-          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode,
+          mCRLiteCTMergeDelaySeconds, originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
       rv = BuildCertChain(
           trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity,
           KeyUsage::digitalSignature, KeyPurposeId::id_kp_clientAuth,
@@ -649,9 +652,10 @@ Result CertVerifier::VerifyCert(
             trustSSL, evOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft,
             mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS,
             ValidityCheckingMode::CheckForEV, sha1ModeConfigurations[i],
-            mNetscapeStepUpPolicy, mCRLiteMode, originAttributes,
-            mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-            extraCertificates, builtChain, pinningTelemetryInfo, hostname);
+            mNetscapeStepUpPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+            originAttributes, mThirdPartyRootInputs,
+            mThirdPartyIntermediateInputs, extraCertificates, builtChain,
+            pinningTelemetryInfo, hostname);
         rv = BuildCertChainForOneKeyUsage(
             trustDomain, certDER, time,
             KeyUsage::digitalSignature,  // (EC)DHE
@@ -729,9 +733,9 @@ Result CertVerifier::VerifyCert(
               mOCSPTimeoutSoft, mOCSPTimeoutHard, mCertShortLifetimeInDays,
               keySizeOptions[i], ValidityCheckingMode::CheckingOff,
               sha1ModeConfigurations[j], mNetscapeStepUpPolicy, mCRLiteMode,
-              originAttributes, mThirdPartyRootInputs,
-              mThirdPartyIntermediateInputs, extraCertificates, builtChain,
-              pinningTelemetryInfo, hostname);
+              mCRLiteCTMergeDelaySeconds, originAttributes,
+              mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
+              extraCertificates, builtChain, pinningTelemetryInfo, hostname);
           rv = BuildCertChainForOneKeyUsage(
               trustDomain, certDER, time,
               KeyUsage::digitalSignature,  //(EC)DHE
@@ -796,9 +800,10 @@ Result CertVerifier::VerifyCert(
           trustSSL, defaultOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft,
           mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS_WEAK,
           ValidityCheckingMode::CheckingOff, SHA1Mode::Allowed,
-          mNetscapeStepUpPolicy, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          mNetscapeStepUpPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+          originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
       rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeCA,
                           KeyUsage::keyCertSign, KeyPurposeId::id_kp_serverAuth,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
@@ -810,9 +815,10 @@ Result CertVerifier::VerifyCert(
           trustEmail, defaultOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft,
           mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS_WEAK,
           ValidityCheckingMode::CheckingOff, SHA1Mode::Allowed,
-          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode,
+          mCRLiteCTMergeDelaySeconds, originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
       rv = BuildCertChain(
           trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity,
           KeyUsage::digitalSignature, KeyPurposeId::id_kp_emailProtection,
@@ -834,9 +840,10 @@ Result CertVerifier::VerifyCert(
           trustEmail, defaultOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft,
           mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS_WEAK,
           ValidityCheckingMode::CheckingOff, SHA1Mode::Allowed,
-          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          NetscapeStepUpPolicy::NeverMatch, mCRLiteMode,
+          mCRLiteCTMergeDelaySeconds, originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::keyEncipherment,  // RSA

security/certverifier/CertVerifier.h

@@ -222,6 +222,7 @@ class CertVerifier {
                BRNameMatchingPolicy::Mode nameMatchingMode,
                NetscapeStepUpPolicy netscapeStepUpPolicy,
                CertificateTransparencyMode ctMode, CRLiteMode crliteMode,
+               uint64_t crliteCTMergeDelaySeconds,
                const Vector<EnterpriseCert>& thirdPartyCerts);
   ~CertVerifier();
 
@@ -237,6 +238,7 @@ class CertVerifier {
   const NetscapeStepUpPolicy mNetscapeStepUpPolicy;
   const CertificateTransparencyMode mCTMode;
   const CRLiteMode mCRLiteMode;
+  const uint64_t mCRLiteCTMergeDelaySeconds;
 
  private:
   OCSPCache mOCSPCache;

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -9,7 +9,6 @@
 #include <stdint.h>
 #include <utility>
 
-#include "CRLiteTimestamp.h"
 #include "ExtendedValidation.h"
 #include "MultiLogCTVerifier.h"
 #include "NSSErrorsService.h"
@@ -73,7 +72,8 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
     TimeDuration ocspTimeoutHard, uint32_t certShortLifetimeInDays,
     unsigned int minRSABits, ValidityCheckingMode validityCheckingMode,
     CertVerifier::SHA1Mode sha1Mode, NetscapeStepUpPolicy netscapeStepUpPolicy,
-    CRLiteMode crliteMode, const OriginAttributes& originAttributes,
+    CRLiteMode crliteMode, uint64_t crliteCTMergeDelaySeconds,
+    const OriginAttributes& originAttributes,
     const Vector<Input>& thirdPartyRootInputs,
     const Vector<Input>& thirdPartyIntermediateInputs,
     const Maybe<nsTArray<nsTArray<uint8_t>>>& extraCertificates,
@@ -92,6 +92,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
       mSHA1Mode(sha1Mode),
       mNetscapeStepUpPolicy(netscapeStepUpPolicy),
       mCRLiteMode(crliteMode),
+      mCRLiteCTMergeDelaySeconds(crliteCTMergeDelaySeconds),
       mSawDistrustedCAByPolicyError(false),
       mOriginAttributes(originAttributes),
       mThirdPartyRootInputs(thirdPartyRootInputs),
@@ -599,24 +600,10 @@ static Result GetOCSPAuthorityInfoAccessLocation(const UniquePLArenaPool& arena,
   return Success;
 }
 
-NS_IMPL_ISUPPORTS(CRLiteTimestamp, nsICRLiteTimestamp)
+Result GetEarliestSCTTimestamp(Input sctExtension,
+                               Maybe<uint64_t>& earliestTimestamp) {
+  earliestTimestamp.reset();
 
-NS_IMETHODIMP
-CRLiteTimestamp::GetLogID(nsTArray<uint8_t>& aLogID) {
-  aLogID.Clear();
-  aLogID.AppendElements(mLogID);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-CRLiteTimestamp::GetTimestamp(uint64_t* aTimestamp) {
-  *aTimestamp = mTimestamp;
-  return NS_OK;
-}
-
-Result BuildCRLiteTimestampArray(
-    Input sctExtension,
-    /*out*/ nsTArray<RefPtr<nsICRLiteTimestamp>>& timestamps) {
   Input sctList;
   Result rv =
       ExtractSignedCertificateTimestampListFromExtension(sctExtension, sctList);
@@ -627,9 +614,10 @@ Result BuildCRLiteTimestampArray(
   size_t decodingErrors;
   DecodeSCTs(sctList, decodedSCTs, decodingErrors);
   Unused << decodingErrors;
-
-  for (const auto& sct : decodedSCTs) {
-    timestamps.AppendElement(new CRLiteTimestamp(sct));
+  for (const auto& scts : decodedSCTs) {
+    if (!earliestTimestamp.isSome() || scts.timestamp < *earliestTimestamp) {
+      earliestTimestamp = Some(scts.timestamp);
+    }
   }
   return Success;
 }
@@ -661,43 +649,65 @@ Result NSSCertDBTrustDomain::CheckCRLiteStash(
 Result NSSCertDBTrustDomain::CheckCRLite(
     const nsTArray<uint8_t>& issuerBytes,
     const nsTArray<uint8_t>& issuerSubjectPublicKeyInfoBytes,
-    const nsTArray<uint8_t>& serialNumberBytes,
-    const nsTArray<RefPtr<nsICRLiteTimestamp>>& timestamps,
-    /*out*/ bool& filterCoversCertificate) {
+    const nsTArray<uint8_t>& serialNumberBytes, uint64_t earliestSCTTimestamp,
+    bool& filterCoversCertificate) {
   filterCoversCertificate = false;
+  uint64_t filterTimestamp;
   int16_t crliteRevocationState;
   nsresult rv = mCertStorage->GetCRLiteRevocationState(
       issuerBytes, issuerSubjectPublicKeyInfoBytes, serialNumberBytes,
-      timestamps, &crliteRevocationState);
+      &filterTimestamp, &crliteRevocationState);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
             ("NSSCertDBTrustDomain::CheckCRLite: CRLite call failed"));
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
   MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
           ("NSSCertDBTrustDomain::CheckCRLite: CRLite check returned "
-           "state=%hd",
-           crliteRevocationState));
-
-  switch (crliteRevocationState) {
-    case nsICertStorage::STATE_ENFORCE:
-      filterCoversCertificate = true;
-      return Result::ERROR_REVOKED_CERTIFICATE;
-    case nsICertStorage::STATE_UNSET:
-      filterCoversCertificate = true;
-      return Success;
-    case nsICertStorage::STATE_NOT_ENROLLED:
-      filterCoversCertificate = false;
-      return Success;
-    case nsICertStorage::STATE_NOT_COVERED:
-      filterCoversCertificate = false;
-      return Success;
-    default:
-      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-              ("NSSCertDBTrustDomain::CheckCRLite: Unknown CRLite revocation "
-               "state"));
-      return Result::FATAL_ERROR_LIBRARY_FAILURE;
+           "state=%hd filter timestamp=%llu",
+           crliteRevocationState,
+           // The cast is to silence warnings on compilers where uint64_t is
+           // an unsigned long as opposed to an unsigned long long.
+           static_cast<unsigned long long>(filterTimestamp)));
+  Time filterTimestampTime(TimeFromEpochInSeconds(filterTimestamp));
+  // We can only use this result if the earliest embedded signed
+  // certificate timestamp from the certificate is older than what cert
+  // storage returned for its CRLite timestamp. Otherwise, the CRLite
+  // filter cascade may have been created before this certificate existed,
+  // and if it would create a false positive, it hasn't been accounted for.
+  // SCT timestamps are milliseconds since the epoch.
+  Time earliestCertificateTimestamp(
+      TimeFromEpochInSeconds(earliestSCTTimestamp / 1000));
+  Result result =
+      earliestCertificateTimestamp.AddSeconds(mCRLiteCTMergeDelaySeconds);
+  if (result != Success) {
+    // This shouldn't happen - the merge delay is at most a year in seconds,
+    // and the SCT timestamp is supposed to be in the past.
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain::CheckRevocation: integer overflow "
+             "calculating sct timestamp + merge delay (%llu + %llu)",
+             static_cast<unsigned long long>(earliestSCTTimestamp / 1000),
+             static_cast<unsigned long long>(mCRLiteCTMergeDelaySeconds)));
+    // While we do have control over the possible values of the CT merge
+    // delay parameter, we don't have control over the SCT timestamp.
+    // Thus, if we've reached this point, the CA has probably made a
+    // mistake and we should treat this certificate as revoked.
+    return Result::ERROR_REVOKED_CERTIFICATE;
+  }
+  if (filterTimestamp != 0 &&
+      earliestCertificateTimestamp <= filterTimestampTime &&
+      crliteRevocationState != nsICertStorage::STATE_NOT_ENROLLED) {
+    filterCoversCertificate = true;
+  }
+  if (filterCoversCertificate &&
+      crliteRevocationState == nsICertStorage::STATE_ENFORCE) {
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain::CheckRevocation: certificate revoked via "
+             "CRLite"));
+    return Result::ERROR_REVOKED_CERTIFICATE;
   }
+
+  return Success;
 }
 
 Result NSSCertDBTrustDomain::CheckRevocation(
@@ -715,10 +725,20 @@ Result NSSCertDBTrustDomain::CheckRevocation(
   MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
           ("NSSCertDBTrustDomain: Top of CheckRevocation\n"));
 
+  Maybe<uint64_t> earliestSCTTimestamp = Nothing();
+  if (sctExtension) {
+    Result rv = GetEarliestSCTTimestamp(*sctExtension, earliestSCTTimestamp);
+    if (rv != Success) {
+      MOZ_LOG(
+          gCertVerifierLog, LogLevel::Debug,
+          ("decoding SCT extension failed - CRLite will be not be consulted"));
+    }
+  }
+
   bool crliteFilterCoversCertificate = false;
   Result crliteResult = Success;
   if (endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
-      mCRLiteMode != CRLiteMode::Disabled && sctExtension) {
+      mCRLiteMode != CRLiteMode::Disabled && earliestSCTTimestamp.isSome()) {
     MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
             ("NSSCertDBTrustDomain::CheckRevocation: checking CRLite"));
     nsTArray<uint8_t> issuerSubjectPublicKeyInfoBytes;
@@ -735,32 +755,22 @@ Result NSSCertDBTrustDomain::CheckRevocation(
     if (rv != Success) {
       return rv;
     }
-
     nsTArray<uint8_t> issuerBytes;
     issuerBytes.AppendElements(certID.issuer.UnsafeGetData(),
                                certID.issuer.GetLength());
-
-    nsTArray<RefPtr<nsICRLiteTimestamp>> timestamps;
-    rv = BuildCRLiteTimestampArray(*sctExtension, timestamps);
-    if (rv != Success) {
-      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-              ("decoding SCT extension failed - CRLite will be not be "
-               "consulted"));
-    } else {
-      crliteResult = CheckCRLite(issuerBytes, issuerSubjectPublicKeyInfoBytes,
-                                 serialNumberBytes, timestamps,
-                                 crliteFilterCoversCertificate);
-      // If CheckCRLite returned an error other than "revoked certificate",
-      // propagate that error.
-      if (crliteResult != Success &&
-          crliteResult != Result::ERROR_REVOKED_CERTIFICATE) {
-        return crliteResult;
-      }
-      // Always return the result of CheckCRLite if CRLite is being enforced and
-      // the certificate is covered by the CRLite filter.
-      if (mCRLiteMode == CRLiteMode::Enforce && crliteFilterCoversCertificate) {
-        return crliteResult;
-      }
+    crliteResult = CheckCRLite(issuerBytes, issuerSubjectPublicKeyInfoBytes,
+                               serialNumberBytes, *earliestSCTTimestamp,
+                               crliteFilterCoversCertificate);
+    // If CheckCRLite returned an error other than "revoked certificate",
+    // propagate that error.
+    if (crliteResult != Success &&
+        crliteResult != Result::ERROR_REVOKED_CERTIFICATE) {
+      return crliteResult;
+    }
+    // Always return the result of CheckCRLite if CRLite is being enforced and
+    // the certificate is covered by the CRLite filter.
+    if (mCRLiteMode == CRLiteMode::Enforce && crliteFilterCoversCertificate) {
+      return crliteResult;
     }
   }