Bug 1667829 - CRLite: allow taking the log merge delay into account r=jcj

This patch adds the preference "security.pki.crlite_ct_merge_delay_seconds" that adds a configurable delay between the earliest certificate timestamp and the filter creation date. This allows the implementation to take into account CT log merge delays (i.e. when an SCT exists for a certificate but that certificate hasn't yet been merged into the log). The default value is 28 hours in seconds. The minimum value is 0 seconds, and the maximum value is one year in seconds. Differential Revision: https://phabricator.services.mozilla.com/D92295
2020-10-07 00:16:49 +00:00 · 2020-10-07 00:16:49 +00:00 · e0531b8283
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@ -185,6 +185,10 @@ pref("security.pki.distrust_ca_policy", 2);
 // 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.
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@ -92,6 +92,7 @@ CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc,
                           CertificateTransparencyMode ctMode,
                           DistrustedCAPolicy distrustedCAPolicy,
                           CRLiteMode crliteMode,
+                           uint64_t crliteCTMergeDelaySeconds,
                           const Vector<EnterpriseCert>& thirdPartyCerts)
    : mOCSPDownloadConfig(odc),
      mOCSPStrict(osc == ocspStrict),
@ -104,7 +105,8 @@ CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc,
      mNetscapeStepUpPolicy(netscapeStepUpPolicy),
      mCTMode(ctMode),
      mDistrustedCAPolicy(distrustedCAPolicy),
-      mCRLiteMode(crliteMode) {
+      mCRLiteMode(crliteMode),
+      mCRLiteCTMergeDelaySeconds(crliteCTMergeDelaySeconds) {
  LoadKnownCTLogs();
  for (const auto& root : thirdPartyCerts) {
    EnterpriseCert rootCopy;
@ -566,9 +568,10 @@ Result CertVerifier::VerifyCert(
          mOCSPTimeoutHard, mCertShortLifetimeInDays, pinningDisabled,
          MIN_RSA_BITS_WEAK, ValidityCheckingMode::CheckingOff,
          SHA1Mode::Allowed, NetscapeStepUpPolicy::NeverMatch,
-          mDistrustedCAPolicy, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          mDistrustedCAPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+          originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
      rv = BuildCertChain(
          trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity,
          KeyUsage::digitalSignature, KeyPurposeId::id_kp_clientAuth,
@ -643,10 +646,10 @@ Result CertVerifier::VerifyCert(
            mOCSPTimeoutHard, mCertShortLifetimeInDays, mPinningMode,
            MIN_RSA_BITS, ValidityCheckingMode::CheckForEV,
            sha1ModeConfigurations[i], mNetscapeStepUpPolicy,
-            mDistrustedCAPolicy, mCRLiteMode, originAttributes,
-            mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-            extraCertificates, builtChain, pinningTelemetryInfo, crliteInfo,
-            hostname);
+            mDistrustedCAPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+            originAttributes, mThirdPartyRootInputs,
+            mThirdPartyIntermediateInputs, extraCertificates, builtChain,
+            pinningTelemetryInfo, crliteInfo, hostname);
        rv = BuildCertChainForOneKeyUsage(
            trustDomain, certDER, time,
            KeyUsage::digitalSignature,  // (EC)DHE
@ -730,9 +733,10 @@ Result CertVerifier::VerifyCert(
              mPinningMode, keySizeOptions[i],
              ValidityCheckingMode::CheckingOff, sha1ModeConfigurations[j],
              mNetscapeStepUpPolicy, mDistrustedCAPolicy, mCRLiteMode,
-              originAttributes, mThirdPartyRootInputs,
-              mThirdPartyIntermediateInputs, extraCertificates, builtChain,
-              pinningTelemetryInfo, crliteInfo, hostname);
+              mCRLiteCTMergeDelaySeconds, originAttributes,
+              mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
+              extraCertificates, builtChain, pinningTelemetryInfo, crliteInfo,
+              hostname);
          rv = BuildCertChainForOneKeyUsage(
              trustDomain, certDER, time,
              KeyUsage::digitalSignature,  //(EC)DHE
@ -800,9 +804,9 @@ Result CertVerifier::VerifyCert(
          mOCSPTimeoutHard, mCertShortLifetimeInDays, pinningDisabled,
          MIN_RSA_BITS_WEAK, ValidityCheckingMode::CheckingOff,
          SHA1Mode::Allowed, mNetscapeStepUpPolicy, mDistrustedCAPolicy,
-          mCRLiteMode, originAttributes, mThirdPartyRootInputs,
-          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
-          nullptr);
+          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);
@ -815,9 +819,10 @@ Result CertVerifier::VerifyCert(
          mOCSPTimeoutHard, mCertShortLifetimeInDays, pinningDisabled,
          MIN_RSA_BITS_WEAK, ValidityCheckingMode::CheckingOff,
          SHA1Mode::Allowed, NetscapeStepUpPolicy::NeverMatch,
-          mDistrustedCAPolicy, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          mDistrustedCAPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+          originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
      rv = BuildCertChain(
          trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity,
          KeyUsage::digitalSignature, KeyPurposeId::id_kp_emailProtection,
@ -840,9 +845,10 @@ Result CertVerifier::VerifyCert(
          mOCSPTimeoutHard, mCertShortLifetimeInDays, pinningDisabled,
          MIN_RSA_BITS_WEAK, ValidityCheckingMode::CheckingOff,
          SHA1Mode::Allowed, NetscapeStepUpPolicy::NeverMatch,
-          mDistrustedCAPolicy, mCRLiteMode, originAttributes,
-          mThirdPartyRootInputs, mThirdPartyIntermediateInputs,
-          extraCertificates, builtChain, nullptr, nullptr);
+          mDistrustedCAPolicy, mCRLiteMode, mCRLiteCTMergeDelaySeconds,
+          originAttributes, mThirdPartyRootInputs,
+          mThirdPartyIntermediateInputs, extraCertificates, builtChain, nullptr,
+          nullptr);
      rv = BuildCertChain(trustDomain, certDER, time,
                          EndEntityOrCA::MustBeEndEntity,
                          KeyUsage::keyEncipherment,  // RSA
--- a/security/certverifier/CertVerifier.h
+++ b/security/certverifier/CertVerifier.h
@ -264,6 +264,7 @@ class CertVerifier {
               NetscapeStepUpPolicy netscapeStepUpPolicy,
               CertificateTransparencyMode ctMode,
               DistrustedCAPolicy distrustedCAPolicy, CRLiteMode crliteMode,
+               uint64_t crliteCTMergeDelaySeconds,
               const Vector<EnterpriseCert>& thirdPartyCerts);
  ~CertVerifier();

@ -281,6 +282,7 @@ class CertVerifier {
  const CertificateTransparencyMode mCTMode;
  const DistrustedCAPolicy mDistrustedCAPolicy;
  const CRLiteMode mCRLiteMode;
+  const uint64_t mCRLiteCTMergeDelaySeconds;

 private:
  OCSPCache mOCSPCache;
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@ -74,6 +74,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
    ValidityCheckingMode validityCheckingMode, CertVerifier::SHA1Mode sha1Mode,
    NetscapeStepUpPolicy netscapeStepUpPolicy,
    DistrustedCAPolicy distrustedCAPolicy, CRLiteMode crliteMode,
+    uint64_t crliteCTMergeDelaySeconds,
    const OriginAttributes& originAttributes,
    const Vector<Input>& thirdPartyRootInputs,
    const Vector<Input>& thirdPartyIntermediateInputs,
@ -96,6 +97,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
      mNetscapeStepUpPolicy(netscapeStepUpPolicy),
      mDistrustedCAPolicy(distrustedCAPolicy),
      mCRLiteMode(crliteMode),
+      mCRLiteCTMergeDelaySeconds(crliteCTMergeDelaySeconds),
      mSawDistrustedCAByPolicyError(false),
      mOriginAttributes(originAttributes),
      mThirdPartyRootInputs(thirdPartyRootInputs),
@ -711,6 +713,27 @@ Result NSSCertDBTrustDomain::CheckRevocation(
      // 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)));
+        if (mCRLiteMode == CRLiteMode::Enforce) {
+          // 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 Time::AddSeconds fails, the original value is unchanged. Since in
+        // this case `earliestCertificateTimestamp` must represent a value far
+        // in the future, any CRLite result will be discarded.
+      }
      if (earliestCertificateTimestamp <= filterTimestampTime &&
          crliteRevocationState == nsICertStorage::STATE_ENFORCE) {
        if (mCRLiteTelemetryInfo) {
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@ -166,6 +166,7 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain {
      CertVerifier::SHA1Mode sha1Mode,
      NetscapeStepUpPolicy netscapeStepUpPolicy,
      DistrustedCAPolicy distrustedCAPolicy, CRLiteMode crliteMode,
+      uint64_t crliteCTMergeDelaySeconds,
      const OriginAttributes& originAttributes,
      const Vector<mozilla::pkix::Input>& thirdPartyRootInputs,
      const Vector<mozilla::pkix::Input>& thirdPartyIntermediateInputs,
@ -288,6 +289,7 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain {
  NetscapeStepUpPolicy mNetscapeStepUpPolicy;
  DistrustedCAPolicy mDistrustedCAPolicy;
  CRLiteMode mCRLiteMode;
+  uint64_t mCRLiteCTMergeDelaySeconds;
  bool mSawDistrustedCAByPolicyError;
  const OriginAttributes& mOriginAttributes;
  const Vector<mozilla::pkix::Input>& mThirdPartyRootInputs;  // non-owning
--- a/security/manager/ssl/SharedCertVerifier.h
+++ b/security/manager/ssl/SharedCertVerifier.h
@ -29,12 +29,13 @@ class SharedCertVerifier : public mozilla::psm::CertVerifier {
                     NetscapeStepUpPolicy netscapeStepUpPolicy,
                     CertificateTransparencyMode ctMode,
                     DistrustedCAPolicy distrustedCAPolicy,
-                     CRLiteMode crliteMode,
+                     CRLiteMode crliteMode, uint64_t crliteCTMergeDelaySeconds,
                     const Vector<EnterpriseCert>& thirdPartyCerts)
      : mozilla::psm::CertVerifier(
            odc, osc, ocspSoftTimeout, ocspHardTimeout, certShortLifetimeInDays,
            pinningMode, sha1Mode, nameMatchingMode, netscapeStepUpPolicy,
-            ctMode, distrustedCAPolicy, crliteMode, thirdPartyCerts) {}
+            ctMode, distrustedCAPolicy, crliteMode, crliteCTMergeDelaySeconds,
+            thirdPartyCerts) {}
 };

 }  // namespace psm
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@ -1499,6 +1499,16 @@ void nsNSSComponent::setValidationOptions(
      break;
  }

+  uint32_t defaultCRLiteCTMergeDelaySeconds =
+      60 * 60 * 28;  // 28 hours in seconds
+  uint64_t maxCRLiteCTMergeDelaySeconds = 60 * 60 * 24 * 365;
+  uint64_t crliteCTMergeDelaySeconds =
+      Preferences::GetUint("security.pki.crlite_ct_merge_delay_seconds",
+                           defaultCRLiteCTMergeDelaySeconds);
+  if (crliteCTMergeDelaySeconds > maxCRLiteCTMergeDelaySeconds) {
+    crliteCTMergeDelaySeconds = maxCRLiteCTMergeDelaySeconds;
+  }
+
  CertVerifier::OcspDownloadConfig odc;
  CertVerifier::OcspStrictConfig osc;
  uint32_t certShortLifetimeInDays;
@ -1512,7 +1522,8 @@ void nsNSSComponent::setValidationOptions(
      odc, osc, softTimeout, hardTimeout, certShortLifetimeInDays,
      PublicSSLState()->PinningMode(), sha1Mode,
      PublicSSLState()->NameMatchingMode(), netscapeStepUpPolicy, ctMode,
-      distrustedCAPolicy, crliteMode, mEnterpriseCerts);
+      distrustedCAPolicy, crliteMode, crliteCTMergeDelaySeconds,
+      mEnterpriseCerts);
 }

 void nsNSSComponent::UpdateCertVerifierWithEnterpriseRoots() {
@ -1532,7 +1543,7 @@ void nsNSSComponent::UpdateCertVerifierWithEnterpriseRoots() {
      oldCertVerifier->mSHA1Mode, oldCertVerifier->mNameMatchingMode,
      oldCertVerifier->mNetscapeStepUpPolicy, oldCertVerifier->mCTMode,
      oldCertVerifier->mDistrustedCAPolicy, oldCertVerifier->mCRLiteMode,
-      mEnterpriseCerts);
+      oldCertVerifier->mCRLiteCTMergeDelaySeconds, mEnterpriseCerts);
 }

 // Enable the TLS versions given in the prefs, defaulting to TLS 1.0 (min) and
@ -2409,7 +2420,9 @@ nsNSSComponent::Observe(nsISupports* aSubject, const char* aTopic,
               prefName.EqualsLiteral(
                   "security.OCSP.timeoutMilliseconds.hard") ||
               prefName.EqualsLiteral("security.pki.distrust_ca_policy") ||
-               prefName.EqualsLiteral("security.pki.crlite_mode")) {
+               prefName.EqualsLiteral("security.pki.crlite_mode") ||
+               prefName.EqualsLiteral(
+                   "security.pki.crlite_ct_merge_delay_seconds")) {
      MutexAutoLock lock(mMutex);
      setValidationOptions(false, lock);
 #ifdef DEBUG
--- a/security/manager/ssl/tests/unit/test_crlite_filters.js
+++ b/security/manager/ssl/tests/unit/test_crlite_filters.js
@ -423,11 +423,11 @@ add_task(
    });

    let result = await syncAndDownload([
-      { timestamp: "2019-11-19T00:00:00Z", type: "full", id: "0000" },
+      { timestamp: "2019-01-19T00:00:00Z", type: "full", id: "0000" },
    ]);
    equal(
      result,
-      "finished;2019-11-19T00:00:00Z-full",
+      "finished;2019-01-19T00:00:00Z-full",
      "CRLite filter download should have run"
    );

@ -475,9 +475,9 @@ add_task(
    );

    result = await syncAndDownload([
-      { timestamp: "2019-11-20T00:00:00Z", type: "full", id: "0000" },
+      { timestamp: "2019-01-20T00:00:00Z", type: "full", id: "0000" },
      {
-        timestamp: "2019-11-20T06:00:00Z",
+        timestamp: "2019-01-20T06:00:00Z",
        type: "diff",
        id: "0001",
        parent: "0000",
@ -487,7 +487,7 @@ add_task(
    equal(status, "finished", "CRLite filter download should have run");
    deepEqual(
      filters,
-      ["2019-11-20T00:00:00Z-full", "2019-11-20T06:00:00Z-diff"],
+      ["2019-01-20T00:00:00Z-full", "2019-01-20T06:00:00Z-diff"],
      "Should have downloaded the expected CRLite filters"
    );

@ -555,6 +555,28 @@ add_task(
    Services.prefs.clearUserPref("network.dns.localDomains");
    Services.prefs.clearUserPref("security.OCSP.require");
    Services.prefs.clearUserPref("security.OCSP.enabled");
+
+    // If the earliest certificate timestamp is within the merge delay of the
+    // logs for the filter we have, it won't be looked up, and thus won't be
+    // revoked.
+    // The earliest timestamp in this certificate is in May 2018, whereas the
+    // filter timestamp is in Janurary 2019, so setting the merge delay to this
+    // large value simluates the situation being tested.
+    Services.prefs.setIntPref(
+      "security.pki.crlite_ct_merge_delay_seconds",
+      60 * 60 * 24 * 360
+    );
+    await checkCertErrorGenericAtTime(
+      certdb,
+      revokedCert,
+      PRErrorCodeSuccess,
+      certificateUsageSSLServer,
+      new Date("2019-11-20T00:00:00Z").getTime() / 1000,
+      false,
+      "schunk-group.com",
+      Ci.nsIX509CertDB.FLAG_LOCAL_ONLY
+    );
+    Services.prefs.clearUserPref("security.pki.crlite_ct_merge_delay_seconds");
  }
 );

--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@ -47,9 +47,9 @@ support-files =
 [test_blocklist_onecrl.js]
 # Skip signature tests for Thunderbird (Bug 1341983).
 skip-if = appname == "thunderbird"
-tags = remote-settings blocklist
+tags = remote-settings blocklist psm
 [test_blocklist_pinning.js]
-tags = remote-settings blocklist
+tags = remote-settings blocklist psm
 [test_broken_fips.js]
 # FIPS has never been a thing on Android, so the workaround doesn't
 # exist on that platform.
@ -100,7 +100,7 @@ skip-if = toolkit == 'android' && processor == 'x86_64'
 [test_constructX509FromBase64.js]
 [test_content_signing.js]
 [test_crlite_filters.js]
-tags = remote-settings
+tags = remote-settings psm
 [test_ct.js]
 # Requires hard-coded debug-only data
 skip-if = !debug