Bug 1799976 - onecrl: clean up the old versions of updated blocklisted entries r=jschanck

When a onecrl revocation gets updated, the entry corresponding to its previous
value needs to be unset.

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

security/manager/ssl/RemoteSecuritySettings.jsm

@@ -172,7 +172,8 @@ const updateCertBlocklist = async function({
     created = current;
   }
 
-  for (let item of deleted) {
+  let toDelete = deleted.concat(updated.map(u => u.old));
+  for (let item of toDelete) {
     if (item.issuerName && item.serialNumber) {
       items.push(
         new IssuerAndSerialRevocationState(

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

@@ -1556,8 +1556,8 @@ impl CertStorage {
         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 is fixed.
+        // TODO (bug 1541212): We really want to restrict this to non-main-threads only in non-test
+        // contexts, but we can't do so until bug 1406854 is fixed.
         if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key.is_null() {
             return NS_ERROR_NULL_POINTER;
         }

security/manager/ssl/nsICertStorage.idl

@@ -142,7 +142,8 @@ interface nsICertStorage : nsISupports {
    * serial - serial number, DER encoded
    * subject - subject name, DER encoded
    * pubkey - public key, DER encoded
-   * Must not be called from the main thread. See bug 1541212.
+   * In gecko, must not be called from the main thread. See bug 1541212.
+   * xpcshell tests may call this from the main thread.
    */
   [must_use]
   short getRevocationState(in Array<octet> issuer,

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

@@ -36,12 +36,12 @@ add_task(async function test_default_jexl_filter_is_used() {
 
 add_task(
   async function test_revocations_are_updated_on_sync_with_cert_storage() {
-    const certList = Cc["@mozilla.org/security/certstorage;1"].getService(
+    const certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(
       Ci.nsICertStorage
     );
     const has_revocations = () =>
       new Promise(resolve => {
-        certList.hasPriorData(
+        certStorage.hasPriorData(
           Ci.nsICertStorage.DATA_TYPE_REVOCATION,
           (rv, hasPriorData) => {
             if (rv == Cr.NS_OK) {
@@ -71,3 +71,112 @@ add_task(
     Assert.ok(await has_revocations());
   }
 );
+
+add_task(async function test_updated_entry() {
+  // Revoke a particular issuer/serial number.
+  await OneCRLBlocklistClient.emit("sync", {
+    data: {
+      current: [],
+      created: [
+        {
+          issuerName: "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=",
+          serialNumber: "a0X7/7DlTaedpgrIJg25iBPOkIM=",
+        },
+      ],
+      updated: [],
+      deleted: [],
+    },
+  });
+  const certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(
+    Ci.nsICertStorage
+  );
+  let issuerArray = [
+    0x30,
+    0x12,
+    0x31,
+    0x10,
+    0x30,
+    0xe,
+    0x6,
+    0x3,
+    0x55,
+    0x4,
+    0x3,
+    0xc,
+    0x7,
+    0x54,
+    0x65,
+    0x73,
+    0x74,
+    0x20,
+    0x43,
+    0x41,
+  ];
+  let serialArray = [
+    0x6b,
+    0x45,
+    0xfb,
+    0xff,
+    0xb0,
+    0xe5,
+    0x4d,
+    0xa7,
+    0x9d,
+    0xa6,
+    0xa,
+    0xc8,
+    0x26,
+    0xd,
+    0xb9,
+    0x88,
+    0x13,
+    0xce,
+    0x90,
+    0x83,
+  ];
+  let revocationState = certStorage.getRevocationState(
+    issuerArray,
+    serialArray,
+    [],
+    []
+  );
+  Assert.equal(revocationState, Ci.nsICertStorage.STATE_ENFORCE);
+
+  // Update the revocation to be a different serial number; the original
+  // (issuer, serial) pair should now not be revoked.
+  await OneCRLBlocklistClient.emit("sync", {
+    data: {
+      current: [],
+      created: [],
+      updated: [
+        {
+          old: {
+            issuerName: "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=",
+            serialNumber: "a0X7/7DlTaedpgrIJg25iBPOkIM=",
+          },
+          new: {
+            issuerName: "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=",
+            serialNumber: "ALtF+/+w5U0=",
+          },
+        },
+      ],
+      deleted: [],
+    },
+  });
+  let oldRevocationState = certStorage.getRevocationState(
+    issuerArray,
+    serialArray,
+    [],
+    []
+  );
+  Assert.equal(oldRevocationState, Ci.nsICertStorage.STATE_UNSET);
+
+  let newSerialArray = [0x00, 0xbb, 0x45, 0xfb, 0xff, 0xb0, 0xe5, 0x4d];
+  let newRevocationState = certStorage.getRevocationState(
+    issuerArray,
+    newSerialArray,
+    [],
+    []
+  );
+  Assert.equal(newRevocationState, Ci.nsICertStorage.STATE_ENFORCE);
+});