fix: offer and keep a key from a message only when it names the sender alone #58

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<TestCert> {
+export async function makeSmimeCert(email: string, opts: { notAfter?: Date; extraEmails?: string[] } = {}): Promise<TestCert> {
   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());

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' })]));

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<PgpKeyRecord> {
   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 {

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<PgpKeyInfo, 'userIDs'>, 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 {

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