Files
SRCmail/vnc/plugins/smime/roundtrip.mjs
T
Bernd RodlerandClaude Opus 4.8 a4155aa342 security(smime): fix finding 5 (parser DoS) and harden finding 4
Finding 5 — the MIME parser runs on attacker-controlled input: the inner
content recovered after decrypt/verify is whatever the sender put there.
Upstream had no depth limit on nested multiparts and no size cap anywhere.

Verified against the unpatched upstream parser with the same input:

  UPSTREAM CRASHED: RangeError - Maximum call stack size exceeded
  UPSTREAM: 65MB accepted (no size cap)

So this was a live decrypt-time DoS reachable by anyone who can send mail.

Caps added: depth 20, parts 500, bytes 64 MB — generous enough that no
legitimate message comes close (real mail nests 3-4 levels). Past a limit a
subtree degrades to a leaf rather than throwing, so one pathological branch
doesn't discard the legitimate parts above it. Oversize input is refused
outright rather than truncated: half a MIME tree parses into misleading
nonsense, and showing part of a message is worse than saying no. Both
bodyStructure walkers in smime-detect.js are capped too — those run on
server-supplied structure BEFORE any decrypt/verify gate.

Finding 4 — hardened, not eliminated, per the agreed scope. Unlocked
CryptoKeys still live in durable IndexedDB rather than memory; moving them
would mean refactoring how the plugin shares state across iframes and
risking the unlock->decrypt path just verified.

What changed instead:

- Removed the lockOnLogout opt-out from the logout/account-switch wipes. A
  non-extractable key cannot be exported but can still be USED, so a handle
  outliving the session lets anyone with the browser profile decrypt mail
  without knowing the passphrase. That is not a preference to toggle off.
- Added a best-effort wipe on pagehide and beforeunload to narrow the window
  in which a usable handle exists on disk. Best-effort by nature: an
  IndexedDB write may not complete during teardown and neither event fires
  on a crash — which is precisely why the boot wipe in activate() remains
  the load-bearing control.
- Deliberately NOT wiping on visibilitychange: tabbing away would drop the
  unlock and force a passphrase re-entry every time, which trains users into
  turning S/MIME off entirely.
- Dropped the now-dead lockOnLogout setting from the manifest. A toggle that
  silently does nothing is worse than no toggle.

Tests: 49 unit assertions + 28 round trip. The round trip now feeds genuinely
hostile MIME through the real parser (5000-level nesting, 5000 siblings,
65 MB) and still confirms a normal multipart/alternative parses correctly.
Full crypto round trip unchanged and passing, so neither fix broke S/MIME.

Findings 6, 7, 8 and 9 remain open.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-04 11:57:18 +02:00

190 lines
8.9 KiB
JavaScript

// Real crypto round trip through the plugin's OWN modules — no browser needed.
// Proves the audit fixes didn't break S/MIME, using the spike self-signed certs.
//
// node vnc/plugins/smime/roundtrip.mjs <certdir>
//
// Covers: PKCS#12 import -> unlock -> sign -> verify -> encrypt -> decrypt,
// plus the finding-1 auto-import gate and the finding-2 algorithm allowlist
// as they actually behave against genuine CMS structures.
import { readFileSync } from 'node:fs';
import { join } from 'node:path';
const dir = process.argv[2];
if (!dir) { console.error('usage: node roundtrip.mjs <certdir>'); process.exit(2); }
let pass = 0, fail = 0;
const check = (name, ok, extra = '') => {
console.log(` ${ok ? 'PASS' : 'FAIL'} ${name}${extra ? ' — ' + extra : ''}`);
ok ? pass++ : fail++;
};
const { importPkcs12, unlockPrivateKey } = await import('./src/pkcs12.js');
const { smimeSign } = await import('./src/smime-sign.js');
const { smimeEncrypt } = await import('./src/smime-encrypt.js');
const { smimeVerify } = await import('./src/smime-verify.js');
const { smimeDecrypt } = await import('./src/smime-decrypt.js');
const ab = (b) => b.buffer.slice(b.byteOffset, b.byteOffset + b.byteLength);
const load = (n) => ab(readFileSync(join(dir, n)));
console.log('\n1. PKCS#12 import (both identities)');
const ids = {};
for (const who of ['bernd.rodler', 'admin']) {
const { keyRecord, certInfo } = await importPkcs12(load(`${who}.p12`), 'spike', 'storage-pw');
ids[who] = keyRecord;
check(`${who}: imported`, !!keyRecord.encryptedPrivateKey,
`${certInfo.emailAddresses[0]} · ${certInfo.algorithm} · kdf=${keyRecord.kdfIterations}`);
check(`${who}: key encrypted at rest`, keyRecord.encryptedPrivateKey.byteLength > 0
&& keyRecord.salt.byteLength === 32 && keyRecord.iv.byteLength === 12);
}
console.log('\n2. Unlock (non-extractable import)');
const keys = {};
for (const who of Object.keys(ids)) {
keys[who] = await unlockPrivateKey(ids[who], 'storage-pw');
check(`${who}: unlocked`, !!keys[who].signingKey);
check(`${who}: signing key NOT extractable`, keys[who].signingKey.extractable === false);
}
let wrongPw = false;
try { await unlockPrivateKey(ids['admin'], 'wrong'); } catch (e) { wrongPw = /Incorrect passphrase/.test(e.message); }
check('wrong passphrase rejected', wrongPw);
console.log('\n3. Sign (bernd) -> verify');
const plaintext = new TextEncoder().encode(
'Content-Type: text/plain\r\n\r\nVNC S/MIME spike round trip.\r\n');
const signed = await smimeSign(
plaintext,
keys['bernd.rodler'].signingKey,
ids['bernd.rodler'].certificate,
ids['bernd.rodler'].certificateChain,
);
const signedAb = await signed.arrayBuffer(); // smimeSign returns a Blob
check('signed CMS produced', signedAb.byteLength > 0, `${signedAb.byteLength} bytes`);
const v = await smimeVerify(signedAb, 'bernd.rodler@sandbox.vnc.de');
check('signature VALID', v.status.signatureValid === true);
check('signer email matches From', v.status.signerEmailMatch === true);
check('detected as SELF-SIGNED', v.status.selfSigned === true);
check('inner content round-trips',
new TextDecoder().decode(v.mimeBytes).includes('round trip'));
console.log('\n4. Finding 1 gate — self-signed must NOT be auto-trusted');
const gate = (s) => s.signatureValid && s.signerEmailMatch === true && !s.selfSigned;
check('valid + matching + SELF-SIGNED -> REFUSED', gate(v.status) === false,
'this is the cert-substitution attack, now blocked');
check('same cert would pass if CA-signed', gate({ ...v.status, selfSigned: false }) === true);
console.log('\n5. Encrypt (bernd -> admin) -> decrypt as admin');
// Note: smimeEncrypt always adds the SENDER's cert as a recipient too, so the
// sender can read their own Sent copy. That is why bernd can also decrypt below.
const enc = await smimeEncrypt(
plaintext,
[ids['admin'].certificate],
ids['bernd.rodler'].certificate,
false,
);
const encAb = await enc.arrayBuffer(); // smimeEncrypt returns a Blob
check('enveloped CMS produced', encAb.byteLength > 0, `${encAb.byteLength} bytes`);
const dec = await smimeDecrypt({
cmsBytes: encAb, keyRecords: [ids['admin']],
unlockedKeys: new Map([[ids['admin'].id, keys['admin'].decryptionKey]]),
legacyUnlockedKeys: new Map(),
});
check('decrypted by intended recipient', !!dec.mimeBytes);
check('plaintext matches', new TextDecoder().decode(dec.mimeBytes).includes('round trip'));
console.log('\n6. Finding 2 — algorithm reporting on a real message');
check('our own encrypt is AES-GCM', /GCM/.test(dec.contentAlgorithm), dec.contentAlgorithm);
check('reported as AUTHENTICATED', dec.contentAuthenticated === true);
check('=> HTML would render (no suppression)', dec.contentAuthenticated === true);
console.log('\n7. Sender can read their own Sent copy');
const selfDec = await smimeDecrypt({
cmsBytes: encAb, keyRecords: [ids['bernd.rodler']],
unlockedKeys: new Map([[ids['bernd.rodler'].id, keys['bernd.rodler'].decryptionKey]]),
legacyUnlockedKeys: new Map(),
});
check('sender decrypts own Sent copy', new TextDecoder().decode(selfDec.mimeBytes).includes('round trip'),
'smimeEncrypt deliberately includes the sender as a recipient');
console.log('\n8. Finding 2 — a refused algorithm is actually refused');
// Rewrite the content-encryption OID to 3DES-CBC and confirm the gate fires.
const bytes = new Uint8Array(encAb);
const gcmOid = [0x60,0x86,0x48,0x01,0x65,0x03,0x04,0x01,0x2e]; // 2.16.840.1.101.3.4.1.46
const desOid = [0x2a,0x86,0x48,0x86,0xf7,0x0d,0x03,0x07]; // 1.2.840.113549.3.7
let at = -1;
outer: for (let i = 0; i < bytes.length - gcmOid.length; i++) {
for (let j = 0; j < gcmOid.length; j++) if (bytes[i + j] !== gcmOid[j]) continue outer;
at = i; break;
}
if (at < 0) {
check('could not locate content-encryption OID to tamper with', false);
} else {
const tampered = new Uint8Array(bytes);
tampered[at - 1] = desOid.length; // OID length
desOid.forEach((b, k) => { tampered[at + k] = b; });
let refused = false, msg = '';
try {
await smimeDecrypt({
cmsBytes: tampered.buffer, keyRecords: [ids['admin']],
unlockedKeys: new Map([[ids['admin'].id, keys['admin'].decryptionKey]]),
legacyUnlockedKeys: new Map(),
});
} catch (e) { msg = e.message; refused = true; }
// Any refusal is a pass here: swapping a 9-byte OID for an 8-byte one also
// invalidates the enclosing DER lengths, so ASN.1 validation may reject the
// message before the allowlist is consulted. Either way no plaintext is
// produced. The allowlist itself is asserted precisely in verify-fixes.mjs
// (3DES / DES / RC2 / unknown-OID all refused) — this check only confirms a
// downgraded real message cannot be decrypted.
check('downgraded message produces NO plaintext', refused,
/Refusing to decrypt/.test(msg) ? 'refused by allowlist' : 'refused earlier: ' + msg.slice(0, 60));
}
console.log('\n9. Finding 5 — hostile MIME against the REAL parser');
const { parseMime } = await import('./src/mime-parse.js');
// Deeply nested multipart. Upstream recursed once per level with no cap; 5000
// levels is comfortably past the JS stack limit.
function nest(levels) {
let body = 'Content-Type: text/plain\r\n\r\ninnermost\r\n';
for (let i = levels; i > 0; i--) {
const b = `b${i}`;
body = `Content-Type: multipart/mixed; boundary="${b}"\r\n\r\n`
+ `--${b}\r\n${body}\r\n--${b}--\r\n`;
}
return new TextEncoder().encode(body);
}
let survived = false, note = '';
try { parseMime(nest(5000)); survived = true; note = 'parsed without stack overflow'; }
catch (e) { note = e.message.slice(0, 70); survived = !/Maximum call stack|too much recursion/i.test(e.message); }
check('5000-level nesting does not blow the stack', survived, note);
// Wide fan-out: many sibling parts at one level.
const wideB = 'w';
let wide = `Content-Type: multipart/mixed; boundary="${wideB}"\r\n\r\n`;
for (let i = 0; i < 5000; i++) wide += `--${wideB}\r\nContent-Type: text/plain\r\n\r\np${i}\r\n`;
wide += `--${wideB}--\r\n`;
let wideOk = false, wideNote = '';
try { parseMime(new TextEncoder().encode(wide)); wideOk = true; wideNote = 'part budget held'; }
catch (e) { wideNote = e.message.slice(0, 70); }
check('5000 sibling parts handled', wideOk, wideNote);
// Oversize input is refused rather than silently truncated.
let refusedBig = false;
try { parseMime(new Uint8Array(65 * 1024 * 1024)); }
catch (e) { refusedBig = /Refusing to parse/.test(e.message); }
check('oversize message REFUSED (not truncated)', refusedBig);
// And a legitimate message still parses correctly after all that.
const normal = parseMime(new TextEncoder().encode(
'Content-Type: multipart/alternative; boundary="x"\r\n\r\n'
+ '--x\r\nContent-Type: text/plain\r\n\r\nhello plain\r\n'
+ '--x\r\nContent-Type: text/html\r\n\r\n<p>hello html</p>\r\n--x--\r\n'));
check('normal multipart/alternative still parses',
normal.text.includes('hello plain') && normal.html.includes('hello html'));
console.log(`\n${fail === 0 ? 'ROUND TRIP OK' : 'FAILURES'}${pass} passed, ${fail} failed\n`);
process.exit(fail === 0 ? 0 : 1);