Commit Graph
4 Commits
Author SHA1 Message Date
Bernd RodlerandClaude Opus 4.8 5c1f14fa8b docs(smime): record UI key-import verification
User manually imported bernd.rodler.p12 through the real Settings >
S/MIME > Import key dialog on localhost:3100 - real native file picker,
real PKCS#12 passphrase, real storage passphrase. Succeeded.

This closes the last unverified layer. Every step of the delivery path
is now proven end to end: crypto correctness, parser hardening against
hostile input, admin install, client activation under the B-04 gate,
and now UI key import.

Also fixes a stale line in the audit doc that still listed finding 5
as open after it was fixed in a4155aa3.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-04 12:09:23 +02:00
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
Bernd RodlerandClaude Opus 4.8 bc5d2a57e8 security(smime): fix audit finding 2 — unauthenticated CBC on decrypt
Upstream applied no content-encryption check at all on decrypt, and ran
every decryption through the liner engine — which registers DES-CBC,
3DES-CBC and RC2-CBC. Those OIDs exist in crypto-engine.js for PKCS#12
password-based encryption; the CMS content path merely reused the same
engine and inherited them. A crafted message could therefore be decrypted
under a broken cipher, and unauthenticated plaintext was handed straight
to the renderer — the EFAIL precondition.

The obvious fix would have been wrong. Accepting only AEAD breaks most
real S/MIME mail: RFC 5751 makes AES-128-CBC the MUST-implement content
cipher, Outlook and Thunderbird default to CBC, and AES-GCM in CMS
(RFC 5084) is barely deployed. An AEAD-only allowlist is a functionality
catastrophe wearing a security fix's clothes.

Three layers instead:

1. Allowlist the AES family and refuse everything else, with the gate
   running before any private key is touched. CBC stays for interop;
   DES/3DES/RC2 are refused.

2. Take the mail path off the legacy engine. Normal decryption now uses
   nativeEngine(); the liner engine is reachable only when a genuine
   legacy RSAES-PKCS1-v1_5 key is in play. This removes the weak ciphers
   structurally rather than by policy — native WebCrypto handles RSA-OAEP
   key transport and AES-CBC/GCM content perfectly well.

3. Refuse to render unauthenticated plaintext as HTML. CBC output is
   malleable and HTML is EFAIL's exfiltration channel. The host does block
   remote content by default (allowExternalContent starts false), but that
   is a user/admin setting this plugin cannot observe, so we don't lean on
   it. New renderUnauthenticatedHtml setting (default false) is the
   documented opt-out. Our own encrypt path always uses AES-GCM, so mail
   we send renders fully; only legacy inbound CBC degrades to text.

Built from source with the repo's own pipeline (esbuild, 1.69 MB) and
packaged to smime-vnc.zip (0.27 MB). All four fixes verified present in
the built bundle. Build output is gitignored — never vendor a prebuilt
bundle, which was the upstream mistake.

Correcting an earlier assumption: this bundle does NOT trip the B-01
pattern scanner (zero matches on all five patterns), so the override is
not needed to install it. B-01 remains correct — it closed a real
entrypoint-only coverage gap — but it isn't load-bearing here.

verify-fixes.mjs now carries 36 assertions covering all three fixes,
including source checks that fail if a guard is removed, if a legacy CBC
OID reappears in the allowlist, or if the mail path stops using the
native engine.

Findings 4 (unlocked keys persisted to IndexedDB), 5 (parser DoS) and 6
(PKCS1v1.5 oracle surface) remain open.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-04 10:47:43 +02:00
Bernd RodlerandClaude Opus 4.8 f7e487171c security(smime): fork upstream plugin and fix two audit findings
S-01 audited bulwarkmail/plugins/smime @ 91085a3 (2,935 lines). Nine
findings, two HIGH. No backdoor and no exfiltration path anywhere in the
bundle — the problems are trust-model and input-validation gaps. Full
report in vnc/audits/SMIME-PLUGIN-AUDIT-2026-08-04.md.

Fork is source-only. The upstream smime.zip is a 1.77 MB prebuilt bundle
whose manifest reads 1.0.1 while the source reads 1.0.2, so auditing
src/ would not audit what that zip installs. We build from source.

Finding 1 (HIGH) — certificate substitution. maybeAutoImportSigner gated
on signatureValid alone, but smimeVerify runs checkChain:false, so that
only proves "signed by whoever holds this key", not that the claimed
identity is real. Self-sign a cert asserting victim@example.com, send one
signed message, and it was stored as the encryption target for that
address — the user's next Encrypt to the victim went to the attacker.
Now requires signerEmailMatch === true and !selfSigned. Both values were
already computed and displayed as untrusted in the banner; only the
import path ignored them. Tests for `true` explicitly so an undefined
match (missing From header) fails closed.

Finding 3 (MED-HIGH) — CRLF header injection. Escaping reached only
Subject and attachment filename; display names, raw addresses,
Message-ID, In-Reply-To, References and attachment Content-Type were
emitted verbatim, and formatAddress escapes only backslash and quote.
In-Reply-To/References/display names are copied from inbound mail when
replying or forwarding, so the value is attacker-supplied. Sanitising
inside formatHeader covers all 17 call sites by construction; the three
headers assembled directly get stripCrlf explicitly.

Also adds auth:observe to the manifest. The plugin registers
onAfterLogout/onAccountSwitch — real hooks (lib/plugin-hooks.ts:362-363)
— without declaring the permission, so under B-09 the session-key wipe
would silently stop running.

verify-fixes.mjs carries 19 assertions including source checks that fail
if either guard is removed or a new unsanitised interpolated header
appears. That last one immediately caught the interpolated smime-type
Content-Type header, which manual review had dismissed as static.

Finding 2 (unauthenticated CBC accepted on decrypt) is NOT fixed. This
is not safe for real mail yet — sandbox accounts only.

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