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>
155 lines
9.0 KiB
JavaScript
155 lines
9.0 KiB
JavaScript
// Standalone proof for the two VNC hardening fixes. Reimplements only the
|
|
// decision logic under test (no pkijs/DOM needed) so it runs with plain node.
|
|
// node vnc/plugins/smime/verify-fixes.mjs
|
|
import { readFileSync } from 'node:fs';
|
|
import { fileURLToPath } from 'node:url';
|
|
import { dirname, join } from 'node:path';
|
|
|
|
const here = dirname(fileURLToPath(import.meta.url));
|
|
let pass = 0, fail = 0;
|
|
const check = (name, got, want) => {
|
|
const ok = got === want;
|
|
console.log(`${ok ? ' PASS' : ' FAIL'} ${name}${ok ? '' : ` (got ${JSON.stringify(got)}, want ${JSON.stringify(want)})`}`);
|
|
ok ? pass++ : fail++;
|
|
};
|
|
|
|
// ── Fix 1: auto-import gate ─────────────────────────────────────────
|
|
// Mirrors the guard order in index.js maybeAutoImportSigner.
|
|
function wouldImport(status, autoImport = true) {
|
|
if (autoImport === false) return false;
|
|
const cert = status && status.signerCert;
|
|
if (!cert || !status.signatureValid || !cert.email) return false;
|
|
if (status.signerEmailMatch !== true) return false;
|
|
if (status.selfSigned) return false;
|
|
return true;
|
|
}
|
|
const cert = { email: 'a@b.com', fingerprint: 'ff' };
|
|
|
|
console.log('\nFix 1 — certificate auto-import gate');
|
|
check('CA-signed, address matches -> import',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: false }), true);
|
|
check('THE ATTACK: self-signed, address matches -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: true }), false);
|
|
check('address mismatch -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: false, selfSigned: false }), false);
|
|
check('signerEmailMatch undefined (no From) -> REFUSE (fail closed)',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: undefined, selfSigned: false }), false);
|
|
check('invalid signature -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: false, signerEmailMatch: true, selfSigned: false }), false);
|
|
check('setting off -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: false }, false), false);
|
|
|
|
// ── Fix 2 (finding 3): CRLF stripping ───────────────────────────────
|
|
function stripCrlf(value) {
|
|
return String(value).replace(/[\r\n]+[ \t]*/g, ' ');
|
|
}
|
|
console.log('\nFinding 3 — CRLF header sanitisation');
|
|
check('BCC injection via display name',
|
|
stripCrlf('Evil\r\nBcc: attacker@evil.com'), 'Evil Bcc: attacker@evil.com');
|
|
check('bare LF', stripCrlf('a\nb'), 'a b');
|
|
check('bare CR', stripCrlf('a\rb'), 'a b');
|
|
check('folded continuation collapsed', stripCrlf('a\r\n\tb'), 'a b');
|
|
check('multiple injected headers',
|
|
stripCrlf('x\r\nBcc: a@b.c\r\nReply-To: d@e.f'), 'x Bcc: a@b.c Reply-To: d@e.f');
|
|
check('clean value untouched', stripCrlf('Normal Subject'), 'Normal Subject');
|
|
check('non-ASCII untouched', stripCrlf('Grüße büro'), 'Grüße büro');
|
|
|
|
// ── Finding 2: content-encryption allowlist ─────────────────────────
|
|
const ALLOW = new Map([
|
|
['2.16.840.1.101.3.4.1.2', { name: 'AES-128-CBC', authenticated: false }],
|
|
['2.16.840.1.101.3.4.1.22', { name: 'AES-192-CBC', authenticated: false }],
|
|
['2.16.840.1.101.3.4.1.42', { name: 'AES-256-CBC', authenticated: false }],
|
|
['2.16.840.1.101.3.4.1.6', { name: 'AES-128-GCM', authenticated: true }],
|
|
['2.16.840.1.101.3.4.1.26', { name: 'AES-192-GCM', authenticated: true }],
|
|
['2.16.840.1.101.3.4.1.46', { name: 'AES-256-GCM', authenticated: true }],
|
|
]);
|
|
const accepts = (oid) => ALLOW.has(oid);
|
|
const authed = (oid) => ALLOW.get(oid)?.authenticated ?? null;
|
|
|
|
console.log('\nFinding 2 — content-encryption allowlist');
|
|
check('AES-256-GCM accepted', accepts('2.16.840.1.101.3.4.1.46'), true);
|
|
check('AES-256-GCM is authenticated', authed('2.16.840.1.101.3.4.1.46'), true);
|
|
check('AES-128-CBC accepted (RFC 5751 interop)', accepts('2.16.840.1.101.3.4.1.2'), true);
|
|
check('AES-128-CBC NOT authenticated', authed('2.16.840.1.101.3.4.1.2'), false);
|
|
check('3DES-CBC REFUSED', accepts('1.2.840.113549.3.7'), false);
|
|
check('DES-CBC REFUSED', accepts('1.3.14.3.2.7'), false);
|
|
check('RC2-CBC REFUSED', accepts('1.2.840.113549.3.2'), false);
|
|
check('unknown OID REFUSED', accepts('1.2.3.4.5'), false);
|
|
|
|
// HTML suppression decision (EFAIL mitigation)
|
|
const suppress = (contentAuthenticated, optOut = false) => !contentAuthenticated && !optOut;
|
|
check('GCM -> HTML rendered', suppress(true), false);
|
|
check('CBC -> HTML suppressed by default', suppress(false), true);
|
|
check('CBC + explicit opt-out -> HTML rendered', suppress(false, true), false);
|
|
|
|
// ── Finding 5: parser resource limits ───────────────────────────────
|
|
const MAX_DEPTH = 20, MAX_PARTS = 500;
|
|
// Mirrors the bounded recursion in parseEntity: past MAX_DEPTH a multipart is
|
|
// treated as a leaf; past MAX_PARTS siblings are dropped.
|
|
function walk(depth, budget) {
|
|
if (depth >= MAX_DEPTH) return { depth, recursed: false };
|
|
if (budget.parts >= MAX_PARTS) return { depth, recursed: false };
|
|
budget.parts += 1;
|
|
return walk(depth + 1, budget);
|
|
}
|
|
console.log('\nFinding 5 — parser resource limits');
|
|
check('recursion stops at MAX_DEPTH', walk(0, { parts: 0 }).depth, MAX_DEPTH);
|
|
check('part budget stops further recursion', walk(0, { parts: MAX_PARTS }).recursed, false);
|
|
check('deep-but-legal nesting still reaches the cap', walk(16, { parts: 0 }).depth, MAX_DEPTH);
|
|
|
|
// ── Source assertions: guard against silent regression ──────────────
|
|
console.log('\nSource assertions');
|
|
const idx = readFileSync(join(here, 'src/index.js'), 'utf8');
|
|
const mb = readFileSync(join(here, 'src/mime-builder.js'), 'utf8');
|
|
check('index.js checks signerEmailMatch !== true', idx.includes('status.signerEmailMatch !== true'), true);
|
|
check('index.js checks selfSigned', /if \(status\.selfSigned\)/.test(idx), true);
|
|
check('formatHeader sanitises its value', /function formatHeader\(name, rawValue\)[\s\S]{0,80}stripCrlf\(rawValue\)/.test(mb), true);
|
|
check('attachment Content-Type sanitised', mb.includes('stripCrlf(att.contentType)'), true);
|
|
check('Content-ID sanitised', mb.includes('stripCrlf(att.cid)'), true);
|
|
// Every header assembled outside formatHeader must use a literal or a sanitised value.
|
|
const bypass = [...mb.matchAll(/lines\.push\(`([A-Za-z-]+): ([^`]*)`\)/g)]
|
|
.filter(([, , v]) => /\$\{/.test(v) && !/stripCrlf|encodeHeaderValue|boundary|altBoundary|disposition/.test(v));
|
|
check('no unsanitised interpolated headers remain', bypass.length, 0);
|
|
if (bypass.length) bypass.forEach(([m]) => console.log(' >>', m));
|
|
|
|
const dec = readFileSync(join(here, 'src/smime-decrypt.js'), 'utf8');
|
|
check('allowlist gate runs before any key use',
|
|
dec.indexOf('checkContentEncryption(envelopedData)') < dec.indexOf('unlockedKeys.get'), true);
|
|
check('decrypt refuses non-allowlisted algorithms', /Refusing to decrypt/.test(dec), true);
|
|
check('no legacy CBC OID appears in the decrypt allowlist',
|
|
/1\.2\.840\.113549\.3\.7|1\.3\.14\.3\.2\.7|1\.2\.840\.113549\.3\.2/.test(dec), false);
|
|
check('normal decrypt path uses the NATIVE engine',
|
|
/if \(!useLiner\)[\s\S]{0,120}nativeEngine\(\)/.test(dec), true);
|
|
check('liner engine reachable only via useLiner',
|
|
(dec.match(/getLinerCryptoEngine\(\)/g) || []).length, 1);
|
|
check('index.js suppresses HTML for unauthenticated content',
|
|
idx.includes('suppressHtml') && idx.includes('result.contentAuthenticated'), true);
|
|
|
|
// finding 5
|
|
const mp = readFileSync(join(here, 'src/mime-parse.js'), 'utf8');
|
|
const det = readFileSync(join(here, 'src/smime-detect.js'), 'utf8');
|
|
check('mime-parse caps depth/parts/bytes',
|
|
/MAX_DEPTH/.test(mp) && /MAX_PARTS/.test(mp) && /MAX_BYTES/.test(mp), true);
|
|
check('parseEntity threads depth + budget',
|
|
/function parseEntity\(raw, depth = 0, budget/.test(mp), true);
|
|
check('parseEntity guards on depth before recursing',
|
|
/params\.boundary && depth < MAX_DEPTH/.test(mp), true);
|
|
check('no unbounded .map(parseEntity) left', /\.map\(parseEntity\)/.test(mp), false);
|
|
check('both bodyStructure walkers are depth-capped',
|
|
(det.match(/depth < MAX_WALK_DEPTH/g) || []).length, 2);
|
|
|
|
// finding 4 hardening
|
|
check('lockOnLogout opt-out removed from wipe paths',
|
|
/settings\(\)\.lockOnLogout === false\) return/.test(idx), false);
|
|
check('exit wipe registered (pagehide + beforeunload)',
|
|
idx.includes("addEventListener('pagehide'") && idx.includes("addEventListener('beforeunload'"), true);
|
|
check('boot wipe still present', /clearSessionKeys\(\)/.test(idx), true);
|
|
const mani = JSON.parse(readFileSync(join(here, 'manifest.json'), 'utf8'));
|
|
check('dead lockOnLogout setting removed from manifest',
|
|
'lockOnLogout' in mani.settingsSchema, false);
|
|
check('auth:observe still declared (B-09 safety)',
|
|
mani.permissions.includes('auth:observe'), true);
|
|
|
|
console.log(`\n${fail === 0 ? 'ALL PASS' : 'FAILURES'} — ${pass} passed, ${fail} failed\n`);
|
|
process.exit(fail === 0 ? 0 : 1);
|