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>

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.
 

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",

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<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);

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)`);

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) {
+  if (type.startsWith('multipart/') && params.boundary && depth < MAX_DEPTH) {
-    node.children = splitMultipart(body, params.boundary).map(parseEntity);
+    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;
 }

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;
     }
   }

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);