bug 1548040 - batch cert_storage certificate adding/removal r=jcj,myk
authorDana Keeler <dkeeler@mozilla.com>
Tue, 14 May 2019 20:51:10 +0000
changeset 532797 73ead3a81fdf357101987a2796c7c1c1b24dc2bd
parent 532796 44144136273adb24b1aad01ba4db492346bb7559
child 532798 23e99af8279d9e9dff4e8e2f748c162d92ccde74
push id11272
push userapavel@mozilla.com
push dateThu, 16 May 2019 15:28:22 +0000
treeherdermozilla-beta@2265bfc5920d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj, myk
bugs1548040
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1548040 - batch cert_storage certificate adding/removal r=jcj,myk Differential Revision: https://phabricator.services.mozilla.com/D30271
security/manager/ssl/RemoteSecuritySettings.jsm
security/manager/ssl/cert_storage/src/lib.rs
security/manager/ssl/nsICertStorage.idl
security/manager/ssl/tests/unit/test_cert_storage_broken_db.js
security/manager/ssl/tests/unit/test_cert_storage_direct.js
--- a/security/manager/ssl/RemoteSecuritySettings.jsm
+++ b/security/manager/ssl/RemoteSecuritySettings.jsm
@@ -78,16 +78,25 @@ function stringToBytes(s) {
 // Converts an array of bytes to a JS string using fromCharCode on each byte.
 function bytesToString(bytes) {
   if (bytes.length > 65535) {
     throw new Error("input too long for bytesToString");
   }
   return String.fromCharCode.apply(null, bytes);
 }
 
+class CertInfo {
+  constructor(cert, subject) {
+    this.cert = cert;
+    this.subject = subject;
+    this.trust = Ci.nsICertStorage.TRUST_INHERIT;
+  }
+}
+CertInfo.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsICertInfo]);
+
 this.RemoteSecuritySettings = class RemoteSecuritySettings {
     constructor() {
         this.client = RemoteSettings(Services.prefs.getCharPref(INTERMEDIATES_COLLECTION_PREF), {
           bucketNamePref: INTERMEDIATES_BUCKET_PREF,
           lastCheckTimePref: INTERMEDIATES_CHECKED_SECONDS_PREF,
           signerName: Services.prefs.getCharPref(INTERMEDIATES_SIGNER_PREF),
           localFields: ["cert_import_complete"],
         });
@@ -143,32 +152,52 @@ this.RemoteSecuritySettings = class Remo
         }
         const current = await this.client.get();
         const waiting = current.filter(record => !record.cert_import_complete);
 
         log.debug(`There are ${waiting.length} intermediates awaiting download.`);
 
         TelemetryStopwatch.start(INTERMEDIATES_UPDATE_MS_TELEMETRY);
 
-        Promise.all(waiting.slice(0, maxDownloadsPerRun)
-          .map(record => this.maybeDownloadAttachment(record, col, certStorage))
-        ).then(async () => {
-          const finalCurrent = await this.client.get();
-          const finalWaiting = finalCurrent.filter(record => !record.cert_import_complete);
-          const countPreloaded = finalCurrent.length - finalWaiting.length;
+        let toDownload = waiting.slice(0, maxDownloadsPerRun);
+        let recordsCertsAndSubjects = await Promise.all(
+          toDownload.map(record => this.maybeDownloadAttachment(record)));
+        let certInfos = [];
+        let recordsToUpdate = [];
+        for (let {record, cert, subject} of recordsCertsAndSubjects) {
+          if (cert && subject) {
+            certInfos.push(new CertInfo(cert, subject));
+            recordsToUpdate.push(record);
+          }
+        }
+        let result = await new Promise((resolve) => {
+          certStorage.addCerts(certInfos, resolve);
+        }).catch((err) => err);
+        if (result != Cr.NS_OK) {
+          Cu.reportError(`certStorage.addCerts failed: ${result}`);
+          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+            .add("failedToUpdateDB");
+          return;
+        }
+        await Promise.all(recordsToUpdate.map((record) => {
+          record.cert_import_complete = true;
+          return col.update(record);
+        }));
+        const finalCurrent = await this.client.get();
+        const finalWaiting = finalCurrent.filter(record => !record.cert_import_complete);
+        const countPreloaded = finalCurrent.length - finalWaiting.length;
 
-          TelemetryStopwatch.finish(INTERMEDIATES_UPDATE_MS_TELEMETRY);
-          Services.telemetry.scalarSet(INTERMEDIATES_PRELOADED_TELEMETRY,
-                                       countPreloaded);
-          Services.telemetry.scalarSet(INTERMEDIATES_PENDING_TELEMETRY,
-                                       finalWaiting.length);
+        TelemetryStopwatch.finish(INTERMEDIATES_UPDATE_MS_TELEMETRY);
+        Services.telemetry.scalarSet(INTERMEDIATES_PRELOADED_TELEMETRY,
+                                     countPreloaded);
+        Services.telemetry.scalarSet(INTERMEDIATES_PENDING_TELEMETRY,
+                                     finalWaiting.length);
 
-          Services.obs.notifyObservers(null, "remote-security-settings:intermediates-updated",
-                                       "success");
-        });
+        Services.obs.notifyObservers(null, "remote-security-settings:intermediates-updated",
+                                     "success");
     }
 
     async onObservePollEnd(subject, topic, data) {
         log.debug(`onObservePollEnd ${subject} ${topic}`);
 
         try {
           await this.updatePreloadedIntermediates();
         } catch (err) {
@@ -222,126 +251,108 @@ this.RemoteSecuritySettings = class Remo
         return resp.arrayBuffer();
       })
       .then(buffer => new Uint8Array(buffer));
     }
 
     /**
      * Attempts to download the attachment, assuming it's not been processed
      * already. Does not retry, and always resolves (e.g., does not reject upon
-     * failure.) Errors are reported via Cu.reportError; If you need to know
-     * success/failure, check record.cert_import_complete.
+     * failure.) Errors are reported via Cu.reportError.
      * @param  {AttachmentRecord} record defines which data to obtain
-     * @param  {KintoCollection}  col The kinto collection to update
-     * @param  {nsICertStorage}   certStorage The certificate storage to update
-     * @return {Promise}          a Promise representing the transaction
+     * @return {Promise}          a Promise that will resolve to an object with the properties
+     *                            record, cert, and subject. record is the original record.
+     *                            cert is the base64-encoded bytes of the downloaded certificate (if
+     *                            downloading was successful), and null otherwise.
+     *                            subject is the base64-encoded bytes of the subject distinguished
+     *                            name of the same.
      */
-    async maybeDownloadAttachment(record, col, certStorage) {
+    async maybeDownloadAttachment(record) {
       const {attachment: {hash, size}} = record;
-
-      return this._downloadAttachmentBytes(record)
-      .then(async function(attachmentData) {
-        if (!attachmentData || attachmentData.length == 0) {
-          // Bug 1519273 - Log telemetry for these rejections
-          log.debug(`Empty attachment. Hash=${hash}`);
-
-          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("emptyAttachment");
-
-          return;
-        }
-
-        // check the length
-        if (attachmentData.length !== size) {
-          log.debug(`Unexpected attachment length. Hash=${hash} Lengths ${attachmentData.length} != ${size}`);
-
-          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("unexpectedLength");
-
-          return;
-        }
-
-        // check the hash
-        let dataAsString = gTextDecoder.decode(attachmentData);
-        let calculatedHash = getHash(dataAsString);
-        if (calculatedHash !== hash) {
-          log.warn(`Invalid hash. CalculatedHash=${calculatedHash}, Hash=${hash}, data=${dataAsString}`);
-
-          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("unexpectedHash");
-
-          return;
-        }
+      let result = { record, cert: null, subject: null };
 
-        let certBase64;
-        let subjectBase64;
-        try {
-          // split off the header and footer
-          certBase64 = dataAsString.split("-----")[2].replace(/\s/g, "");
-          // get an array of bytes so we can use X509.jsm
-          let certBytes = stringToBytes(atob(certBase64));
-          let cert = new X509.Certificate();
-          cert.parse(certBytes);
-          // get the DER-encoded subject and get a base64-encoded string from it
-          // TODO(bug 1542028): add getters for _der and _bytes
-          subjectBase64 = btoa(bytesToString(cert.tbsCertificate.subject._der._bytes));
-        } catch (err) {
-          Cu.reportError(`Failed to decode cert: ${err}`);
-
-          // Re-purpose the "failedToUpdateNSS" telemetry tag as "failed to
-          // decode preloaded intermediate certificate"
-          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("failedToUpdateNSS");
-
-          return;
-        }
-        log.debug(`Adding cert. Hash=${hash}. Size=${size}`);
-        // We can assume that certs obtained from remote-settings are part of
-        // the root program. If they aren't, they won't be used for path-
-        // building anyway, so just add it to the DB with trust set to
-        // "inherit".
-        let result = await new Promise((resolve) => {
-          certStorage.addCertBySubject(certBase64, subjectBase64,
-                                       Ci.nsICertStorage.TRUST_INHERIT,
-                                       resolve);
-        });
-        if (result != Cr.NS_OK) {
-          Cu.reportError(`Failed to add to cert storage: ${result}`);
-          Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("failedToUpdateDB");
-          return;
-        }
-
-        record.cert_import_complete = true;
-        await col.update(record);
-      })
-      .catch(() => {
-        // Don't abort the outer Promise.all because of an error. Errors were
-        // sent using Cu.reportError()
+      let attachmentData;
+      try {
+        attachmentData = await this._downloadAttachmentBytes(record);
+      } catch (err) {
+        Cu.reportError(`Failed to download attachment: ${err}`);
         Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
           .add("failedToDownloadMisc");
-      });
+        return result;
+      }
+
+      if (!attachmentData || attachmentData.length == 0) {
+        // Bug 1519273 - Log telemetry for these rejections
+        log.debug(`Empty attachment. Hash=${hash}`);
+
+        Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("emptyAttachment");
+
+        return result;
+      }
+
+      // check the length
+      if (attachmentData.length !== size) {
+        log.debug(`Unexpected attachment length. Hash=${hash} Lengths ${attachmentData.length} != ${size}`);
+
+        Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("unexpectedLength");
+
+        return result;
+      }
+
+      // check the hash
+      let dataAsString = gTextDecoder.decode(attachmentData);
+      let calculatedHash = getHash(dataAsString);
+      if (calculatedHash !== hash) {
+        log.warn(`Invalid hash. CalculatedHash=${calculatedHash}, Hash=${hash}, data=${dataAsString}`);
+
+        Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("unexpectedHash");
+
+        return result;
+      }
+      log.debug(`downloaded cert with hash=${hash}, size=${size}`);
+
+      let certBase64;
+      let subjectBase64;
+      try {
+        // split off the header and footer
+        certBase64 = dataAsString.split("-----")[2].replace(/\s/g, "");
+        // get an array of bytes so we can use X509.jsm
+        let certBytes = stringToBytes(atob(certBase64));
+        let cert = new X509.Certificate();
+        cert.parse(certBytes);
+        // get the DER-encoded subject and get a base64-encoded string from it
+        // TODO(bug 1542028): add getters for _der and _bytes
+        subjectBase64 = btoa(bytesToString(cert.tbsCertificate.subject._der._bytes));
+      } catch (err) {
+        Cu.reportError(`Failed to decode cert: ${err}`);
+
+        // Re-purpose the "failedToUpdateNSS" telemetry tag as "failed to
+        // decode preloaded intermediate certificate"
+        Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("failedToUpdateNSS");
+
+        return result;
+      }
+      result.cert = certBase64;
+      result.subject = subjectBase64;
+      return result;
     }
 
     async maybeSync(expectedTimestamp, options) {
       return this.client.maybeSync(expectedTimestamp, options);
     }
 
     async removeCerts(recordsToRemove) {
       let certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage);
-      let failures = 0;
-      for (let record of recordsToRemove) {
-        let result = await new Promise((resolve) => {
-          certStorage.removeCertByHash(record.pubKeyHash, resolve);
-        });
-        if (result != Cr.NS_OK) {
-          Cu.reportError(`Failed to remove intermediate certificate Hash=${record.pubKeyHash}: ${result}`);
-          failures++;
-        }
-      }
-
-      if (failures > 0) {
-        Cu.reportError(`Failed to remove ${failures} intermediate certificates`);
+      let hashes = recordsToRemove.map(record => record.pubKeyHash);
+      let result = await new Promise((resolve) => {
+          certStorage.removeCertsByHashes(hashes, resolve);
+      }).catch((err) => err);
+      if (result != Cr.NS_OK) {
+        Cu.reportError(`Failed to remove some intermediate certificates`);
         Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
           .add("failedToRemove");
       }
     }
 };
--- a/security/manager/ssl/cert_storage/src/lib.rs
+++ b/security/manager/ssl/cert_storage/src/lib.rs
@@ -21,17 +21,17 @@ extern crate storage_variant;
 extern crate style;
 
 use byteorder::{NetworkEndian, ReadBytesExt, WriteBytesExt};
 use crossbeam_utils::atomic::AtomicCell;
 use lmdb::EnvironmentFlags;
 use moz_task::{create_thread, is_main_thread, Task, TaskRunnable};
 use nserror::{
     nsresult, NS_ERROR_FAILURE, NS_ERROR_NOT_SAME_THREAD, NS_ERROR_NO_AGGREGATION,
-    NS_ERROR_UNEXPECTED, NS_OK,
+    NS_ERROR_NULL_POINTER, NS_ERROR_UNEXPECTED, NS_OK,
 };
 use nsstring::{nsACString, nsAString, nsCStr, nsCString, nsString};
 use rkv::error::StoreError;
 use rkv::{Rkv, SingleStore, StoreOptions, Value};
 use sha2::{Digest, Sha256};
 use std::collections::HashMap;
 use std::ffi::{CStr, CString};
 use std::fmt::Display;
@@ -42,19 +42,19 @@ use std::os::raw::c_char;
 use std::path::{Path, PathBuf};
 use std::slice;
 use std::str;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::{Duration, SystemTime};
 use storage_variant::VariantType;
 use thin_vec::ThinVec;
 use xpcom::interfaces::{
-    nsICertStorage, nsICertStorageCallback, nsIFile, nsIIssuerAndSerialRevocationState,
-    nsIObserver, nsIPrefBranch, nsIRevocationState, nsISubjectAndPubKeyRevocationState,
-    nsISupports, nsIThread,
+    nsICertInfo, nsICertStorage, nsICertStorageCallback, nsIFile,
+    nsIIssuerAndSerialRevocationState, nsIObserver, nsIPrefBranch, nsIRevocationState,
+    nsISubjectAndPubKeyRevocationState, nsISupports, nsIThread,
 };
 use xpcom::{nsIID, GetterAddrefs, RefPtr, ThreadBoundRefPtr, XpCom};
 
 const PREFIX_REV_IS: &str = "is";
 const PREFIX_REV_SPK: &str = "spk";
 const PREFIX_SUBJECT: &str = "subject";
 const PREFIX_CERT: &str = "cert";
 const PREFIX_DATA_TYPE: &str = "datatype";
@@ -390,31 +390,29 @@ impl SecurityState {
             "security.onecrl.maximum_staleness_in_seconds",
         )
     }
 
     pub fn pref_seen(&mut self, name: &str, value: u32) {
         self.int_prefs.insert(name.to_owned(), value);
     }
 
-    // To store a certificate by subject, we first create a Cert out of the given cert, subject, and
-    // trust. We hash the certificate with sha-256 to obtain a unique* key for that certificate, and
-    // we store the Cert in the database. We also look up or create a CertHashList for the given
-    // subject and add the new certificate's hash if it isn't present in the list. If it wasn't
-    // present, we write out the updated CertHashList.
+    // To store certificates, we create a Cert out of each given cert, subject, and trust tuple. We
+    // hash each certificate with sha-256 to obtain a unique* key for that certificate, and we store
+    // the Cert in the database. We also look up or create a CertHashList for the given subject and
+    // add the new certificate's hash if it isn't present in the list. If it wasn't present, we
+    // write out the updated CertHashList.
     // *By the pigeon-hole principle, there exist collisions for sha-256, so this key is not
     // actually unique. We rely on the assumption that sha-256 is a cryptographically strong hash.
     // If an adversary can find two different certificates with the same sha-256 hash, they can
     // probably forge a sha-256-based signature, so assuming the keys we create here are unique is
     // not a security issue.
-    pub fn add_cert_by_subject(
+    pub fn add_certs(
         &mut self,
-        cert_der: &[u8],
-        subject: &[u8],
-        trust: i16,
+        certs: &[(Vec<u8>, Vec<u8>, i16)],
     ) -> Result<(), SecurityStateError> {
         self.reopen_store_read_write()?;
         {
             let env_and_store = match self.env_and_store.as_mut() {
                 Some(env_and_store) => env_and_store,
                 None => return Err(SecurityStateError::from("env and store not initialized?")),
             };
             let mut writer = env_and_store.env.write()?;
@@ -423,89 +421,94 @@ impl SecurityState {
                 &mut writer,
                 &make_key!(
                     PREFIX_DATA_TYPE,
                     &[nsICertStorage::DATA_TYPE_CERTIFICATE as u8]
                 ),
                 &Value::Bool(true),
             )?;
 
-            let mut digest = Sha256::default();
-            digest.input(cert_der);
-            let cert_hash = digest.result();
-            let cert_key = make_key!(PREFIX_CERT, &cert_hash);
-            let cert = Cert::new(cert_der, subject, trust)?;
-            env_and_store
-                .store
-                .put(&mut writer, &cert_key, &Value::Blob(&cert.to_bytes()?))?;
-            let subject_key = make_key!(PREFIX_SUBJECT, subject);
-            // This reader will only be able to "see" data outside the current transaction. This is
-            // fine, though, because what we're reading has not yet been touched by this
-            // transaction.
-            let reader = env_and_store.env.read()?;
-            let empty_vec = Vec::new();
-            let old_cert_hash_list = match env_and_store.store.get(&reader, &subject_key)? {
-                Some(Value::Blob(hashes)) => hashes,
-                Some(_) => &empty_vec,
-                None => &empty_vec,
-            };
-            let new_cert_hash_list = CertHashList::add(old_cert_hash_list, &cert_hash)?;
-            if new_cert_hash_list.len() != old_cert_hash_list.len() {
-                env_and_store.store.put(
-                    &mut writer,
-                    &subject_key,
-                    &Value::Blob(&new_cert_hash_list),
-                )?;
+            for (cert_der, subject, trust) in certs {
+                let mut digest = Sha256::default();
+                digest.input(cert_der);
+                let cert_hash = digest.result();
+                let cert_key = make_key!(PREFIX_CERT, &cert_hash);
+                let cert = Cert::new(cert_der, subject, *trust)?;
+                env_and_store
+                    .store
+                    .put(&mut writer, &cert_key, &Value::Blob(&cert.to_bytes()?))?;
+                let subject_key = make_key!(PREFIX_SUBJECT, subject);
+                let empty_vec = Vec::new();
+                let old_cert_hash_list = match env_and_store.store.get(&writer, &subject_key)? {
+                    Some(Value::Blob(hashes)) => hashes.to_owned(),
+                    Some(_) => empty_vec,
+                    None => empty_vec,
+                };
+                let new_cert_hash_list = CertHashList::add(&old_cert_hash_list, &cert_hash)?;
+                if new_cert_hash_list.len() != old_cert_hash_list.len() {
+                    env_and_store.store.put(
+                        &mut writer,
+                        &subject_key,
+                        &Value::Blob(&new_cert_hash_list),
+                    )?;
+                }
             }
 
             writer.commit()?;
         }
         self.reopen_store_read_only()?;
         Ok(())
     }
 
-    // Given a certificate's sha-256 hash, we can look up its Cert entry in the database. We use
-    // this to find its subject so we can look up the CertHashList it should appear in. If that list
-    // contains the given hash, we remove it and update the CertHashList. Finally we delete the Cert
-    // entry.
-    pub fn remove_cert_by_hash(&mut self, hash: &[u8]) -> Result<(), SecurityStateError> {
+    // Given a list of certificate sha-256 hashes, we can look up each Cert entry in the database.
+    // We use this to find the corresponding subject so we can look up the CertHashList it should
+    // appear in. If that list contains the given hash, we remove it and update the CertHashList.
+    // Finally we delete the Cert entry.
+    pub fn remove_certs_by_hashes(&mut self, hashes: &[Vec<u8>]) -> Result<(), SecurityStateError> {
         self.reopen_store_read_write()?;
         {
             let env_and_store = match self.env_and_store.as_mut() {
                 Some(env_and_store) => env_and_store,
                 None => return Err(SecurityStateError::from("env and store not initialized?")),
             };
             let mut writer = env_and_store.env.write()?;
+            let reader = env_and_store.env.read()?;
 
-            let reader = env_and_store.env.read()?;
-            let cert_key = make_key!(PREFIX_CERT, hash);
-            if let Some(Value::Blob(cert_bytes)) = env_and_store.store.get(&reader, &cert_key)? {
-                if let Ok(cert) = Cert::from_bytes(cert_bytes) {
-                    let subject_key = make_key!(PREFIX_SUBJECT, &cert.subject);
-                    let empty_vec = Vec::new();
-                    let old_cert_hash_list = match env_and_store.store.get(&reader, &subject_key)? {
-                        Some(Value::Blob(hashes)) => hashes,
-                        Some(_) => &empty_vec,
-                        None => &empty_vec,
-                    };
-                    let new_cert_hash_list = CertHashList::remove(old_cert_hash_list, hash)?;
-                    if new_cert_hash_list.len() != old_cert_hash_list.len() {
-                        env_and_store.store.put(
-                            &mut writer,
-                            &subject_key,
-                            &Value::Blob(&new_cert_hash_list),
-                        )?;
+            for hash in hashes {
+                let cert_key = make_key!(PREFIX_CERT, hash);
+                if let Some(Value::Blob(cert_bytes)) =
+                    env_and_store.store.get(&reader, &cert_key)?
+                {
+                    if let Ok(cert) = Cert::from_bytes(cert_bytes) {
+                        let subject_key = make_key!(PREFIX_SUBJECT, &cert.subject);
+                        let empty_vec = Vec::new();
+                        // We have to use the writer here to make sure we have an up-to-date view of
+                        // the cert hash list.
+                        let old_cert_hash_list =
+                            match env_and_store.store.get(&writer, &subject_key)? {
+                                Some(Value::Blob(hashes)) => hashes.to_owned(),
+                                Some(_) => empty_vec,
+                                None => empty_vec,
+                            };
+                        let new_cert_hash_list = CertHashList::remove(&old_cert_hash_list, hash)?;
+                        if new_cert_hash_list.len() != old_cert_hash_list.len() {
+                            env_and_store.store.put(
+                                &mut writer,
+                                &subject_key,
+                                &Value::Blob(&new_cert_hash_list),
+                            )?;
+                        }
                     }
                 }
+                match env_and_store.store.delete(&mut writer, &cert_key) {
+                    Ok(()) => {}
+                    Err(StoreError::LmdbError(lmdb::Error::NotFound)) => {}
+                    Err(e) => return Err(SecurityStateError::from(e)),
+                };
             }
-            match env_and_store.store.delete(&mut writer, &cert_key) {
-                Ok(()) => {}
-                Err(StoreError::LmdbError(lmdb::Error::NotFound)) => {}
-                Err(e) => return Err(SecurityStateError::from(e)),
-            };
             writer.commit()?;
         }
         self.reopen_store_read_only()?;
         Ok(())
     }
 
     // Given a certificate's subject, we look up the corresponding CertHashList. In theory, each
     // hash in that list corresponds to a certificate with the given subject, so we look up each of
@@ -1024,17 +1027,17 @@ impl CertStorage {
         &self,
         data_type: u8,
         callback: *const nsICertStorageCallback,
     ) -> nserror::nsresult {
         if !is_main_thread() {
             return NS_ERROR_NOT_SAME_THREAD;
         }
         if callback.is_null() {
-            return NS_ERROR_FAILURE;
+            return NS_ERROR_NULL_POINTER;
         }
         let task = Box::new(SecurityStateTask::new(
             &*callback,
             &self.security_state,
             move |ss| ss.get_has_prior_data(data_type),
         ));
         let thread = try_ns!(self.thread.lock());
         let runnable = try_ns!(TaskRunnable::new("HasPriorData", task));
@@ -1046,17 +1049,17 @@ impl CertStorage {
         &self,
         revocations: *const ThinVec<RefPtr<nsIRevocationState>>,
         callback: *const nsICertStorageCallback,
     ) -> nserror::nsresult {
         if !is_main_thread() {
             return NS_ERROR_NOT_SAME_THREAD;
         }
         if revocations.is_null() || callback.is_null() {
-            return NS_ERROR_FAILURE;
+            return NS_ERROR_NULL_POINTER;
         }
 
         let revocations = &*revocations;
         let mut entries = Vec::with_capacity(revocations.len());
 
         // By continuing when an nsIRevocationState attribute value is invalid,
         // we prevent errors relating to individual blocklist entries from
         // causing sync to fail. We will accumulate telemetry on these failures
@@ -1110,17 +1113,17 @@ impl CertStorage {
         serial: *const ThinVec<u8>,
         subject: *const ThinVec<u8>,
         pub_key: *const ThinVec<u8>,
         state: *mut i16,
     ) -> nserror::nsresult {
         // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we
         // can't do so until bug 1406854 and bug 1534600 are fixed.
         if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key.is_null() {
-            return NS_ERROR_FAILURE;
+            return NS_ERROR_NULL_POINTER;
         }
         *state = nsICertStorage::STATE_UNSET as i16;
         let ss = get_security_state!(self);
         match ss.get_revocation_state(&*issuer, &*serial, &*subject, &*pub_key) {
             Ok(st) => {
                 *state = st;
                 NS_OK
             }
@@ -1135,74 +1138,88 @@ impl CertStorage {
         *fresh = match ss.is_blocklist_fresh() {
             Ok(is_fresh) => is_fresh,
             Err(_) => false,
         };
 
         NS_OK
     }
 
-    unsafe fn AddCertBySubject(
+    unsafe fn AddCerts(
         &self,
-        cert: *const nsACString,
-        subject: *const nsACString,
-        trust: i16,
+        certs: *const ThinVec<RefPtr<nsICertInfo>>,
         callback: *const nsICertStorageCallback,
     ) -> nserror::nsresult {
         if !is_main_thread() {
             return NS_ERROR_NOT_SAME_THREAD;
         }
-        if cert.is_null() || subject.is_null() || callback.is_null() {
-            return NS_ERROR_FAILURE;
+        if certs.is_null() || callback.is_null() {
+            return NS_ERROR_NULL_POINTER;
         }
-        let cert_decoded = try_ns!(base64::decode(&*cert));
-        let subject_decoded = try_ns!(base64::decode(&*subject));
+        let certs = &*certs;
+        let mut cert_entries = Vec::with_capacity(certs.len());
+        for cert in certs {
+            let mut der = nsCString::new();
+            try_ns!((*cert).GetCert(&mut *der).to_result(), or continue);
+            let der = try_ns!(base64::decode(&der), or continue);
+            let mut subject = nsCString::new();
+            try_ns!((*cert).GetSubject(&mut *subject).to_result(), or continue);
+            let subject = try_ns!(base64::decode(&subject), or continue);
+            let mut trust: i16 = 0;
+            try_ns!((*cert).GetTrust(&mut trust).to_result(), or continue);
+            cert_entries.push((der, subject, trust));
+        }
         let task = Box::new(SecurityStateTask::new(
             &*callback,
             &self.security_state,
-            move |ss| ss.add_cert_by_subject(&cert_decoded, &subject_decoded, trust),
+            move |ss| ss.add_certs(&cert_entries),
         ));
         let thread = try_ns!(self.thread.lock());
-        let runnable = try_ns!(TaskRunnable::new("AddCertBySubject", task));
+        let runnable = try_ns!(TaskRunnable::new("AddCerts", task));
         try_ns!(runnable.dispatch(&*thread));
         NS_OK
     }
 
-    unsafe fn RemoveCertByHash(
+    unsafe fn RemoveCertsByHashes(
         &self,
-        hash: *const nsACString,
+        hashes: *const ThinVec<nsCString>,
         callback: *const nsICertStorageCallback,
     ) -> nserror::nsresult {
         if !is_main_thread() {
             return NS_ERROR_NOT_SAME_THREAD;
         }
-        if hash.is_null() || callback.is_null() {
-            return NS_ERROR_FAILURE;
+        if hashes.is_null() || callback.is_null() {
+            return NS_ERROR_NULL_POINTER;
         }
-        let hash_decoded = try_ns!(base64::decode(&*hash));
+        let hashes = &*hashes;
+        let mut hash_entries = Vec::with_capacity(hashes.len());
+        for hash in hashes {
+            let hash_decoded = try_ns!(base64::decode(&*hash), or continue);
+            hash_entries.push(hash_decoded);
+        }
         let task = Box::new(SecurityStateTask::new(
             &*callback,
             &self.security_state,
-            move |ss| ss.remove_cert_by_hash(&hash_decoded),
+            move |ss| ss.remove_certs_by_hashes(&hash_entries),
         ));
         let thread = try_ns!(self.thread.lock());
-        let runnable = try_ns!(TaskRunnable::new("RemoveCertByHash", task));
+        let runnable = try_ns!(TaskRunnable::new("RemoveCertsByHashes", task));
         try_ns!(runnable.dispatch(&*thread));
         NS_OK
     }
 
     unsafe fn FindCertsBySubject(
         &self,
         subject: *const ThinVec<u8>,
         certs: *mut ThinVec<ThinVec<u8>>,
     ) -> nserror::nsresult {
         // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we
         // can't do so until bug 1406854 and bug 1534600 are fixed.
         if subject.is_null() || certs.is_null() {
-            return NS_ERROR_FAILURE;
+            return NS_ERROR_NULL_POINTER;
         }
         let ss = get_security_state!(self);
         match ss.find_certs_by_subject(&*subject, &mut *certs) {
             Ok(()) => NS_OK,
             Err(_) => NS_ERROR_FAILURE,
         }
     }
 
--- a/security/manager/ssl/nsICertStorage.idl
+++ b/security/manager/ssl/nsICertStorage.idl
@@ -50,16 +50,32 @@ interface nsIIssuerAndSerialRevocationSt
  * and public key hash are base64-encoded.
  */
 [scriptable, uuid(e78b51b4-6fa4-41e2-92ce-e9404f541e96)]
 interface nsISubjectAndPubKeyRevocationState : nsIRevocationState {
     readonly attribute ACString subject;
     readonly attribute ACString pubKey;
 };
 
+/**
+ * An interface representing a certificate to add to storage. Consists of the
+ * base64-encoded DER bytes of the certificate (cert), the base64-encoded DER
+ * bytes of the subject distinguished name of the certificate (subject), and the
+ * trust of the certificate (one of the nsICertStorage.TRUST_* constants).
+ * (Note that this implementation does not validate that the given subject DN
+ * actually matches the subject DN of the certificate, nor that the given cert
+ * is a valid DER X.509 certificate.)
+ */
+[scriptable, uuid(27b66f5e-0faf-403b-95b4-bc11691ac50d)]
+interface nsICertInfo : nsISupports {
+  readonly attribute ACString cert;
+  readonly attribute ACString subject;
+  readonly attribute short trust;
+};
+
 [scriptable, uuid(327100a7-3401-45ef-b160-bf880f1016fd)]
 interface nsICertStorage : nsISupports {
   const octet DATA_TYPE_REVOCATION = 1;
   const octet DATA_TYPE_CERTIFICATE = 2;
 
   /**
    * Asynchronously check if the backing storage has stored data of the given
    * type in the past. This is useful if the backing storage may have had to
@@ -113,44 +129,37 @@ interface nsICertStorage : nsISupports {
    * TRUST_INHERIT indicates a certificate inherits trust from another
    * certificate.
    * TRUST_ANCHOR indicates the certificate is a root of trust.
    */
   const short TRUST_INHERIT = 0;
   const short TRUST_ANCHOR = 1;
 
   /**
-   * Asynchronously add a certificate to the backing storage.
-   * cert is the bytes of the certificate as base64-encoded DER.
-   * subject is the subject distinguished name of the certificate as
-   * base64-encoded DER (although we don't actually validate that the given
-   * certificate has the indicated subject).
-   * trust is one of the TRUST_* constants in this interface.
+   * Asynchronously add a list of certificates to the backing storage.
+   * See the documentation for nsICertInfo.
    * The given callback is called with the result of the operation when it
    * completes.
    * Must only be called from the main thread.
    */
   [must_use]
-  void addCertBySubject(in ACString cert,
-                        in ACString subject,
-                        in short trust,
-                        in nsICertStorageCallback callback);
+  void addCerts(in Array<nsICertInfo> certs, in nsICertStorageCallback callback);
 
   /**
-   * Asynchronously remove the certificate with the given sha-256 hash from the
-   * backing storage.
-   * hash is the base64-encoded bytes of the sha-256 hash of the certificate's
-   * bytes (DER-encoded).
+   * Asynchronously remove the certificates with the given sha-256 hashes from
+   * the backing storage.
+   * hashes is an array of base64-encoded bytes of the sha-256 hashes of each
+   * certificate's bytes (DER-encoded).
    * The given callback is called with the result of the operation when it
    * completes.
    * Must only be called from the main thread.
    */
   [must_use]
-  void removeCertByHash(in ACString hash,
-                        in nsICertStorageCallback callback);
+  void removeCertsByHashes(in Array<ACString> hashes,
+                           in nsICertStorageCallback callback);
 
   /**
    * Find all certificates in the backing storage with the given subject
    * distinguished name.
    * subject is the DER-encoded bytes of the subject distinguished name.
    * Returns an array of arrays of bytes, where each inner array corresponds to
    * the DER-encoded bytes of a certificate that has the given subject (although
    * as these certificates were presumably added via addCertBySubject, this
--- a/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js
+++ b/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js
@@ -49,16 +49,15 @@ add_task({
     certStorage.setRevocations([], resolve);
   });
   Assert.equal(result, Cr.NS_OK, "setRevocations should succeed");
 
   check_has_prior_revocation_data(certStorage, true);
   check_has_prior_cert_data(certStorage, false);
 
   result = await new Promise((resolve) => {
-    certStorage.addCertBySubject(btoa("some cert"), btoa("some subject"),
-                                 Ci.nsICertStorage.TRUST_INHERIT, resolve);
+    certStorage.addCerts([], resolve);
   });
-  Assert.equal(result, Cr.NS_OK, "addCertBySubject should succeed");
+  Assert.equal(result, Cr.NS_OK, "addCerts should succeed");
 
   check_has_prior_revocation_data(certStorage, true);
   check_has_prior_cert_data(certStorage, true);
 });
--- a/security/manager/ssl/tests/unit/test_cert_storage_direct.js
+++ b/security/manager/ssl/tests/unit/test_cert_storage_direct.js
@@ -8,29 +8,28 @@
 // integration test).
 
 do_get_profile();
 
 if (AppConstants.MOZ_NEW_CERT_STORAGE) {
   this.certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage);
 }
 
-async function addCertBySubject(cert, subject) {
+async function addCerts(certInfos) {
   let result = await new Promise((resolve) => {
-    certStorage.addCertBySubject(btoa(cert), btoa(subject), Ci.nsICertStorage.TRUST_INHERIT,
-                                 resolve);
+    certStorage.addCerts(certInfos, resolve);
   });
-  Assert.equal(result, Cr.NS_OK, "addCertBySubject should succeed");
+  Assert.equal(result, Cr.NS_OK, "addCerts should succeed");
 }
 
-async function removeCertByHash(hashBase64) {
+async function removeCertsByHashes(hashesBase64) {
   let result = await new Promise((resolve) => {
-    certStorage.removeCertByHash(hashBase64, resolve);
+    certStorage.removeCertsByHashes(hashesBase64, resolve);
   });
-  Assert.equal(result, Cr.NS_OK, "removeCertByHash should succeed");
+  Assert.equal(result, Cr.NS_OK, "removeCertsByHashes should succeed");
 }
 
 function stringToArray(s) {
   let a = [];
   for (let i = 0; i < s.length; i++) {
     a.push(s.charCodeAt(i));
   }
   return a;
@@ -43,29 +42,41 @@ function arrayToString(a) {
   }
   return s;
 }
 
 function getLongString(uniquePart, length) {
   return String(uniquePart).padStart(length, "0");
 }
 
+class CertInfo {
+  constructor(cert, subject) {
+    this.cert = btoa(cert);
+    this.subject = btoa(subject);
+    this.trust = Ci.nsICertStorage.TRUST_INHERIT;
+  }
+}
+if (AppConstants.MOZ_NEW_CERT_STORAGE) {
+  CertInfo.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsICertInfo]);
+}
+
 add_task({
     skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
   }, async function test_common_subject() {
-  await addCertBySubject("some certificate bytes 1", "some common subject");
-  await addCertBySubject("some certificate bytes 2", "some common subject");
-  await addCertBySubject("some certificate bytes 3", "some common subject");
+  let someCert1 = new CertInfo("some certificate bytes 1", "some common subject");
+  let someCert2 = new CertInfo("some certificate bytes 2", "some common subject");
+  let someCert3 = new CertInfo("some certificate bytes 3", "some common subject");
+  await addCerts([someCert1, someCert2, someCert3]);
   let storedCerts = certStorage.findCertsBySubject(stringToArray("some common subject"));
   let storedCertsAsStrings = storedCerts.map(arrayToString);
   let expectedCerts = ["some certificate bytes 1", "some certificate bytes 2",
                        "some certificate bytes 3"];
   Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(), "should find expected certs");
 
-  await addCertBySubject("some other certificate bytes", "some other subject");
+  await addCerts([new CertInfo("some other certificate bytes", "some other subject")]);
   storedCerts = certStorage.findCertsBySubject(stringToArray("some common subject"));
   storedCertsAsStrings = storedCerts.map(arrayToString);
   Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(),
                    "should still find expected certs");
 
   let storedOtherCerts = certStorage.findCertsBySubject(stringToArray("some other subject"));
   let storedOtherCertsAsStrings = storedOtherCerts.map(arrayToString);
   let expectedOtherCerts = ["some other certificate bytes"];
@@ -73,64 +84,90 @@ add_task({
 });
 
 add_task({
     skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
   }, async function test_many_entries() {
   const NUM_CERTS = 500;
   const CERT_LENGTH = 3000;
   const SUBJECT_LENGTH = 40;
+  let certs = [];
   for (let i = 0; i < NUM_CERTS; i++) {
-    await addCertBySubject(getLongString(i, CERT_LENGTH), getLongString(i, SUBJECT_LENGTH));
+    certs.push(new CertInfo(getLongString(i, CERT_LENGTH), getLongString(i, SUBJECT_LENGTH)));
   }
+  await addCerts(certs);
   for (let i = 0; i < NUM_CERTS; i++) {
     let subject = stringToArray(getLongString(i, SUBJECT_LENGTH));
     let storedCerts = certStorage.findCertsBySubject(subject);
     Assert.equal(storedCerts.length, 1, "should have 1 certificate (lots of data test)");
     let storedCertAsString = arrayToString(storedCerts[0]);
     Assert.equal(storedCertAsString, getLongString(i, CERT_LENGTH),
                  "certificate should be as expected (lots of data test)");
   }
 });
 
 add_task({
     skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
   }, async function test_removal() {
   // As long as cert_storage is given valid base64, attempting to delete some nonexistent
   // certificate will "succeed" (it'll do nothing).
-  await removeCertByHash(btoa("thishashisthewrongsize"));
+  await removeCertsByHashes([btoa("thishashisthewrongsize")]);
 
-  await addCertBySubject("removal certificate bytes 1", "common subject to remove");
-  await addCertBySubject("removal certificate bytes 2", "common subject to remove");
-  await addCertBySubject("removal certificate bytes 3", "common subject to remove");
+  let removalCert1 = new CertInfo("removal certificate bytes 1", "common subject to remove");
+  let removalCert2 = new CertInfo("removal certificate bytes 2", "common subject to remove");
+  let removalCert3 = new CertInfo("removal certificate bytes 3", "common subject to remove");
+  await addCerts([removalCert1, removalCert2, removalCert3]);
 
   let storedCerts = certStorage.findCertsBySubject(stringToArray("common subject to remove"));
   let storedCertsAsStrings = storedCerts.map(arrayToString);
   let expectedCerts = ["removal certificate bytes 1", "removal certificate bytes 2",
                        "removal certificate bytes 3"];
   Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(),
                    "should find expected certs before removing them");
 
   // echo -n "removal certificate bytes 2" | sha256sum | xxd -r -p | base64
-  await removeCertByHash("2nUPHwl5TVr1mAD1FU9FivLTlTb0BAdnVUhsYgBccN4=");
+  await removeCertsByHashes(["2nUPHwl5TVr1mAD1FU9FivLTlTb0BAdnVUhsYgBccN4="]);
   storedCerts = certStorage.findCertsBySubject(stringToArray("common subject to remove"));
   storedCertsAsStrings = storedCerts.map(arrayToString);
   expectedCerts = ["removal certificate bytes 1", "removal certificate bytes 3"];
   Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(),
                    "should only have first and third certificates now");
 
   // echo -n "removal certificate bytes 1" | sha256sum | xxd -r -p | base64
-  await removeCertByHash("8zoRqHYrklr7Zx6UWpzrPuL+ol8KL1Ml6XHBQmXiaTY=");
+  await removeCertsByHashes(["8zoRqHYrklr7Zx6UWpzrPuL+ol8KL1Ml6XHBQmXiaTY="]);
   storedCerts = certStorage.findCertsBySubject(stringToArray("common subject to remove"));
   storedCertsAsStrings = storedCerts.map(arrayToString);
   expectedCerts = ["removal certificate bytes 3"];
   Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(),
                    "should only have third certificate now");
 
   // echo -n "removal certificate bytes 3" | sha256sum | xxd -r -p | base64
-  await removeCertByHash("vZn7GwDSabB/AVo0T+N26nUsfSXIIx4NgQtSi7/0p/w=");
+  await removeCertsByHashes(["vZn7GwDSabB/AVo0T+N26nUsfSXIIx4NgQtSi7/0p/w="]);
   storedCerts = certStorage.findCertsBySubject(stringToArray("common subject to remove"));
   Assert.equal(storedCerts.length, 0, "shouldn't have any certificates now");
 
   // echo -n "removal certificate bytes 3" | sha256sum | xxd -r -p | base64
   // Again, removing a nonexistent certificate should "succeed".
-  await removeCertByHash("vZn7GwDSabB/AVo0T+N26nUsfSXIIx4NgQtSi7/0p/w=");
+  await removeCertsByHashes(["vZn7GwDSabB/AVo0T+N26nUsfSXIIx4NgQtSi7/0p/w="]);
 });
+
+add_task({
+    skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
+}, async function test_batched_removal() {
+  let removalCert1 = new CertInfo("batch removal certificate bytes 1", "batch subject to remove");
+  let removalCert2 = new CertInfo("batch removal certificate bytes 2", "batch subject to remove");
+  let removalCert3 = new CertInfo("batch removal certificate bytes 3", "batch subject to remove");
+  await addCerts([removalCert1, removalCert2, removalCert3]);
+  let storedCerts = certStorage.findCertsBySubject(stringToArray("batch subject to remove"));
+  let storedCertsAsStrings = storedCerts.map(arrayToString);
+  let expectedCerts = ["batch removal certificate bytes 1", "batch removal certificate bytes 2",
+                       "batch removal certificate bytes 3"];
+  Assert.deepEqual(storedCertsAsStrings.sort(), expectedCerts.sort(),
+                   "should find expected certs before removing them");
+  // echo -n "batch removal certificate bytes 1" | sha256sum | xxd -r -p | base64
+  // echo -n "batch removal certificate bytes 2" | sha256sum | xxd -r -p | base64
+  // echo -n "batch removal certificate bytes 3" | sha256sum | xxd -r -p | base64
+  await removeCertsByHashes(["EOEEUTuanHZX9NFVCoMKVT22puIJC6g+ZuNPpJgvaa8=",
+                             "Xz6h/Kvn35cCLJEZXkjPqk1GG36b56sreLyAXpO+0zg=",
+                             "Jr7XdiTT8ZONUL+ogNNMW2oxKxanvYOLQPKBPgH/has="]);
+  storedCerts = certStorage.findCertsBySubject(stringToArray("batch subject to remove"));
+  Assert.equal(storedCerts.length, 0, "shouldn't have any certificates now");
+});