Bug 1539549 - update cert blocklist using single transaction r=keeler
authorMyk Melez <myk@mykzilla.org>
Fri, 26 Apr 2019 20:10:59 +0000
changeset 530403 0b745e9ecd500577d9da5049ea21980a5f0659d6
parent 530402 792aae9f893aeebd2f8e20cb4e10948278b6de75
child 530404 f41ea9d40a05327739aa20813430b45c2ef63785
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1539549
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 1539549 - update cert blocklist using single transaction r=keeler Differential Revision: https://phabricator.services.mozilla.com/D28540
Cargo.lock
security/manager/ssl/cert_storage/Cargo.toml
security/manager/ssl/cert_storage/src/lib.rs
security/manager/ssl/nsICertStorage.idl
security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
services/common/blocklist-clients.js
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -428,16 +428,17 @@ source = "registry+https://github.com/ru
 
 [[package]]
 name = "cert_storage"
 version = "0.0.1"
 dependencies = [
  "base64 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "crossbeam-utils 0.6.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "lmdb-rkv 0.11.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "moz_task 0.1.0",
  "nserror 0.1.0",
  "nsstring 0.1.0",
  "rkv 0.9.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "sha2 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "style 0.0.1",
  "thin-vec 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "time 0.1.40 (registry+https://github.com/rust-lang/crates.io-index)",
--- a/security/manager/ssl/cert_storage/Cargo.toml
+++ b/security/manager/ssl/cert_storage/Cargo.toml
@@ -2,16 +2,17 @@
 name = "cert_storage"
 version = "0.0.1"
 authors = ["Dana Keeler <dkeeler@mozilla.com>", "Mark Goodwin <mgoodwin@mozilla.com"]
 
 [dependencies]
 base64 = "0.10"
 crossbeam-utils = "0.6.3"
 lmdb-rkv = "0.11"
+log = "0.4"
 moz_task = { path = "../../../../xpcom/rust/moz_task" }
 nserror = { path = "../../../../xpcom/rust/nserror" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rkv = "^0.9"
 sha2 = "^0.8"
 style = { path = "../../../../servo/components/style" }
 thin-vec = { version = "0.1.0", features = ["gecko-ffi"] }
 time = "0.1"
--- a/security/manager/ssl/cert_storage/src/lib.rs
+++ b/security/manager/ssl/cert_storage/src/lib.rs
@@ -1,15 +1,17 @@
 /* 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/. */
 
 extern crate base64;
 extern crate crossbeam_utils;
 extern crate lmdb;
+#[macro_use]
+extern crate log;
 extern crate moz_task;
 extern crate nserror;
 extern crate nsstring;
 extern crate rkv;
 extern crate sha2;
 extern crate thin_vec;
 extern crate time;
 #[macro_use]
@@ -34,18 +36,19 @@ use std::io::{BufRead, BufReader};
 use std::os::raw::c_char;
 use std::path::PathBuf;
 use std::slice;
 use std::str;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::{Duration, SystemTime};
 use thin_vec::ThinVec;
 use xpcom::interfaces::{
-    nsICertStorage, nsICertStorageCallback, nsIFile, nsIObserver, nsIPrefBranch, nsISupports,
-    nsIThread,
+    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_CRLITE: &str = "crlite";
 const PREFIX_WL: &str = "wl";
 
@@ -258,32 +261,38 @@ impl SecurityState {
             }
             Ok(None) => Ok(None),
             _ => Err(SecurityStateError::from(
                 "There was a problem getting the value",
             )),
         }
     }
 
-    pub fn set_revocation_by_issuer_and_serial(
+    pub fn set_revocations(
         &mut self,
-        issuer: &[u8],
-        serial: &[u8],
-        state: i16,
+        entries: &[(Vec<u8>, i16)],
     ) -> Result<(), SecurityStateError> {
-        self.write_entry(&make_key(PREFIX_REV_IS, issuer, serial), state)
-    }
+        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()?;
 
-    pub fn set_revocation_by_subject_and_pub_key(
-        &mut self,
-        subject: &[u8],
-        pub_key_hash: &[u8],
-        state: i16,
-    ) -> Result<(), SecurityStateError> {
-        self.write_entry(&make_key(PREFIX_REV_SPK, subject, pub_key_hash), state)
+            for entry in entries {
+                env_and_store
+                    .store
+                    .put(&mut writer, &entry.0, &Value::I64(entry.1 as i64))?;
+            }
+
+            writer.commit()?;
+        }
+        self.reopen_store_read_only()?;
+        Ok(())
     }
 
     pub fn set_enrollment(
         &mut self,
         issuer: &[u8],
         serial: &[u8],
         state: i16,
     ) -> Result<(), SecurityStateError> {
@@ -598,27 +607,76 @@ macro_rules! security_state_task {
                     NS_OK => Ok(()),
                     e => Err(e),
                 }
             }
         }
     };
 }
 
-security_state_task!(
-    SetRevocationByIssuerAndSerialTask,
-    set_revocation_by_issuer_and_serial
-);
-security_state_task!(
-    SetRevocationBySubjectAndPubKeyTask,
-    set_revocation_by_subject_and_pub_key
-);
 security_state_task!(SetEnrollmentTask, set_enrollment);
 security_state_task!(SetWhitelistTask, set_whitelist);
 
+struct SetRevocationsTask {
+    callback: AtomicCell<Option<ThreadBoundRefPtr<nsICertStorageCallback>>>,
+    security_state: Arc<RwLock<SecurityState>>,
+    entries: Vec<(Vec<u8>, i16)>,
+    result: AtomicCell<Option<nserror::nsresult>>,
+}
+impl SetRevocationsTask {
+    fn new(
+        callback: &nsICertStorageCallback,
+        security_state: &Arc<RwLock<SecurityState>>,
+        entries: Vec<(Vec<u8>, i16)>,
+    ) -> SetRevocationsTask {
+        SetRevocationsTask {
+            callback: AtomicCell::new(Some(ThreadBoundRefPtr::new(RefPtr::new(callback)))),
+            security_state: Arc::clone(security_state),
+            entries,
+            result: AtomicCell::new(None),
+        }
+    }
+}
+impl Task for SetRevocationsTask {
+    fn run(&self) {
+        let mut ss = match self.security_state.write() {
+            Ok(ss) => ss,
+            Err(_) => {
+                self.result.store(Some(NS_ERROR_FAILURE));
+                return;
+            }
+        };
+        // this is a no-op if the DB is already open
+        match ss.open_db() {
+            Ok(()) => {}
+            Err(_) => {
+                self.result.store(Some(NS_ERROR_FAILURE));
+                return;
+            }
+        };
+        match ss.set_revocations(&self.entries) {
+            Ok(_) => self.result.store(Some(NS_OK)),
+            Err(_) => self.result.store(Some(NS_ERROR_FAILURE)),
+        };
+    }
+
+    fn done(&self) -> Result<(), nsresult> {
+        let threadbound = self.callback.swap(None).ok_or(NS_ERROR_FAILURE)?;
+        let callback = threadbound.get_ref().ok_or(NS_ERROR_FAILURE)?;
+        let nsrv = match self.result.swap(None) {
+            Some(result) => unsafe { callback.Done(result) },
+            None => unsafe { callback.Done(NS_ERROR_FAILURE) },
+        };
+        match nsrv {
+            NS_OK => Ok(()),
+            e => Err(e),
+        }
+    }
+}
+
 #[no_mangle]
 pub extern "C" fn cert_storage_constructor(
     outer: *const nsISupports,
     iid: *const xpcom::nsIID,
     result: *mut *mut xpcom::reexports::libc::c_void,
 ) -> nserror::nsresult {
     if !outer.is_null() {
         return NS_ERROR_NO_AGGREGATION;
@@ -635,16 +693,25 @@ pub extern "C" fn cert_storage_construct
 
 macro_rules! try_ns {
     ($e:expr) => {
         match $e {
             Ok(value) => value,
             Err(_) => return NS_ERROR_FAILURE,
         }
     };
+    ($e:expr, or continue) => {
+        match $e {
+            Ok(value) => value,
+            Err(err) => {
+                error!("{}", err);
+                continue;
+            }
+        }
+    };
 }
 
 #[derive(xpcom)]
 #[xpimplements(nsICertStorage, nsIObserver)]
 #[refcnt = "atomic"]
 struct InitCertStorage {
     security_state: Arc<RwLock<SecurityState>>,
     thread: Mutex<RefPtr<nsIThread>>,
@@ -687,68 +754,74 @@ impl CertStorage {
                 Err(_) => return Err(SecurityStateError::from("could not read pref")),
             };
             assert!(rv.succeeded());
         }
 
         Ok(())
     }
 
-    unsafe fn SetRevocationByIssuerAndSerial(
+    unsafe fn SetRevocations(
         &self,
-        issuer: *const nsACString,
-        serial: *const nsACString,
-        state: i16,
+        revocations: *const ThinVec<RefPtr<nsIRevocationState>>,
         callback: *const nsICertStorageCallback,
     ) -> nserror::nsresult {
         if !is_main_thread() {
             return NS_ERROR_NOT_SAME_THREAD;
         }
-        if issuer.is_null() || serial.is_null() || callback.is_null() {
+        if revocations.is_null() || callback.is_null() {
             return NS_ERROR_FAILURE;
         }
-        let issuer_decoded = try_ns!(base64::decode(&*issuer));
-        let serial_decoded = try_ns!(base64::decode(&*serial));
-        let task = Box::new(SetRevocationByIssuerAndSerialTask::new(
+
+        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
+        // in bug 1254099.
+
+        for revocation in revocations {
+            let mut state: i16 = 0;
+            try_ns!(revocation.GetState(&mut state).to_result(), or continue);
+
+            if let Some(revocation) =
+                (*revocation).query_interface::<nsIIssuerAndSerialRevocationState>()
+            {
+                let mut issuer = nsCString::new();
+                try_ns!(revocation.GetIssuer(&mut *issuer).to_result(), or continue);
+                let issuer = try_ns!(base64::decode(&issuer), or continue);
+
+                let mut serial = nsCString::new();
+                try_ns!(revocation.GetSerial(&mut *serial).to_result(), or continue);
+                let serial = try_ns!(base64::decode(&serial), or continue);
+
+                entries.push((make_key(PREFIX_REV_IS, &issuer, &serial), state));
+            } else if let Some(revocation) =
+                (*revocation).query_interface::<nsISubjectAndPubKeyRevocationState>()
+            {
+                let mut subject = nsCString::new();
+                try_ns!(revocation.GetSubject(&mut *subject).to_result(), or continue);
+                let subject = try_ns!(base64::decode(&subject), or continue);
+
+                let mut pub_key_hash = nsCString::new();
+                try_ns!(revocation.GetPubKey(&mut *pub_key_hash).to_result(), or continue);
+                let pub_key_hash = try_ns!(base64::decode(&pub_key_hash), or continue);
+
+                entries.push((make_key(PREFIX_REV_SPK, &subject, &pub_key_hash), state));
+            }
+        }
+
+        let task = Box::new(SetRevocationsTask::new(
             &*callback,
             &self.security_state,
-            issuer_decoded,
-            serial_decoded,
-            state,
+            entries,
         ));
         let thread = try_ns!(self.thread.lock());
-        let runnable = try_ns!(TaskRunnable::new("SetRevocationByIssuerAndSerial", task));
-        try_ns!(runnable.dispatch(&*thread));
-        NS_OK
-    }
-
-    unsafe fn SetRevocationBySubjectAndPubKey(
-        &self,
-        subject: *const nsACString,
-        pub_key_hash: *const nsACString,
-        state: i16,
-        callback: *const nsICertStorageCallback,
-    ) -> nserror::nsresult {
-        if !is_main_thread() {
-            return NS_ERROR_NOT_SAME_THREAD;
-        }
-        if subject.is_null() || pub_key_hash.is_null() || callback.is_null() {
-            return NS_ERROR_FAILURE;
-        }
-        let subject_decoded = try_ns!(base64::decode(&*subject));
-        let pub_key_hash_decoded = try_ns!(base64::decode(&*pub_key_hash));
-        let task = Box::new(SetRevocationBySubjectAndPubKeyTask::new(
-            &*callback,
-            &self.security_state,
-            subject_decoded,
-            pub_key_hash_decoded,
-            state,
-        ));
-        let thread = try_ns!(self.thread.lock());
-        let runnable = try_ns!(TaskRunnable::new("SetRevocationBySubjectAndPubKey", task));
+        let runnable = try_ns!(TaskRunnable::new("SetRevocations", task));
         try_ns!(runnable.dispatch(&*thread));
         NS_OK
     }
 
     unsafe fn SetEnrollment(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
--- a/security/manager/ssl/nsICertStorage.idl
+++ b/security/manager/ssl/nsICertStorage.idl
@@ -15,51 +15,64 @@
  * operation.
  */
 [scriptable, function, uuid(3f8fe26a-a436-4ad4-9c1c-a53c60973c31)]
 interface nsICertStorageCallback : nsISupports {
   [must_use]
   void done(in nsresult result);
 };
 
+/**
+ * A base interface for representing the revocation state of a certificate.
+ * Implementing this interface by itself is insufficient; your type must
+ * implement an inheriting interface that specifies the certificate by issuer
+ * and serial number or by subject and public key hash.
+ * Set state to nsICertStorage.STATE_UNSET to mark the certificate as not revoked.
+ * Set state to nsICertStorage.STATE_ENFORCE to mark the certificate as revoked.
+ */
+[scriptable, uuid(96db6fd7-6b64-4a5a-955d-310bd9ca4234)]
+interface nsIRevocationState : nsISupports {
+  readonly attribute short state;
+};
+
+/**
+ * An interface representing the revocation state of a certificate by issuer
+ * and serial number. Both issuer name and serial number are base64-encoded.
+ */
+[scriptable, uuid(23ce3546-f1b9-46f6-8de3-77704da5702f)]
+interface nsIIssuerAndSerialRevocationState : nsIRevocationState {
+    readonly attribute ACString issuer;
+    readonly attribute ACString serial;
+};
+
+/**
+ * An interface representing the revocation state of a certificate by subject
+ * and pub key hash (the hash algorithm should be SHA-256). Both subject name
+ * 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;
+};
+
 [scriptable, uuid(327100a7-3401-45ef-b160-bf880f1016fd)]
 interface nsICertStorage : nsISupports {
   const short STATE_UNSET = 0;
   const short STATE_ENFORCE = 1;
 
   /**
-   * Asynchronously set the revocation state of a certificate by issuer and
-   * serial number. Both issuer name and serial number are base64-encoded.
-   * Pass STATE_UNSET to mark the certificate as not revoked.
-   * Pass STATE_ENFORCE to mark the certificate as revoked.
+   * Asynchronously set the revocation states of a set of certificates.
    * 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 setRevocationByIssuerAndSerial(in ACString issuer,
-                                      in ACString serialNumber,
-                                      in short state,
-                                      in nsICertStorageCallback callback);
-
-  /**
-   * Asynchronously set the revocation state of a certificate by subject and
-   * public key hash (the hash algorithm should be SHA-256). Both subject name
-   * and public key hash are base64-encoded.
-   * Pass STATE_UNSET to mark the certificate as not revoked.
-   * Pass STATE_ENFORCE to mark the certificate as revoked.
-   * 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 setRevocationBySubjectAndPubKey(in ACString subject,
-                                       in ACString pubKeyHash,
-                                       in short state,
-                                       in nsICertStorageCallback callback);
+  void setRevocations(in Array<nsIRevocationState> revocations,
+                      in nsICertStorageCallback callback);
 
   /**
    * Asynchronously set the whitelist state of an intermediate certificate by
    * issuer and serial number. Both are base64-encoded.
    * state (short) is STATE_ENFORCE for whitelisted certs, STATE_UNSET otherwise.
    * The given callback is called with the result of the operation when it
    * completes.
    * Must only be called from the main thread.
--- a/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
+++ b/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
@@ -98,21 +98,22 @@ add_task(async function testUntrusted() 
 
 add_task(async function testRevoked() {
   // Note that there's currently no way to un-do this. This should only be a
   // problem if another test re-uses a certificate with this same key (perhaps
   // likely) and subject (less likely).
   let certBlocklist = Cc["@mozilla.org/security/certstorage;1"]
                         .getService(Ci.nsICertStorage);
   let result = await new Promise((resolve) =>
-    certBlocklist.setRevocationBySubjectAndPubKey(
-      "MBIxEDAOBgNVBAMMB3Jldm9rZWQ=", // CN=revoked
-      "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=", // hash of the shared key
-      Ci.nsICertStorage.STATE_ENFORCE, // yes, we want this to be revoked
-      resolve));
+    certBlocklist.setRevocations([{
+      QueryInterface: ChromeUtils.generateQI([Ci.nsISubjectAndPubKeyRevocationState]),
+      subject: "MBIxEDAOBgNVBAMMB3Jldm9rZWQ=", // CN=revoked
+      pubKey: "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=", // hash of the shared key
+      state: Ci.nsICertStorage.STATE_ENFORCE, // yes, we want this to be revoked
+    }], resolve));
   Assert.equal(result, Cr.NS_OK, "setting revocation state should succeed");
   let cert = await readCertificate("revoked.pem", ",,");
   let win = await displayCertificate(cert);
   // As of bug 1312827, OneCRL only applies to TLS web server certificates, so
   // this certificate will actually verify successfully for every end-entity
   // usage except TLS web server.
   checkUsages(win, [{id: "verify-email-recip"}, {id: "verify-email-signer"}, {id: "verify-ssl-client"}]);
   checkDetailsPane(win, ["ca", "revoked"]);
--- a/services/common/blocklist-clients.js
+++ b/services/common/blocklist-clients.js
@@ -31,64 +31,85 @@ const PREF_BLOCKLIST_PINNING_ENABLED    
 const PREF_BLOCKLIST_PINNING_BUCKET          = "services.blocklist.pinning.bucket";
 const PREF_BLOCKLIST_PINNING_COLLECTION      = "services.blocklist.pinning.collection";
 const PREF_BLOCKLIST_PINNING_CHECKED_SECONDS = "services.blocklist.pinning.checked";
 const PREF_BLOCKLIST_PINNING_SIGNER          = "services.blocklist.pinning.signer";
 const PREF_BLOCKLIST_GFX_COLLECTION          = "services.blocklist.gfx.collection";
 const PREF_BLOCKLIST_GFX_CHECKED_SECONDS     = "services.blocklist.gfx.checked";
 const PREF_BLOCKLIST_GFX_SIGNER              = "services.blocklist.gfx.signer";
 
-function setRevocationByIssuerAndSerial(certStorage, issuer, serial, state) {
-  return new Promise((resolve) =>
-    certStorage.setRevocationByIssuerAndSerial(issuer, serial, state, resolve)
-  );
+class RevocationState {
+  constructor(state) {
+    this.state = state;
+  }
 }
 
-function setRevocationBySubjectAndPubKey(certStorage, subject, pubKey, state) {
+class IssuerAndSerialRevocationState extends RevocationState {
+  constructor(issuer, serial, state) {
+    super(state);
+    this.issuer = issuer;
+    this.serial = serial;
+  }
+}
+IssuerAndSerialRevocationState.prototype.QueryInterface =
+  ChromeUtils.generateQI([Ci.nsIIssuerAndSerialRevocationState]);
+
+class SubjectAndPubKeyRevocationState extends RevocationState {
+  constructor(subject, pubKey, state) {
+    super(state);
+    this.subject = subject;
+    this.pubKey = pubKey;
+  }
+}
+SubjectAndPubKeyRevocationState.prototype.QueryInterface =
+  ChromeUtils.generateQI([Ci.nsISubjectAndPubKeyRevocationState]);
+
+function setRevocations(certStorage, revocations) {
   return new Promise((resolve) =>
-    certStorage.setRevocationBySubjectAndPubKey(subject, pubKey, state, resolve)
+    certStorage.setRevocations(revocations, resolve)
   );
 }
 
 /**
  * Revoke the appropriate certificates based on the records from the blocklist.
  *
  * @param {Object} data   Current records in the local db.
  */
 async function updateCertBlocklist({ data: { created, updated, deleted } }) {
   const certList = Cc["@mozilla.org/security/certstorage;1"]
                      .getService(Ci.nsICertStorage);
+  let items = [];
+
   for (let item of deleted) {
     if (item.issuerName && item.serialNumber) {
-      await setRevocationByIssuerAndSerial(certList, item.issuerName, item.serialNumber,
-                                           Ci.nsICertStorage.STATE_UNSET);
+      items.push(new IssuerAndSerialRevocationState(item.issuerName,
+        item.serialNumber, Ci.nsICertStorage.STATE_UNSET));
     } else if (item.subject && item.pubKeyHash) {
-      await setRevocationBySubjectAndPubKey(certList, item.subject, item.pubKeyHash,
-                                            Ci.nsICertStorage.STATE_UNSET);
+      items.push(new SubjectAndPubKeyRevocationState(item.subject,
+        item.pubKeyHash, Ci.nsICertStorage.STATE_UNSET));
     }
   }
 
   const toAdd = created.concat(updated.map(u => u.new));
 
   for (let item of toAdd) {
-    try {
-      if (item.issuerName && item.serialNumber) {
-        await setRevocationByIssuerAndSerial(certList, item.issuerName, item.serialNumber,
-                                             Ci.nsICertStorage.STATE_ENFORCE);
-      } else if (item.subject && item.pubKeyHash) {
-        await setRevocationBySubjectAndPubKey(certList, item.subject, item.pubKeyHash,
-                                             Ci.nsICertStorage.STATE_ENFORCE);
-      }
-    } catch (e) {
-      // prevent errors relating to individual blocklist entries from
-      // causing sync to fail. We will accumulate telemetry on these failures in
-      // bug 1254099.
-      Cu.reportError(e);
+    if (item.issuerName && item.serialNumber) {
+      items.push(new IssuerAndSerialRevocationState(item.issuerName,
+        item.serialNumber, Ci.nsICertStorage.STATE_ENFORCE));
+    } else if (item.subject && item.pubKeyHash) {
+      items.push(new SubjectAndPubKeyRevocationState(item.subject,
+        item.pubKeyHash, Ci.nsICertStorage.STATE_ENFORCE));
     }
   }
+
+  try {
+    await setRevocations(certList, items);
+  } catch (e) {
+    Cu.reportError(e);
+  }
 }
 
 /**
  * Modify the appropriate security pins based on records from the remote
  * collection.
  *
  * @param {Object} data   Current records in the local db.
  */