Bug 1926414 - add a pref to make CRLite coverage checks more strict. r=keeler

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

Cargo.lock

@@ -743,6 +743,7 @@ dependencies = [
  "rkv",
  "rust_cascade",
  "sha2",
+ "static_prefs",
  "storage_variant",
  "tempfile",
  "thin-vec",

modules/libpref/init/StaticPrefList.yaml

@@ -15761,6 +15761,14 @@
 #endif
   mirror: never
 
+# The number of SCTs that must be "covered" by a CRLite filter before
+# we will enforce a result from that filter.
+- name: security.pki.crlite_timestamps_for_coverage
+  type: RelaxedAtomicUint32
+  value: 1
+  mirror: always
+  rust: true
+
 - name: security.pki.use_modern_crypto_with_pkcs12
   type: RelaxedAtomicBool
   value: true

security/manager/ssl/cert_storage/Cargo.toml

@@ -19,6 +19,7 @@ nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rkv = { version = "0.19", default-features = false }
 rust_cascade = "1.4.0"
 sha2 = "0.10.2"
+static_prefs = { path = "../../../../modules/libpref/init/static_prefs" }
 storage_variant = { path = "../../../../storage/variant" }
 tempfile = "3"
 thin-vec = { version = "0.2.1", features = ["gecko-ffi"] }

security/manager/ssl/cert_storage/src/lib.rs

@@ -30,8 +30,8 @@ use wr_malloc_size_of as malloc_size_of;
 
 use base64::prelude::*;
 use byteorder::{LittleEndian, NetworkEndian, ReadBytesExt, WriteBytesExt};
-use clubcard::ApproximateSizeOf;
-use clubcard_crlite::{CRLiteClubcard, CRLiteKey, CRLiteStatus};
+use clubcard::{ApproximateSizeOf, Queryable};
+use clubcard_crlite::{CRLiteClubcard, CRLiteKey, CRLiteQuery, CRLiteStatus};
 use crossbeam_utils::atomic::AtomicCell;
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use moz_task::{create_background_task_queue, is_main_thread, Task, TaskRunnable};
@@ -225,14 +225,17 @@ impl CascadeWithMetadata {
     }
 
     fn filter_covers_some_timestamp(&self, timestamps: &[CRLiteTimestamp]) -> bool {
+        let mut covered_timestamp_count = 0;
         for entry in timestamps {
             if let Some(&(low, high)) = self.coverage.get(entry.log_id.as_ref()) {
                 if low <= entry.timestamp && entry.timestamp <= high {
-                    return true;
+                    covered_timestamp_count += 1;
                 }
             }
         }
-        false
+
+        covered_timestamp_count
+            >= static_prefs::pref!("security.pki.crlite_timestamps_for_coverage")
     }
 
     fn issuer_is_enrolled(&self, subject: &[u8], pub_key: &[u8]) -> bool {
@@ -317,7 +320,7 @@ impl Filter {
                 }
             }
             Filter::Clubcard(clubcard) => {
-                let timestamps = timestamps
+                let timestamp_iter = timestamps
                     .iter()
                     .map(|timestamp| {
                         (&*timestamp.log_id) /* ThinVec -> &[u8; 32] */
@@ -326,7 +329,20 @@ impl Filter {
                             .map(|log_id| (log_id, timestamp.timestamp))
                     })
                     .flatten();
-                match clubcard.contains(&clubcard_crlite_key, timestamps) {
+                let mut covered_timestamp_count = 0;
+                for timestamp in timestamp_iter.clone() {
+                    if CRLiteQuery::new(&clubcard_crlite_key, Some(timestamp))
+                        .in_universe(clubcard.universe())
+                    {
+                        covered_timestamp_count += 1;
+                    }
+                }
+                if covered_timestamp_count
+                    < static_prefs::pref!("security.pki.crlite_timestamps_for_coverage")
+                {
+                    return nsICertStorage::STATE_NOT_COVERED;
+                }
+                match clubcard.contains(&clubcard_crlite_key, timestamp_iter) {
                     CRLiteStatus::Good => nsICertStorage::STATE_UNSET,
                     CRLiteStatus::NotCovered => nsICertStorage::STATE_NOT_COVERED,
                     CRLiteStatus::NotEnrolled => nsICertStorage::STATE_NOT_ENROLLED,

security/manager/ssl/tests/unit/test_crlite_filters.js

@@ -794,18 +794,22 @@ async function test_crlite_filters_and_check_revocation(filter_type) {
     0
   );
 
+  // NB: this will cause an OCSP request to be sent to localhost:80, but
+  // since an OCSP responder shouldn't be running on that port, this should
+  // fail safely.
+  Services.prefs.setCharPref("network.dns.localDomains", [
+    "ocsp.digicert.com",
+    "ocsp.godaddy.com",
+  ]);
+  Services.prefs.setBoolPref("security.OCSP.require", true);
+  Services.prefs.setIntPref("security.OCSP.enabled", 1);
+
   // This certificate has no embedded SCTs, so it is not guaranteed to be in
   // CT, so CRLite can't be guaranteed to give the correct answer, so it is
   // not consulted, and the implementation falls back to OCSP. Since the real
   // OCSP responder can't be reached, this results in a
   // SEC_ERROR_OCSP_SERVER_ERROR.
   let noSCTCert = constructCertFromFile("test_crlite_filters/no-sct.pem");
-  // NB: this will cause an OCSP request to be sent to localhost:80, but
-  // since an OCSP responder shouldn't be running on that port, this should
-  // fail safely.
-  Services.prefs.setCharPref("network.dns.localDomains", "ocsp.digicert.com");
-  Services.prefs.setBoolPref("security.OCSP.require", true);
-  Services.prefs.setIntPref("security.OCSP.enabled", 1);
   await checkCertErrorGenericAtTime(
     certdb,
     noSCTCert,
@@ -816,6 +820,22 @@ async function test_crlite_filters_and_check_revocation(filter_type) {
     "mail233.messagelabs.com",
     0
   );
+
+  // If we increase the number of timestamps required for coverage then
+  // even the valid certificate will fallback to OCSP.
+  Services.prefs.setIntPref("security.pki.crlite_timestamps_for_coverage", 100);
+  await checkCertErrorGenericAtTime(
+    certdb,
+    validCert,
+    SEC_ERROR_OCSP_SERVER_ERROR,
+    certificateUsageSSLServer,
+    new Date("2020-10-20T00:00:00Z").getTime() / 1000,
+    false,
+    "vpn.worldofspeed.org",
+    0
+  );
+  Services.prefs.clearUserPref("security.pki.crlite_timestamps_for_coverage");
+
   Services.prefs.clearUserPref("network.dns.localDomains");
   Services.prefs.clearUserPref("security.OCSP.require");
   Services.prefs.clearUserPref("security.OCSP.enabled");

toolkit/components/nimbus/FeatureManifest.yaml

@@ -3830,6 +3830,13 @@ crlite:
         pref: security.pki.crlite_mode
       description: >-
         How CRLite results will be interpreted.
+    timestamps_for_coverage:
+      type: int
+      setPref:
+        branch: default
+        pref: security.pki.crlite_timestamps_for_coverage
+      description: >-
+        How many covered timestamps do we need to see before we enforce a CRLite result.
 
 chatbot:
   description: AI chatbot feature configuration