From cf6ddb7a6a5ed896b74fe3d8507b7c91aac92e7a Mon Sep 17 00:00:00 2001 From: martgil Date: Mon, 24 Aug 2026 13:06:43 +0800 Subject: [PATCH 1/2] fix: improve validation of OpenPGP revocation signatures for contact keys --- extension/chrome/dev/ci_unit_test.ts | 4 ++ extension/js/common/core/crypto/key.ts | 7 ++ .../js/common/core/crypto/pgp/openpgp-key.ts | 7 +- .../js/common/platform/store/contact-store.ts | 7 ++ .../js/common/platform/store/global-store.ts | 2 + extension/js/service_worker/background.ts | 10 ++- extension/js/service_worker/migrations.ts | 44 ++++++++++++ .../browser-unit-tests/unit-ContactStore.js | 68 +++++++++++++++++++ test/source/tests/unit-node.ts | 22 ++++++ 9 files changed, 169 insertions(+), 2 deletions(-) diff --git a/extension/chrome/dev/ci_unit_test.ts b/extension/chrome/dev/ci_unit_test.ts index fb09eb6f9cc..c1abdbb6e8f 100644 --- a/extension/chrome/dev/ci_unit_test.ts +++ b/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,6 +47,7 @@ const libs: unknown[] = [ Url, AcctStore, ContactStore, + GlobalStore, Debug, Catch, CatchHelper, @@ -52,6 +55,7 @@ const libs: unknown[] = [ PgpHash, PgpArmor, Xss, + revalidateStoredRevocations, ]; /* eslint-disable @typescript-eslint/no-explicit-any */ // add them to global scope so ci can use them diff --git a/extension/js/common/core/crypto/key.ts b/extension/js/common/core/crypto/key.ts index ed49354cf09..cca607dc3e7 100644 --- a/extension/js/common/core/crypto/key.ts +++ b/extension/js/common/core/crypto/key.ts @@ -431,6 +431,13 @@ export class KeyUtil { } } + public static async isRevoked(key: Key): Promise { + if (key.family === 'openpgp') { + return await OpenPGPKey.isRevoked(key); + } + return key.revoked; + } + public static async keyInfoObj(prv: Key): Promise { if (!prv.isPrivate) { throw new Error('Key passed into KeyUtil.keyInfoObj must be a Private Key'); diff --git a/extension/js/common/core/crypto/pgp/openpgp-key.ts b/extension/js/common/core/crypto/pgp/openpgp-key.ts index 09ca9b0325c..1543ec7cea1 100644 --- a/extension/js/common/core/crypto/pgp/openpgp-key.ts +++ b/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 { + const opgpKey = await OpenPGPKey.extractExternalLibraryObjFromKey(key); + return await opgpKey.isRevoked(); + } + public static async diagnose(pubkey: Key, passphrase: string): Promise> { const key = await OpenPGPKey.extractExternalLibraryObjFromKey(pubkey); const result = new Map(); diff --git a/extension/js/common/platform/store/contact-store.ts b/extension/js/common/platform/store/contact-store.ts index bbfb8d11e52..8087f0a8608 100644 --- a/extension/js/common/platform/store/contact-store.ts +++ b/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); diff --git a/extension/js/common/platform/store/global-store.ts b/extension/js/common/platform/store/global-store.ts index b4fb9459c47..519ec3699d1 100644 --- a/extension/js/common/platform/store/global-store.ts +++ b/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; }; @@ -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'; diff --git a/extension/js/service_worker/background.ts b/extension/js/service_worker/background.ts index fd8e9800d12..2188ba67f35 100644 --- a/extension/js/service_worker/background.ts +++ b/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); diff --git a/extension/js/service_worker/migrations.ts b/extension/js/service_worker/migrations.ts index ec38450ef40..220a0a6ff38 100644 --- a/extension/js/service_worker/migrations.ts +++ b/extension/js/service_worker/migrations.ts @@ -216,6 +216,50 @@ export const updateOpgpRevocations = async (db: IDBDatabase): Promise => { console.info('done updating'); }; +type StoredRevocation = { fingerprint: string; armoredKey?: string }; + +export const revalidateStoredRevocations = async (db: IDBDatabase): Promise => { + const globalStore = await GlobalStore.get(['contact_store_revocations_revalidated']); + if (globalStore.contact_store_revocations_revalidated) { + return; + } + console.info('re-validating stored revocation records...'); + let records: StoredRevocation[] = []; + { + const tx = db.transaction(['revocations'], 'readonly'); + records = await new Promise((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 { + if (!(await KeyUtil.parse(record.armoredKey)).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 => { if (!db.objectStoreNames.contains('contacts')) { return; diff --git a/test/source/tests/browser-unit-tests/unit-ContactStore.js b/test/source/tests/browser-unit-tests/unit-ContactStore.js index 10e1be69d37..d77b9bbae41 100644 --- a/test/source/tests/browser-unit-tests/unit-ContactStore.js +++ b/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'; +})(); diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index 08f9a40458a..00b44a14d46 100644 --- a/test/source/tests/unit-node.ts +++ b/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") From 412af382fb19cf37a8b48d2bf44262328636068e Mon Sep 17 00:00:00 2001 From: martgil Date: Tue, 25 Aug 2026 18:35:22 +0800 Subject: [PATCH 2/2] fix: improve openpgp key detection for bogus key --- extension/js/service_worker/migrations.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/extension/js/service_worker/migrations.ts b/extension/js/service_worker/migrations.ts index 220a0a6ff38..184730f0664 100644 --- a/extension/js/service_worker/migrations.ts +++ b/extension/js/service_worker/migrations.ts @@ -224,21 +224,19 @@ export const revalidateStoredRevocations = async (db: IDBDatabase): Promise { - const search = tx.objectStore('revocations').getAll(); - ContactStore.setReqPipe(search, resolve, reject); - }); - } + const tx = db.transaction(['revocations'], 'readonly'); + const records = await new Promise((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 { - if (!(await KeyUtil.parse(record.armoredKey)).revoked) { + const key = await KeyUtil.parse(record.armoredKey); + if (key.family === 'openpgp' && (key.id !== record.fingerprint || !key.revoked)) { bogusFingerprints.push(record.fingerprint); } } catch (e) {