#6280 Improve validation of OpenPGP revocation signatures for contact keys

extension/chrome/dev/ci_unit_test.ts

@@ -15,6 +15,8 @@ import { Sks } from '../../js/common/api/key-server/sks.js';
 import { Ui } from '../../js/common/browser/ui.js';
 import { AcctStore } from '../../js/common/platform/store/acct-store.js';
 import { ContactStore } from '../../js/common/platform/store/contact-store.js';
+import { GlobalStore } from '../../js/common/platform/store/global-store.js';
+import { revalidateStoredRevocations } from '../../js/service_worker/migrations.js';
 import { Debug } from '../../js/common/platform/debug.js';
 import { Catch } from '../../js/common/platform/catch.js';
 import { CatchHelper } from '../../js/common/platform/catch-helper.js';
@@ -45,13 +47,15 @@ const libs: unknown[] = [
   Url,
   AcctStore,
   ContactStore,
+  GlobalStore,
   Debug,
   Catch,
   CatchHelper,
   Gmail,
   PgpHash,
   PgpArmor,
   Xss,
+  revalidateStoredRevocations,
 ];
 /* eslint-disable @typescript-eslint/no-explicit-any */
 // add them to global scope so ci can use them

extension/js/common/core/crypto/key.ts

@@ -431,6 +431,13 @@ export class KeyUtil {
     }
   }
 
+  public static async isRevoked(key: Key): Promise<boolean> {
+    if (key.family === 'openpgp') {
+      return await OpenPGPKey.isRevoked(key);
+    }
+    return key.revoked;
+  }
+
   public static async keyInfoObj(prv: Key): Promise<KeyInfoWithIdentity> {
     if (!prv.isPrivate) {
       throw new Error('Key passed into KeyUtil.keyInfoObj must be a Private Key');

extension/js/common/core/crypto/pgp/openpgp-key.ts

@@ -235,7 +235,7 @@ export class OpenPGPKey {
         curve: algoInfo.curve,
         algorithmId: opgp.enums.publicKey[algoInfo.algorithm],
       },
-      revoked: opgpKey.revocationSignatures.length > 0,
+      revoked: await opgpKey.isRevoked(),
     } as Key);
     const keyWithPrivateFields = key as KeyWithPrivateFields;
     keyWithPrivateFields.internal = opgpKey;
@@ -305,6 +305,11 @@ export class OpenPGPKey {
     return key.users.length === 0;
   }
 
+  public static async isRevoked(key: Key): Promise<boolean> {
+    const opgpKey = await OpenPGPKey.extractExternalLibraryObjFromKey(key);
+    return await opgpKey.isRevoked();
+  }
+
   public static async diagnose(pubkey: Key, passphrase: string): Promise<Map<string, string>> {
     const key = await OpenPGPKey.extractExternalLibraryObjFromKey(pubkey);
     const result = new Map<string, string>();

extension/js/common/platform/store/contact-store.ts

@@ -26,6 +26,7 @@ export type Pubkey = {
 
 type Revocation = {
   fingerprint: string;
+  armoredKey?: string;
 };
 
 type PubkeyAttributes = {
@@ -504,6 +505,9 @@ export class ContactStore extends AbstractStore {
       Catch.report(`Wrongly updating prv ${pubkey.id} as contact - converting to pubkey`);
       pubkey = await KeyUtil.asPublicKey(pubkey);
     }
+    if (pubkey?.family === 'openpgp' && pubkey.revoked && !(await KeyUtil.isRevoked(pubkey))) {
+      throw Error(`Refusing to store key ${pubkey.id} with an unverifiable revocation signature for ${validEmail}`);
+    }
     const tx = db.transaction(['emails', 'pubkeys', 'revocations'], 'readwrite');
     await new Promise((resolve, reject) => {
       ContactStore.setTxHandlers(tx, resolve, reject);
@@ -741,6 +745,9 @@ export class ContactStore extends AbstractStore {
     if (!pubkey.revoked) {
       throw new Error('Non-revoked key is supplied to save revocation info');
     }
+    if (!(await KeyUtil.isRevoked(pubkey))) {
+      throw new Error(`Key ${pubkey.id} does not carry a verifiable revocation signature`);
+    }
     if (!db) {
       // relay op through background process
       KeyUtil.pack(pubkey);

extension/js/common/platform/store/global-store.ts

@@ -21,6 +21,7 @@ export type GlobalStoreDict = {
   stored_key_info_migrated?: boolean;
   contact_store_x509_fingerprints_and_longids_updated?: boolean;
   contact_store_opgp_revoked_flags_updated?: boolean;
+  contact_store_revocations_revalidated?: boolean;
   contact_store_searchable_pruned?: boolean;
   local_drafts?: Dict<LocalDraft>;
 };
@@ -36,6 +37,7 @@ export type GlobalIndex =
   | 'key_info_store_fingerprints_added'
   | 'contact_store_x509_fingerprints_and_longids_updated'
   | 'contact_store_opgp_revoked_flags_updated'
+  | 'contact_store_revocations_revalidated'
   | 'contact_store_searchable_pruned'
   | 'local_drafts'
   | 'stored_key_info_migrated';

extension/js/service_worker/background.ts

@@ -10,7 +10,14 @@ import { BgHandlers } from './bg-handlers.js';
 import { Catch } from '../common/platform/catch.js';
 import { ContactStore } from '../common/platform/store/contact-store.js';
 import { BgUtils } from './bgutils.js';
-import { migrateGlobal, moveContactsToEmailsAndPubkeys, updateOpgpRevocations, updateSearchables, updateX509FingerprintsAndLongids } from './migrations.js';
+import {
+  migrateGlobal,
+  moveContactsToEmailsAndPubkeys,
+  revalidateStoredRevocations,
+  updateOpgpRevocations,
+  updateSearchables,
+  updateX509FingerprintsAndLongids,
+} from './migrations.js';
 import { GlobalStore, GlobalStoreDict } from '../common/platform/store/global-store.js';
 import { VERSION } from '../common/core/const.js';
 import { injectFcIntoWebmail } from './inject.js';
@@ -41,6 +48,7 @@ console.info('background.js service worker starting');
   try {
     db = await ContactStore.dbOpen(); // takes 4-10 ms first time
     await updateOpgpRevocations(db);
+    await revalidateStoredRevocations(db);
     await updateX509FingerprintsAndLongids(db);
     await updateSearchables(db);
     await moveContactsToEmailsAndPubkeys(db);

extension/js/service_worker/migrations.ts

@@ -216,6 +216,48 @@ export const updateOpgpRevocations = async (db: IDBDatabase): Promise<void> => {
   console.info('done updating');
 };
 
+type StoredRevocation = { fingerprint: string; armoredKey?: string };
+
+export const revalidateStoredRevocations = async (db: IDBDatabase): Promise<void> => {
+  const globalStore = await GlobalStore.get(['contact_store_revocations_revalidated']);
+  if (globalStore.contact_store_revocations_revalidated) {
+    return;
+  }
+  console.info('re-validating stored revocation records...');
+  const tx = db.transaction(['revocations'], 'readonly');
+  const records = await new Promise<StoredRevocation[]>((resolve, reject) => {
+    const search = tx.objectStore('revocations').getAll();
+    ContactStore.setReqPipe(search, resolve, reject);
+  });
+  const bogusFingerprints: string[] = [];
+  for (const record of records) {
+    if (!record.armoredKey) {
+      continue;
+    }
+    try {
+      const key = await KeyUtil.parse(record.armoredKey);
+      if (key.family === 'openpgp' && (key.id !== record.fingerprint || !key.revoked)) {
+        bogusFingerprints.push(record.fingerprint);
+      }
+    } catch (e) {
+      console.error(`Skipping unparsable stored revocation record ${record.fingerprint}: ${e instanceof Error ? e.message : String(e)}`);
+    }
+  }
+  if (bogusFingerprints.length) {
+    const txUpdate = db.transaction(['revocations'], 'readwrite');
+    await new Promise((resolve, reject) => {
+      ContactStore.setTxHandlers(txUpdate, resolve, reject);
+      const revocationsStore = txUpdate.objectStore('revocations');
+      for (const fingerprint of bogusFingerprints) {
+        revocationsStore.delete(fingerprint);
+      }
+    });
+  }
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  await GlobalStore.set({ contact_store_revocations_revalidated: true });
+  console.info('done re-validating stored revocation records');
+};
+
 export const moveContactsToEmailsAndPubkeys = async (db: IDBDatabase): Promise<void> => {
   if (!db.objectStoreNames.contains('contacts')) {
     return;

test/source/tests/browser-unit-tests/unit-ContactStore.js

@@ -520,3 +520,71 @@ BROWSER_UNIT_TEST_NAME(`ContactStore searchPubkeys { hasPgp: true } returns all
   }
   return 'pass';
 })();
+
+BROWSER_UNIT_TEST_NAME(`ContactStore refuses to store unverifiable revocation and revalidateStoredRevocations purges it`);
+(async () => {
+  const db = await ContactStore.dbOpen();
+  const email = 'some.revoked@localhost.com';
+  const validPubkey = await KeyUtil.parse(testConstants.somerevokedValid);
+  const validFingerprint = 'D6662C5FB9BDE9DA01F3994AAA1EF832D8CCA4F2';
+  if (validPubkey.id !== validFingerprint || validPubkey.revoked) {
+    throw new Error(`Expected a valid key ${validFingerprint} but got ${validPubkey.id} (revoked: ${validPubkey.revoked})`);
+  }
+  const listRevocations = async () => {
+    return await new Promise((resolve, reject) => {
+      const req = db.transaction(['revocations'], 'readonly').objectStore('revocations').getAll();
+      ContactStore.setReqPipe(req, resolve, reject);
+    });
+  };
+  await new Promise((resolve, reject) => {
+    const tx = db.transaction(['revocations'], 'readwrite');
+    ContactStore.setTxHandlers(tx, resolve, reject);
+    tx.objectStore('revocations').put({ fingerprint: validFingerprint, armoredKey: KeyUtil.armor(validPubkey) });
+  });
+  if (!(await listRevocations()).some(r => r.fingerprint === validFingerprint)) {
+    throw new Error('Failed to set up an unverifiable revocation record');
+  }
+  const tampered = await KeyUtil.parse(testConstants.somerevokedValid);
+  tampered.revoked = true;
+  let rejectionMessage;
+  try {
+    await ContactStore.saveRevocation(db, tampered);
+  } catch (e) {
+    rejectionMessage = String(e);
+  }
+  if (!rejectionMessage || !rejectionMessage.includes('does not carry a verifiable revocation signature')) {
+    throw new Error(`saveRevocation was expected to reject unverifiable revocation but it returned "${rejectionMessage}"`);
+  }
+  const genuinelyRevoked = await KeyUtil.parse(testConstants.somerevokedRevoked2);
+  if (!genuinelyRevoked.revoked) {
+    throw new Error('Control key was expected to be genuinely revoked');
+  }
+  const genuineFingerprint = '3930752556D57C46A1C56B63DE8538DDA1648C76';
+  if (genuinelyRevoked.id !== genuineFingerprint) {
+    throw new Error(`Expected control fingerprint ${genuineFingerprint} but got ${genuinelyRevoked.id}`);
+  }
+  await ContactStore.saveRevocation(db, genuinelyRevoked);
+  if (!(await listRevocations()).some(r => r.fingerprint === genuineFingerprint)) {
+    throw new Error('Failed to store a genuine revocation record');
+  }
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  await GlobalStore.set({ contact_store_revocations_revalidated: false });
+  await revalidateStoredRevocations(db);
+  const records = await listRevocations();
+  if (records.some(r => r.fingerprint === validFingerprint)) {
+    throw new Error('The unverifiable revocation record was expected to be purged by revalidation');
+  }
+  if (!records.some(r => r.fingerprint === genuineFingerprint)) {
+    throw new Error('The genuine revocation record was expected to survive revalidation');
+  }
+  await ContactStore.update(db, email, { pubkey: validPubkey });
+  const { sortedPubkeys } = await ContactStore.getOneWithAllPubkeys(db, email);
+  const restoredEntry = sortedPubkeys.find(x => x.pubkey.id === validFingerprint);
+  if (!restoredEntry) {
+    throw new Error(`Expected to find pubkey ${validFingerprint} after revalidation`);
+  }
+  if (restoredEntry.revoked) {
+    throw new Error(`Pubkey ${validFingerprint} was expected to be usable after revalidation but it is still considered revoked`);
+  }
+  return 'pass';
+})();

test/source/tests/unit-node.ts

@@ -188,6 +188,28 @@ Something wrong with this key`),
       expect(await KeyUtil.getOrCreateRevocationCertificate(revokedPub)).to.equal(revocationCertificate);
       expect(await KeyUtil.getOrCreateRevocationCertificate(revokedPrv)).to.equal(revocationCertificate);
     });
+    test(`[unit][OpenPGPKey.parse] does not treat a foreign revocation signature packet as key revocation`, async t => {
+      const attackerPrv = await OpenPGPKey.parse(testConstants.existingPrv);
+      const attackerRevocationCertificate = await OpenPGPKey.getOrCreateRevocationCertificate(attackerPrv);
+      if (!attackerRevocationCertificate) {
+        throw new Error();
+      }
+      const attackerRevokedPub = await OpenPGPKey.applyRevocationCertificate(await KeyUtil.asPublicKey(attackerPrv), attackerRevocationCertificate);
+      expect(attackerRevokedPub.revoked).to.be.true;
+      const victimPub = await KeyUtil.parse(testConstants.somerevokedValid);
+      expect(victimPub.revoked).to.be.false;
+      const attackerOpgpPub = await opgp.readKey({ armoredKey: KeyUtil.armor(attackerRevokedPub) });
+      const victimPackets = (await opgp.readKey({ armoredKey: KeyUtil.armor(victimPub) })).toPacketList();
+      victimPackets.splice(1, 0, attackerOpgpPub.revocationSignatures[0]);
+      const forgedArmored = new opgp.PublicKey(victimPackets).armor();
+      const forgedOpgp = await opgp.readKey({ armoredKey: forgedArmored });
+      expect(forgedOpgp.revocationSignatures.length).to.equal(1);
+      expect(await forgedOpgp.isRevoked()).to.be.false;
+      const forgedParsed = await KeyUtil.parse(forgedArmored);
+      expect(forgedParsed.id).to.equal(victimPub.id);
+      expect(forgedParsed.revoked).to.be.false;
+      t.pass();
+    });
     test(`[unit][MsgBlockParser.detectBlocks] does not get tripped on blocks with unknown headers`, async t => {
       expect(
         MsgBlockParser.detectBlocks("This text breaks email and Gmail web app.\n\n-----BEGIN FOO-----\n\nEven though it's not a vaild PGP m\n\nMuhahah")