Bug 1747320 - Only query CRLite on covered certificates. r=keeler

Differential Revision: https://phabricator.services.mozilla.com/D134566

modules/libpref/init/all.js

@@ -161,10 +161,6 @@ 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/CRLiteTimestamp.h

@@ -0,0 +1,33 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef CRLiteTimestamp_h
+#define CRLiteTimestamp_h
+
+#include "nsICertStorage.h"
+#include "SignedCertificateTimestamp.h"
+
+namespace mozilla::psm {
+
+class CRLiteTimestamp final : public nsICRLiteTimestamp {
+ public:
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSICRLITETIMESTAMP
+
+  CRLiteTimestamp() : mTimestamp(0) {}
+  explicit CRLiteTimestamp(const ct::SignedCertificateTimestamp& sct)
+      : mLogID(Span(sct.logId)), mTimestamp(sct.timestamp) {}
+
+ private:
+  ~CRLiteTimestamp() = default;
+
+  nsTArray<uint8_t> mLogID;
+  uint64_t mTimestamp;
+};
+
+}  // namespace mozilla::psm
+
+#endif  // CRLiteTimestamp_h

security/certverifier/CertVerifier.cpp

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

security/certverifier/CertVerifier.h

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

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -9,6 +9,7 @@
 #include <stdint.h>
 #include <utility>
 
+#include "CRLiteTimestamp.h"
 #include "ExtendedValidation.h"
 #include "MultiLogCTVerifier.h"
 #include "NSSErrorsService.h"
@@ -72,8 +73,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
     TimeDuration ocspTimeoutHard, uint32_t certShortLifetimeInDays,
     unsigned int minRSABits, ValidityCheckingMode validityCheckingMode,
     CertVerifier::SHA1Mode sha1Mode, NetscapeStepUpPolicy netscapeStepUpPolicy,
-    CRLiteMode crliteMode, uint64_t crliteCTMergeDelaySeconds,
-    const OriginAttributes& originAttributes,
+    CRLiteMode crliteMode, const OriginAttributes& originAttributes,
     const Vector<Input>& thirdPartyRootInputs,
     const Vector<Input>& thirdPartyIntermediateInputs,
     const Maybe<nsTArray<nsTArray<uint8_t>>>& extraCertificates,
@@ -92,7 +92,6 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
       mSHA1Mode(sha1Mode),
       mNetscapeStepUpPolicy(netscapeStepUpPolicy),
       mCRLiteMode(crliteMode),
-      mCRLiteCTMergeDelaySeconds(crliteCTMergeDelaySeconds),
       mSawDistrustedCAByPolicyError(false),
       mOriginAttributes(originAttributes),
       mThirdPartyRootInputs(thirdPartyRootInputs),
@@ -600,10 +599,24 @@ static Result GetOCSPAuthorityInfoAccessLocation(const UniquePLArenaPool& arena,
   return Success;
 }
 
-Result GetEarliestSCTTimestamp(Input sctExtension,
-                               Maybe<uint64_t>& earliestTimestamp) {
-  earliestTimestamp.reset();
+NS_IMPL_ISUPPORTS(CRLiteTimestamp, nsICRLiteTimestamp)
 
+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);
@@ -614,10 +627,9 @@ Result GetEarliestSCTTimestamp(Input sctExtension,
   size_t decodingErrors;
   DecodeSCTs(sctList, decodedSCTs, decodingErrors);
   Unused << decodingErrors;
-  for (const auto& scts : decodedSCTs) {
-    if (!earliestTimestamp.isSome() || scts.timestamp < *earliestTimestamp) {
-      earliestTimestamp = Some(scts.timestamp);
-    }
+
+  for (const auto& sct : decodedSCTs) {
+    timestamps.AppendElement(new CRLiteTimestamp(sct));
   }
   return Success;
 }
@@ -649,65 +661,43 @@ Result NSSCertDBTrustDomain::CheckCRLiteStash(
 Result NSSCertDBTrustDomain::CheckCRLite(
     const nsTArray<uint8_t>& issuerBytes,
     const nsTArray<uint8_t>& issuerSubjectPublicKeyInfoBytes,
-    const nsTArray<uint8_t>& serialNumberBytes, uint64_t earliestSCTTimestamp,
-    bool& filterCoversCertificate) {
+    const nsTArray<uint8_t>& serialNumberBytes,
+    const nsTArray<RefPtr<nsICRLiteTimestamp>>& timestamps,
+    /*out*/ bool& filterCoversCertificate) {
   filterCoversCertificate = false;
-  uint64_t filterTimestamp;
   int16_t crliteRevocationState;
   nsresult rv = mCertStorage->GetCRLiteRevocationState(
       issuerBytes, issuerSubjectPublicKeyInfoBytes, serialNumberBytes,
-      &filterTimestamp, &crliteRevocationState);
+      timestamps, &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 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;
-  }
+           "state=%hd",
+           crliteRevocationState));
 
-  return Success;
+  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;
+  }
 }
 
 Result NSSCertDBTrustDomain::CheckRevocation(
@@ -725,20 +715,10 @@ 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 && earliestSCTTimestamp.isSome()) {
+      mCRLiteMode != CRLiteMode::Disabled && sctExtension) {
     MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
             ("NSSCertDBTrustDomain::CheckRevocation: checking CRLite"));
     nsTArray<uint8_t> issuerSubjectPublicKeyInfoBytes;
@@ -755,22 +735,32 @@ Result NSSCertDBTrustDomain::CheckRevocation(
     if (rv != Success) {
       return rv;
     }
+
     nsTArray<uint8_t> issuerBytes;
     issuerBytes.AppendElements(certID.issuer.UnsafeGetData(),
                                certID.issuer.GetLength());
-    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;
+
+    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;
+      }
     }
   }