From ac312352ba0c304e7cc5890029221bba01a158e7 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:05:32 +0100 Subject: [PATCH] fix: offer and keep a key from a message only when it names the sender alone #58 --- src/lib/crypto/__tests__/helpers.ts | 6 ++-- .../__tests__/secure-message-keys.test.ts | 28 +++++++++++++++++++ src/lib/crypto/keyring.ts | 13 +++++---- src/lib/crypto/pgp.ts | 11 ++++++++ src/lib/crypto/secure-message.ts | 4 +-- 5 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/lib/crypto/__tests__/helpers.ts b/src/lib/crypto/__tests__/helpers.ts index 984c4960..c82228c2 100644 --- a/src/lib/crypto/__tests__/helpers.ts +++ b/src/lib/crypto/__tests__/helpers.ts @@ -43,7 +43,7 @@ export interface TestCert { const engine = () => new pkijs.CryptoEngine({ name: 'test', crypto: globalThis.crypto }); /** A self-signed RSA-2048 S/MIME certificate for `email`. */ -export async function makeSmimeCert(email: string, opts: { notAfter?: Date } = {}): Promise { +export async function makeSmimeCert(email: string, opts: { notAfter?: Date; extraEmails?: string[] } = {}): Promise { const subtle = globalThis.crypto.subtle; const keys = await subtle.generateKey( { name: 'RSASSA-PKCS1-v1_5', modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: 'SHA-256' }, @@ -67,7 +67,9 @@ export async function makeSmimeCert(email: string, opts: { notAfter?: Date } = { new pkijs.Extension({ extnID: '2.5.29.15', critical: true, extnValue: ku.toBER(false), parsedValue: ku }), new pkijs.Extension({ extnID: '2.5.29.17', - extnValue: new pkijs.GeneralNames({ names: [new pkijs.GeneralName({ type: 1, value: email })] }).toSchema().toBER(false), + extnValue: new pkijs.GeneralNames({ + names: [email, ...(opts.extraEmails ?? [])].map((value) => new pkijs.GeneralName({ type: 1, value })), + }).toSchema().toBER(false), }), ]; await cert.subjectPublicKeyInfo.importKey(keys.publicKey, engine()); diff --git a/src/lib/crypto/__tests__/secure-message-keys.test.ts b/src/lib/crypto/__tests__/secure-message-keys.test.ts index 7ba6ec67..92151e68 100644 --- a/src/lib/crypto/__tests__/secure-message-keys.test.ts +++ b/src/lib/crypto/__tests__/secure-message-keys.test.ts @@ -75,6 +75,21 @@ describe('a PGP key in a message', () => { expect(stored()).toEqual([]); }, 60_000); + it('is neither stored nor offered when it also names another address', async () => { + // Mallory's own address and the boss's on one key: saved for Mallory, + // it would be encrypted to for the boss and verify mail forged from them. + const { privateKey, publicKey } = await openpgp.generateKey({ + type: 'ecc', curve: 'curve25519Legacy', + userIDs: [{ name: 'Mallory', email: mallory.email }, { name: 'Boss', email: 'boss@example.org' }], + format: 'armored', + }); + const signer = await openpgp.readPrivateKey({ armoredKey: privateKey }); + const res = await openSecureMessage(await inlineSigned(mallory.email, signer, publicKey), mallory.email, keyringAccess()); + expect(res?.offeredKey).toBeUndefined(); + await expect(savePgpKeyFor(publicKey, mallory.email)).rejects.toThrow(/not for mallory@example.org alone/); + expect(stored()).toEqual([]); + }, 60_000); + it('is neither stored nor offered when the signature fails', async () => { await importPgpKey(alice.publicKey); const res = await openSecureMessage(await inlineSigned(alice.email, alice.unlocked, alice.publicKey, true), alice.email, keyringAccess()); @@ -119,6 +134,19 @@ describe('an S/MIME signer certificate', () => { expect(stored()).toEqual([]); }, 60_000); + it('is not kept when it also names another address', async () => { + const both = await makeSmimeCert('mallory@example.org', { extraEmails: ['boss@example.org'] }); + const raw = await buildSecureMessage( + { from: { email: both.email }, to: [{ email: 'bob@example.org' }], subject: 's', textBody: 'signed' }, + { mode: 'smime', sign: true, encrypt: false }, + { smime: { signer: { pkcs8: both.pkcs8, certificate: both.certificate, chain: [], algorithm: 'RSA-2048' } } }, + ); + const res = await openSecureMessage(raw, both.email, keyringAccess()); + expect(res?.status.signature).toBe('valid'); + await new Promise((r) => setTimeout(r, 50)); + expect(stored()).toEqual([]); + }, 60_000); + it('is kept when it names the From address', async () => { await openSecureMessage(await signed(), signer.email, keyringAccess()); await vi.waitFor(() => expect(stored()).toEqual([expect.objectContaining({ kind: 'smime', emails: [signer.email], source: 'message' })])); diff --git a/src/lib/crypto/keyring.ts b/src/lib/crypto/keyring.ts index d13d022a..e31fd728 100644 --- a/src/lib/crypto/keyring.ts +++ b/src/lib/crypto/keyring.ts @@ -9,7 +9,7 @@ import type * as OpenPgp from 'openpgp'; import { useCryptoKeysStore, upsertRecord, keysForEmail, isRecordCurrent, type CryptoKeyRecord, type PgpKeyRecord, type SmimeCertRecord } from '../../stores/crypto-keys-store'; import { setSecret, getSecret, deleteSecret } from './secret-store'; -import { inspectKey, unlockPrivateKey, privateKeyNeedsPassphrase, type PgpKeyInfo } from './pgp'; +import { inspectKey, keyNamesOnly, unlockPrivateKey, privateKeyNeedsPassphrase, type PgpKeyInfo } from './pgp'; import { base64Decode, base64Encode } from './mime'; const pgpSecretId = (id: string) => `pgp.${id}`; @@ -227,10 +227,8 @@ export function allPgpPublicKeys(): string[] { export async function savePgpKeyFor(armored: string, email: string): Promise { const info = await inspectKey(armored); if (info.isPrivate) throw new Error('Only a public key can be saved from a message'); - const address = email.trim().toLowerCase(); - if (!address || !info.userIDs.some((u) => u.email.trim().toLowerCase() === address)) { - throw new Error('The key is not for ' + email); - } + // Only a key for the sender alone (see keyNamesOnly). + if (!keyNamesOnly(info, email)) throw new Error('The key is not for ' + email + ' alone'); const known = records().find((k): k is PgpKeyRecord => k.kind === 'pgp' && k.id === info.fingerprint.toLowerCase()); if (known) return known; const record = pgpRecord(info, false, 'message'); @@ -250,7 +248,10 @@ export async function rememberSignerCertificate(der: Uint8Array, fromEmail: stri const { describeCertificate } = await import('./smime/certificate'); const info = await describeCertificate(der); const from = fromEmail.trim().toLowerCase(); - if (!from || !info.emailAddresses.some((e) => e.toLowerCase() === from)) return; + // Only one for the sender alone: a certificate that also names another + // address would be encrypted to for it (see keyNamesOnly). + const named = info.emailAddresses.map((e) => e.trim().toLowerCase()).filter(Boolean); + if (!from || named.length === 0 || !named.every((e) => e === from)) return; if (records().some((k) => k.kind === 'smime' && k.id === info.fingerprint)) return; upsert(smimeRecord(info, der, [], false, 'message')); } catch { diff --git a/src/lib/crypto/pgp.ts b/src/lib/crypto/pgp.ts index 1475167e..74c6d873 100644 --- a/src/lib/crypto/pgp.ts +++ b/src/lib/crypto/pgp.ts @@ -26,6 +26,17 @@ export interface PgpKeyInfo { algorithm: string; } +/** + * Whether a key names `email` and no other address. A key that arrives in a + * message is offered and saved only then: one that also named another + * address would be encrypted to for it and would verify mail forged from it. + */ +export function keyNamesOnly(info: Pick, email: string): boolean { + const address = email.trim().toLowerCase(); + const named = info.userIDs.map((u) => u.email.trim().toLowerCase()).filter(Boolean); + return !!address && named.length > 0 && named.every((e) => e === address); +} + export type PgpSignatureStatus = 'none' | 'valid' | 'invalid' | 'unknown'; export interface PgpSignatureSummary { diff --git a/src/lib/crypto/secure-message.ts b/src/lib/crypto/secure-message.ts index c2671ce6..23f7deda 100644 --- a/src/lib/crypto/secure-message.ts +++ b/src/lib/crypto/secure-message.ts @@ -9,7 +9,7 @@ import type { BodyPart, Email } from '../../api/types'; import { binaryToBytes, bytesToBinary, decodeBody, findNode, parseEntity, type MimeNode } from './mime'; import { detectPgp, decryptMessage, verifyDetached, verifyCleartext, encryptionKeyIds, keyHasId, canonicalSignedBytes, - extractPublicKeyBlock, inspectKey, PgpArgon2UnsupportedError, type PgpSignatureSummary, + extractPublicKeyBlock, inspectKey, keyNamesOnly, PgpArgon2UnsupportedError, type PgpSignatureSummary, } from './pgp'; import type { SmimeKeyMaterial, SmimeSignatureStatus } from './smime/cms'; @@ -220,7 +220,7 @@ async function withOfferedKey(result: SecureResult, text: string | null | undefi try { const info = await inspectKey(block); if (info.isPrivate) return result; - if (!info.userIDs.some((u) => u.email.trim().toLowerCase() === from)) return result; + if (!keyNamesOnly(info, from)) return result; return { ...result, offeredKey: { armored: info.publicKeyArmored, fingerprint: info.fingerprint.toLowerCase(), email: from } }; } catch { // a broken embedded key: nothing to offer