diff --git a/vnc/audits/SMIME-PLUGIN-AUDIT-2026-08-04.md b/vnc/audits/SMIME-PLUGIN-AUDIT-2026-08-04.md index 1488b11d..2072c0a3 100644 --- a/vnc/audits/SMIME-PLUGIN-AUDIT-2026-08-04.md +++ b/vnc/audits/SMIME-PLUGIN-AUDIT-2026-08-04.md @@ -21,8 +21,10 @@ Forked to `vnc/plugins/smime/` — **source only; the upstream zip was deliberat | 1 · Certificate substitution | ✅ **Fixed** — auto-import now requires `signerEmailMatch === true` **and** `!selfSigned` | | 3 · CRLF header injection | ✅ **Fixed** — sanitised inside `formatHeader` (covers all 17 call sites) plus the 3 headers assembled directly | | 2 · Unauthenticated CBC on decrypt | ✅ **Fixed** — content-encryption allowlist + native engine on the mail path + HTML suppressed for unauthenticated plaintext | +| 5 · Parser DoS | ✅ **Fixed** — depth (20), part (500) and size (64 MB) caps. Upstream crashes with `RangeError: Maximum call stack size exceeded` on the same input; patched code survives | +| 4 · Unlocked keys on disk | ⚠️ **Hardened, not eliminated** — `lockOnLogout` opt-out removed (a security control shouldn't be user-disableable), plus best-effort `pagehide`/`beforeunload` wipe. Keys still live in IndexedDB; the boot wipe remains load-bearing | | — · `auth:observe` | ✅ **Added** to the manifest, so the session-key wipe survives `B-09` | -| 4, 5, 6, 7, 8, 9 | ⛔ Open — see the findings table | +| 6, 7, 8, 9 | ⛔ Open — see the findings table | Regression tests: `vnc/plugins/smime/verify-fixes.mjs` — **36 assertions**, `node vnc/plugins/smime/verify-fixes.mjs`. Covers the attack case for finding 1, CRLF variants for finding 3, the algorithm allowlist and HTML-suppression decision for finding 2, plus source assertions that fail if any guard is removed or a new unsanitised interpolated header is introduced. That last assertion earned its keep immediately: it caught the interpolated `smime-type` Content-Type header (`mime-builder.js:216`), which manual review had wrongly dismissed as a static string. diff --git a/vnc/plugins/smime/manifest.json b/vnc/plugins/smime/manifest.json index 704d5462..35da9be5 100644 --- a/vnc/plugins/smime/manifest.json +++ b/vnc/plugins/smime/manifest.json @@ -38,12 +38,6 @@ "description": "When a validly signed message is opened, remember the signer's certificate so you can later send them encrypted mail without importing it manually.", "default": true }, - "lockOnLogout": { - "type": "boolean", - "label": "Lock keys on logout", - "description": "Wipe all unlocked private keys from memory when you sign out or switch accounts. Leave on unless you have a specific reason not to.", - "default": true - }, "renderUnauthenticatedHtml": { "type": "boolean", "label": "Render HTML in legacy-encrypted mail", diff --git a/vnc/plugins/smime/roundtrip.mjs b/vnc/plugins/smime/roundtrip.mjs index 07ed3837..eb9881cd 100644 --- a/vnc/plugins/smime/roundtrip.mjs +++ b/vnc/plugins/smime/roundtrip.mjs @@ -142,5 +142,48 @@ if (at < 0) { /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

hello html

\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); diff --git a/vnc/plugins/smime/src/index.js b/vnc/plugins/smime/src/index.js index 5f782e2a..04b14776 100644 --- a/vnc/plugins/smime/src/index.js +++ b/vnc/plugins/smime/src/index.js @@ -1082,12 +1082,17 @@ export const hooks = { onComposeSend, onRenderEmailBody, // Wipe unlocked keys from the shared session store on sign-out / account switch. + // + // VNC (audit finding 4): the `lockOnLogout` opt-out was removed. Unlocked keys + // live in DURABLE IndexedDB, not memory, so this wipe is the only thing that + // stops a usable key handle outliving the session on disk. A non-extractable + // key can't be exported but can still be USED — anyone with the browser + // profile could decrypt mail without ever knowing the passphrase. That is not + // a preference to be toggled off. async onAfterLogout() { - if (settings().lockOnLogout === false) return; try { await clearSessionKeys(); } catch (err) { host.log.warn('clearSessionKeys failed', err); } }, async onAccountSwitch() { - if (settings().lockOnLogout === false) return; try { await clearSessionKeys(); } catch (err) { host.log.warn('clearSessionKeys failed', err); } }, }; @@ -1108,7 +1113,27 @@ export async function activate(api) { } // Enforce session scope for unlocked keys: wipe any left over from a prior // app session at boot (mirrors the native "in-memory, cleared on reload"). + // + // VNC (audit finding 4): this boot wipe is the load-bearing one. Because + // unlocked handles sit in durable IndexedDB rather than memory, it is what + // guarantees a handle surviving a crash or force-quit is destroyed before + // anything can use it. try { await clearSessionKeys(); } catch (err) { api.log.warn('S/MIME: clearSessionKeys failed', err); } + + // VNC (audit finding 4): also wipe on the way out, to narrow the window in + // which a usable handle exists on disk at all. Best-effort by nature — an + // IndexedDB write may not complete during teardown, and neither event fires on + // a crash — which is exactly why the boot wipe above still has to exist. + // + // `pagehide` is used alongside `beforeunload` because Safari and mobile + // browsers often skip the latter. 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. + const wipeOnExit = () => { try { clearSessionKeys(); } catch { /* teardown, best effort */ } }; + try { + window.addEventListener('pagehide', wipeOnExit); + window.addEventListener('beforeunload', wipeOnExit); + } catch { /* no window (non-browser test context) */ } let keyCount = 0; try { keyCount = (await listKeyRecords()).length; } catch (err) { api.log.warn('S/MIME: listKeyRecords failed', err); } api.log.info(`S/MIME plugin activated (${keyCount} key${keyCount === 1 ? '' : 's'} imported)`); diff --git a/vnc/plugins/smime/src/mime-parse.js b/vnc/plugins/smime/src/mime-parse.js index 4ea75157..91027a75 100644 --- a/vnc/plugins/smime/src/mime-parse.js +++ b/vnc/plugins/smime/src/mime-parse.js @@ -7,10 +7,32 @@ const decoder = new TextDecoder('utf-8', { fatal: false }); +// ─── VNC: resource limits (audit finding 5) ──────────────────────────── +// +// This parser runs on attacker-controlled input: the inner MIME recovered after +// decrypt/verify is whatever the sender put there. Upstream had no depth limit +// on nested multiparts and no size cap anywhere, so a crafted message could +// blow the stack or exhaust memory — a decrypt-time DoS reachable by anyone who +// can send you mail. +// +// Limits are generous enough that no legitimate message hits them: real mail +// nests maybe 3-4 levels (mixed > alternative > related), and 64 MB is far above +// any sane attachment set surviving base64 in a single message. +const MAX_DEPTH = 20; +const MAX_PARTS = 500; +const MAX_BYTES = 64 * 1024 * 1024; + /** Parse raw inner MIME bytes into { html, text, attachments }. */ export function parseMime(bytes) { + if (bytes.length > MAX_BYTES) { + // Refuse rather than truncate: half a MIME tree parses into misleading + // nonsense, and silently showing part of a message is worse than saying no. + throw new Error( + `Refusing to parse: message exceeds ${Math.round(MAX_BYTES / 1024 / 1024)} MB`, + ); + } const text = binaryString(bytes); - const node = parseEntity(text); + const node = parseEntity(text, 0, { parts: 0 }); const out = { html: '', text: '', attachments: [] }; collect(node, out); // Fallback for non-MIME inner content (e.g. messages signed/encrypted by @@ -31,7 +53,11 @@ function binaryString(bytes) { return s; } -function parseEntity(raw) { +// VNC: `depth` and the shared `budget` bound the recursion (finding 5). Past +// either limit the node is returned as a leaf rather than throwing, so a +// pathological subtree degrades to "not rendered" instead of failing the whole +// message — the parts above it are still legitimate and worth showing. +function parseEntity(raw, depth = 0, budget = { parts: 0 }) { const sepMatch = raw.match(/\r?\n\r?\n/); const headerText = sepMatch ? raw.slice(0, sepMatch.index) : raw; const body = sepMatch ? raw.slice(sepMatch.index + sepMatch[0].length) : ''; @@ -44,8 +70,12 @@ function parseEntity(raw) { const node = { type, params, cte, disposition, headers, body, children: [] }; - if (type.startsWith('multipart/') && params.boundary) { - node.children = splitMultipart(body, params.boundary).map(parseEntity); + if (type.startsWith('multipart/') && params.boundary && depth < MAX_DEPTH) { + for (const seg of splitMultipart(body, params.boundary)) { + if (budget.parts >= MAX_PARTS) break; + budget.parts += 1; + node.children.push(parseEntity(seg, depth + 1, budget)); + } } return node; } diff --git a/vnc/plugins/smime/src/smime-detect.js b/vnc/plugins/smime/src/smime-detect.js index a92f0a80..d8cc9944 100644 --- a/vnc/plugins/smime/src/smime-detect.js +++ b/vnc/plugins/smime/src/smime-detect.js @@ -3,6 +3,11 @@ * Checks Content-Type, JMAP bodyStructure, and attachment metadata. */ +// VNC (audit finding 5): cap for the two bodyStructure walkers below. No real +// message nests anywhere near this; a crafted one could otherwise recurse until +// the stack gives out, before any decrypt/verify gate has run. +const MAX_WALK_DEPTH = 20; + export function detectSmime(contentType, bodyStructure, attachments) { const noResult = { type: null, supported: false }; @@ -66,7 +71,7 @@ export function detectSmime(contentType, bodyStructure, attachments) { return noResult; } -function walkBodyStructure(part) { +function walkBodyStructure(part, depth = 0) { const type = part.type?.toLowerCase() || ''; if (type.includes('application/pkcs7-mime') || type.includes('application/x-pkcs7-mime')) { @@ -85,9 +90,12 @@ function walkBodyStructure(part) { } } - if (part.subParts) { + // VNC (finding 5): bound the walk. bodyStructure comes from the JMAP server, + // but a deeply-nested structure — hostile, or just a server that parsed a + // crafted message loosely — reaches here BEFORE any decrypt/verify gate. + if (part.subParts && depth < MAX_WALK_DEPTH) { for (const sub of part.subParts) { - const result = walkBodyStructure(sub); + const result = walkBodyStructure(sub, depth + 1); if (result) return result; } } @@ -95,15 +103,15 @@ function walkBodyStructure(part) { return null; } -function findCmsPart(bodyStructure, _smimeType) { +function findCmsPart(bodyStructure, _smimeType, depth = 0) { if (!bodyStructure) return null; const type = bodyStructure.type?.toLowerCase() || ''; if (type.includes('application/pkcs7-mime') || type.includes('application/x-pkcs7-mime')) { return bodyStructure; } - if (bodyStructure.subParts) { + if (bodyStructure.subParts && depth < MAX_WALK_DEPTH) { for (const sub of bodyStructure.subParts) { - const found = findCmsPart(sub, _smimeType); + const found = findCmsPart(sub, _smimeType, depth + 1); if (found) return found; } } diff --git a/vnc/plugins/smime/verify-fixes.mjs b/vnc/plugins/smime/verify-fixes.mjs index c8258627..c987831f 100644 --- a/vnc/plugins/smime/verify-fixes.mjs +++ b/vnc/plugins/smime/verify-fixes.mjs @@ -82,6 +82,21 @@ 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'); @@ -110,5 +125,30 @@ check('liner engine reachable only via useLiner', 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);