fb40e747135d628d03402d18e5fd49d613b813cd
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Bernd RodlerandClaude Opus 5
|
fb40e74713 |
fix(smime): certificate address binding prefers the deprecated DN attribute
Finding 11, found while writing the EJBCA runbook rather than from a test -
and it is a blocker that fix 1 created.
extractEmailAddresses collected the Subject DN emailAddress attribute
(OID 1.2.840.113549.1.9.1) BEFORE the SAN rfc822Name, and every consumer
reads emailAddresses[0]. Under RFC 5280/8550 the SAN is authoritative and
the DN attribute is legacy, retained only for old clients - so the order
was exactly backwards. Compounding it, signerEmailMatch compared the From
header against position 0 only, never against the other addresses a
certificate legitimately carries.
Two ways a perfectly valid certificate failed:
1. DN and SAN disagree in any respect - case, domain form, a stale
value. The DN wins, From never matches.
2. A multi-alias certificate where the message was sent From the
SECOND rfc822Name. Only [0] is compared, so it mismatches.
Before fix 1 that was a cosmetic amber "signer != From" banner. After fix
1 it BLOCKS auto-import, so the correspondent's encryption certificate is
never stored and encryption silently never becomes available for them.
I turned a latent wart into a functional blocker in the same audit.
This was not hypothetical for much longer: EJBCA populates both fields by
default once the end-entity profile has an email field, which is exactly
what the CA runbook configures. The internal CA would have shipped
certificates this client mishandles on day one.
Fix:
- collect SAN rfc822Name first, DN emailAddress second, de-duplicated
case-insensitively, so [0] is the authoritative address
- add certAssertsAddress(), matching against every address the
certificate asserts rather than only the first
- file the signer certificate under the address the message actually came
from when the certificate asserts it. That address is the key used for
encryption lookups later, so storing a usable certificate under a
different one of its addresses hides it from the code that needs it.
The manual-import paths (index.js:961, pkcs12.js:114) have no From header
to match against and are corrected by the reordering alone.
Verified: new verify-address-binding.mjs, 18 assertions, self-contained -
it generates its own certificates with openssl, including one whose SAN
and DN deliberately disagree, and asserts openssl really emitted both
forms before drawing any conclusion.
Confirmed the bug was real rather than assumed, by running the same suite
against the pre-fix file restored from git with the old [0]-only matching
shimmed back in: emailAddresses[0] resolves to legacy.address@old.example
and all three match assertions fail. Every REFUSAL case still passed both
before and after, so this removes false negatives without loosening the
gate - lookalike domains, substrings and empty addresses are still
refused.
51 + 28 + 18 = 97 assertions passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Bernd RodlerandClaude Opus 4.8
|
90c1176f93 |
fix(smime): banner slot can silently miss a resolved signature
Found live, not from a test: sent a genuinely signed+encrypted message through the real composer, opened it in Sent, and the banner showed only "Encrypted message" - no signature row at all, despite both Sign and Encrypt having been checked and the body decrypting correctly. Root cause is a race, not a crypto bug. onRenderEmailBody (which fetches the blob, decrypts, verifies the inner signature, and persists the full status) and EmailBanner (a separate plugin UI slot) mount independently. The banner read persisted status exactly once, in a useEffect keyed only on email.id. If that read fired before the async decrypt+verify pipeline finished writing, the banner fell back to a header-derived guess: it can see from the OUTER envelope's Content-Type that a message is encrypted, but has no way to know it is ALSO signed, since that only becomes knowable after decryption completes. This is more than cosmetic. The same race could just as easily hide an INVALID signature - a tampered message or wrong signer - behind the generic "Encrypted message" banner, purely because of timing, with no indication anything needs attention. Fix: track whether the initial read came from a real persisted value or from the header-only fallback. Only in the fallback case, poll briefly (150ms x 20 = 3s) for the real result to land - the same pattern unlockNow already uses after a manual key unlock, generalized to the initial mount. Once persisted state exists, stop. Verified in the real browser: re-sent and re-opened the same signed+ encrypted Sent message after this fix, banner now shows both rows - "Decrypted" and "Valid signature by bernd.rodler@sandbox.vnc.de - self-signed" (amber, correctly, since the spike cert is self-signed and fix 1's selfSigned flag is doing its job). Two source assertions added to verify-fixes.mjs. 51 unit assertions, 28 round-trip assertions, all passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
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> |
||
|
Bernd RodlerandClaude Opus 4.8
|
d047891ded |
test(smime): real crypto round trip against the patched plugin
Adds roundtrip.mjs, which drives the plugin's own modules directly — no browser, no DOM — and proves the three audit fixes did not break S/MIME. 24 assertions, all passing, against the self-signed spike certificates: PKCS#12 import (both identities, RSA-2048, kdf=600000) key encrypted at rest (32-byte salt, 12-byte IV) unlock yields NON-EXTRACTABLE keys; wrong passphrase rejected sign -> verify: signature valid, signer email matches From encrypt -> decrypt by the intended recipient, plaintext matches sender can read their own Sent copy downgraded message produces no plaintext Two results worth recording. Finding 1 is confirmed against a genuine CMS structure, not just a mock: the spike certs are self-signed, smimeVerify reports signatureValid AND signerEmailMatch true AND selfSigned true, and the gate refuses the auto-import. That is exactly the cert-substitution attack, blocked. The same status with selfSigned:false passes, so the gate is not simply refusing everything. Finding 2 is confirmed end to end: our own encrypt path produces AES-256-GCM, decrypt reports contentAuthenticated:true, so HTML renders without suppression. Only legacy inbound CBC degrades to text. The section-8 assertion is deliberately loose. Swapping the 9-byte AES-GCM OID for the 8-byte 3DES OID also invalidates the enclosing DER lengths, so ASN.1 validation rejects the message before the allowlist is reached — either way no plaintext is produced, and the assertion says which path fired rather than pretending it tested the allowlist. The allowlist itself is asserted precisely in verify-fixes.mjs, which now carries 36 assertions including checks that fail if a legacy CBC OID reappears or the mail path stops using the native engine. Browser-side spike result: the patched plugin installs through the admin channel, resolves to the privileged tier, and activates with "hooks=5, slots=3" and no refusals — so the B-04 gate does not block it. Its S/MIME settings section renders and survives SPA navigation. Key import via the UI could not be automated (native file picker), which is a harness limit rather than a product defect; roundtrip.mjs covers that path directly instead. Findings 4, 5 and 6 remain open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
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> |
||
|
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> |