Bernd RodlerandClaude Opus 4.8
bc5d2a57e8
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>
180 lines
15 KiB
Markdown
180 lines
15 KiB
Markdown
# S/MIME plugin security audit — `bulwarkmail/plugins/smime`
|
||
|
||
**Task:** `S-01` · **Date:** 2026-08-04 · **Auditor:** Claude Opus 4.8 (findings independently verified against host source)
|
||
**Subject:** `github.com/bulwarkmail/plugins` @ `91085a3`, `smime/` — 2,935 lines of source across 13 modules
|
||
**Runs as:** privileged (same-origin) tier — full DOM, network, IndexedDB and WebCrypto access; handles users' private keys
|
||
|
||
## Verdict
|
||
|
||
**Do not ship as-is. Fork and fix findings 1–3 first.**
|
||
|
||
This is not malicious code and shows no sign of a backdoor. The cryptographic primitives are handled competently — PBKDF2-SHA256 at 600k iterations, AES-256-GCM wrapping, every private-key import non-extractable, no network egress anywhere in the bundle. The problems are **trust-model and input-validation gaps**, all fixable, two of them in a handful of lines.
|
||
|
||
The single most reassuring property: **there is no exfiltration path.** Across all 13 modules there is no `fetch`, `XMLHttpRequest`, `WebSocket`, `sendBeacon`, `new Image`, `.src =`, `EventSource`, dynamic `import()`, or URL literal of any kind. This matters more than usual because privileged tier is same-origin — the manifest's lack of `http:fetch` would *not* have constrained it, so the absence had to be verified in code rather than inferred from permissions.
|
||
|
||
## Remediation status (updated 2026-08-04)
|
||
|
||
Forked to `vnc/plugins/smime/` — **source only; the upstream zip was deliberately not vendored** (see Supply chain below).
|
||
|
||
| # | Status |
|
||
|---|---|
|
||
| 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 |
|
||
| — · `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 |
|
||
|
||
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.
|
||
|
||
### How finding 2 was fixed, and why not the obvious way
|
||
|
||
The tempting fix — accept only AEAD — would have broken 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, refuse everything else** (`smime-decrypt.js`). CBC stays for interop; DES-CBC (56-bit), 3DES-CBC and RC2-CBC are refused. The gate runs *before any private key is touched*.
|
||
2. **Take the mail path off the legacy engine.** Those weak OIDs are registered in `crypto-engine.js` for PKCS#12 *password-based* encryption; the CMS content path merely reused the same engine and inherited them. Normal decryption now uses `nativeEngine()`, and the liner engine is reachable only when a legacy RSAES-PKCS1-v1_5 key is genuinely in play. This removes the weak ciphers **structurally**, not just by policy.
|
||
3. **Refuse to render unauthenticated plaintext as HTML.** CBC output is malleable, and HTML rendering is EFAIL's exfiltration channel. The host does block remote content by default (`allowExternalContent` starts `false`, `email-viewer.tsx:780`) — but that is a user/admin setting the plugin cannot observe, so we don't lean on it. `renderUnauthenticatedHtml` (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 mail degrades to text.
|
||
|
||
### Build provenance
|
||
|
||
Built from the forked source with the repo's own pipeline (`npm run build` → esbuild → `dist/index.js`, 1.69 MB) and packaged to `smime-vnc.zip` (0.27 MB, well under the 5 MB ceiling). All four fixes verified present in the built bundle.
|
||
|
||
Worth noting against an earlier assumption: **this bundle does not trip the `B-01` pattern scanner** — zero matches on all five patterns. `B-01` remains correct (it closed a real entrypoint-only coverage gap, and openpgp.js for the PGP plugin may yet need the override) but it is not required to install this plugin.
|
||
|
||
**Remaining risk:** findings 4 (unlocked keys persisted to IndexedDB), 5 (parser DoS) and 6 (PKCS1v1.5 oracle surface) are open. Suitable for sandbox use; findings 4 and 5 should be closed before real mailboxes.
|
||
|
||
## Findings
|
||
|
||
| # | Severity | Finding | Location |
|
||
|---|---|---|---|
|
||
| 1 | **HIGH** | Certificate substitution via auto-import of unvalidated self-signed certs | `index.js:361-383` |
|
||
| 2 | **HIGH** | Unauthenticated CBC ciphers accepted on decrypt (EFAIL precondition) | `smime-decrypt.js:258-269`, `crypto-engine.js:114-135` |
|
||
| 3 | **MED-HIGH** | CRLF header injection — escaping covers 2 of ~6 insertion points | `mime-builder.js:115-139` |
|
||
| 4 | **MEDIUM** | Unlocked key handles persisted to durable IndexedDB, not memory | `key-storage.js:106-109` |
|
||
| 5 | **MEDIUM** | MIME parser: unbounded recursion + no size caps (DoS) | `mime-parse.js:47-50`, `smime-detect.js:88-109` |
|
||
| 6 | **MEDIUM** | `RSAES-PKCS1-v1_5` decrypt via JS polyfill (Marvin-class oracle surface) | `pkcs12.js:196-201`, `smime-decrypt.js:51-65` |
|
||
| 7 | LOW | Signer-cert lookup falls back to a blind heuristic for SKI-addressed CMS | `smime-verify.js:158-161` |
|
||
| 8 | LOW | `classifyCapabilities` defaults `canSign`/`canEncrypt` to true when KU/EKU absent | `certificate-utils.js:170-191` |
|
||
| 9 | LOW | PKCS#12 passphrase via `charCodeAt` — mangles non-Latin1 passphrases | `pkcs12.js:18-23` |
|
||
|
||
### 1. Certificate substitution — HIGH
|
||
|
||
`smime-verify.js:44` verifies with **`checkChain: false`** — cryptographic signature only, no trust anchor. This is a defensible design choice, and the module correctly computes both `selfSigned` (line 81) and `signerEmailMatch` (lines 74-77), returning them for the UI banner.
|
||
|
||
**But `maybeAutoImportSigner` ignores both:**
|
||
|
||
```js
|
||
// index.js:361-364
|
||
async function maybeAutoImportSigner(status) {
|
||
if (settings().autoImportSignerCerts === false) return;
|
||
const cert = status && status.signerCert;
|
||
if (!cert || !status.signatureValid || !cert.email) return;
|
||
```
|
||
|
||
It then calls `savePublicCert(...)`, storing the certificate as the **encryption target** for the email address the certificate claims. `autoImportSignerCerts` defaults to **true**.
|
||
|
||
`certificate-utils.js:134-167` collects the identity email from either the legacy Subject `E=` attribute or the SAN `rfc822Name`, treating both as equally authoritative — and for a self-signed cert both are entirely self-asserted.
|
||
|
||
**Attack:** generate a self-signed certificate asserting `victim@vnc.biz`, sign any message with it, send it to the user. On open, the signature verifies (it is internally consistent), the cert is silently stored as the encryption key for `victim@vnc.biz`. A later user-initiated "Encrypt" to that address encrypts to the attacker's key instead of the recipient's. The user sees an encrypted-send confirmation; the legitimate recipient cannot read it, and anyone holding the attacker's key who obtains the ciphertext can.
|
||
|
||
**Fix (small — the data is already computed):** require `!selfSigned` **and** `signerEmailMatch === true` before auto-import; once `S-06`'s trust store exists, require chain validation instead. Ideally require explicit user confirmation before a certificate becomes an encryption target.
|
||
|
||
### 2. Unauthenticated CBC on decrypt — HIGH
|
||
|
||
**There is no content-encryption-algorithm allowlist anywhere in the decrypt path** (verified by exhaustive grep — no reference to `contentEncryptionAlgorithm`, `GCM`, or `CBC` in `smime-decrypt.js`). The decrypt call passes attacker-supplied CMS straight to pkijs:
|
||
|
||
```js
|
||
// smime-decrypt.js:262-263
|
||
return withLinerEngine(async () => {
|
||
const cryptoEngine = getLinerCryptoEngine();
|
||
```
|
||
|
||
And that engine deliberately widens the accepted set to legacy unauthenticated ciphers:
|
||
|
||
```js
|
||
// crypto-engine.js:121-123
|
||
case OID_DES_EDE3_CBC: return { name: 'DES-EDE3-CBC', length: 192 };
|
||
case OID_DES_CBC: return { name: 'DES-CBC', length: 64 };
|
||
case OID_RC2_CBC: return { name: 'RC2-CBC', length: 128 };
|
||
```
|
||
|
||
CMS `EnvelopedData` carries no MAC; only AEAD modes provide integrity. Native AES-CBC is likewise accepted, since nothing inspects the algorithm at all. Decrypted bytes are returned to the renderer with no authenticity gate — the precondition for EFAIL direct-exfiltration and CBC-gadget attacks. End-to-end exploitability additionally depends on the host's HTML sanitiser blocking external resource loads, which is a separate control and should not be the only one.
|
||
|
||
**Fix:** allowlist AEAD content encryption (AES-GCM) on decrypt. If legacy CBC must be supported for old archived mail, gate it behind an explicit per-message user opt-in and never render its output as HTML.
|
||
|
||
### 3. CRLF header injection — MED-HIGH
|
||
|
||
`encodeHeaderValue` (`mime-builder.js:141-152`) does neutralise CR/LF — but only as a side effect of Q-encoding, and it is applied to just **Subject** and **attachment filename**. It is not applied to display names, raw addresses, `Message-ID`, `In-Reply-To`, `References`, or attachment `Content-Type`.
|
||
|
||
```js
|
||
// mime-builder.js:115-121 — escapes only backslash and quote
|
||
function formatAddress(addr) {
|
||
if (addr.name) {
|
||
const escaped = addr.name.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
|
||
return `"${escaped}" <${addr.email}>`;
|
||
}
|
||
return addr.email;
|
||
}
|
||
```
|
||
|
||
`formatHeader` (123-139) performs line *folding* only — no sanitisation. A display name containing `\r\n` is emitted verbatim into the header block.
|
||
|
||
**Remote path:** `In-Reply-To` / `References` / display names are typically copied from an inbound message when replying or forwarding, so the injected value is attacker-supplied. Impact ranges from spoofed `Reply-To` to added recipients, depending on whether the submission path passes explicit `envelopeRecipients` (the host's `sendRawEmail` accepts them; whether the plugin supplies them should be confirmed).
|
||
|
||
**Fix:** strip or encode CR/LF in one place — inside `formatHeader` — so every header value is covered by construction rather than per-call-site.
|
||
|
||
### 4. Unlocked keys persisted to disk — MEDIUM
|
||
|
||
`key-storage.js:106-109` writes unlocked, non-extractable `CryptoKey` handles into the durable `smime-plugin-store` IndexedDB. Raw key material is never exposed (non-extractable survives structured clone), but the *usable handle* is on disk and survives tab close and browser restart unless wiped.
|
||
|
||
The wipe hooks are real — `onAfterLogout` and `onAccountSwitch` exist in our host at `lib/plugin-hooks.ts:362-363` (an earlier reviewer flagged these as possibly fictional; they are not). Two caveats stand:
|
||
|
||
- The `lockOnLogout` setting (default true) lets a user disable the wipe entirely.
|
||
- If the wipe does not run — crash, killed tab, failed transaction — the handle persists, and anyone with the browser profile can decrypt mail **without the passphrase**.
|
||
|
||
This also contradicts the host's own plugin guidance in `WRITING_PLUGINS.md` §8 ("in memory only"), and it is weaker than the native implementation this replaced, which kept unlocked keys in an in-memory `Map`. The trade buys cross-iframe sharing between the settings slot and the background hooks.
|
||
|
||
### 5–9
|
||
|
||
Recursion in `parseEntity` (`mime-parse.js:47-50`) and both `smime-detect.js` walkers (88-109) is depth-unbounded; `binaryString` (`mime-parse.js:28-32`) concatenates byte-by-byte with no size cap. ReDoS specifically was checked across every regex and is **clean** — all linear. Finding 6 is an accepted-risk interop trade needing documentation rather than removal. Findings 7–9 are correctness/robustness issues.
|
||
|
||
## Verified clean
|
||
|
||
| Area | Result |
|
||
|---|---|
|
||
| Network egress | **None** across all 13 modules |
|
||
| `eval` / `new Function` / dynamic code | None |
|
||
| XSS / DOM injection | No `innerHTML`, `document.write`, or `dangerouslySetInnerHTML`; cert fields render as React text children (auto-escaped) |
|
||
| Passphrase handling | Local scope only; never persisted to storage, `api.storage`, cookies, or logs |
|
||
| `api.storage.*` (plaintext localStorage) | Non-secret prefs and verification metadata only — no keys, no passphrases |
|
||
| Key wrapping | PBKDF2-SHA256 600k → AES-256-GCM; 32-byte salt, 12-byte IV |
|
||
| Key extractability | `extractable: false` at all five `importKey` sites |
|
||
| Encrypt path | Hardcoded AES-GCM + RSA-OAEP/SHA-256 — no downgrade negotiation, no RC2/DES reachable |
|
||
| Polyfill scoping | `sign`/`encrypt`/`verify` use `nativeEngine()` exclusively; the JS polyfill is never used for signing or randomness |
|
||
| Engine restore | `withLinerEngine` uses `try/finally` — no leaked global polyfill state |
|
||
| Fingerprints | SHA-256 (not SHA-1) |
|
||
| Backdoors / hardcoded keys / TODO markers | None |
|
||
|
||
## Supply-chain findings
|
||
|
||
**Do not ship the marketplace zip.** `smime.zip` in the repo contains a single 1.77 MB bundled `index.js` — not readable source — and its manifest reads **1.0.1** while the repo source is **1.0.2**. The shipped artifact is stale relative to the code, so auditing `src/` would not audit what that zip installs.
|
||
|
||
**Build from source ourselves** via the repo's own `npm run package` (esbuild bundle of `src/` plus pkijs/asn1js/pvtsutils/webcrypto-liner). That way the artifact corresponds to the audited code and we control it.
|
||
|
||
## Interaction with our own changes
|
||
|
||
- **`B-04` is compatible.** The plugin declares `email:render-takeover` and `email:send`, which our gate requires for `onRenderEmailBody` and `onComposeSend`. It will load correctly.
|
||
- **`B-09` carries a real hazard here.** The plugin registers `onAfterLogout` and `onAccountSwitch` but its manifest requests **no `auth:observe`**. If `B-09` gates auth hooks on that permission, **the session-key wipe silently stops running** — turning finding 4 from a caveat into a live exposure. This is a concrete instance of the over-gating risk noted in `B-09`, and it is security-relevant: add the permission to the fork's manifest, or exempt these hooks deliberately.
|
||
- **`B-01` is exercised by this bundle** — 1.77 MB of minified pkijs/webcrypto-liner will trip the pattern scanner, which is exactly the case the override exists for.
|
||
|
||
## Recommendation
|
||
|
||
1. Fork `smime/` into `vnc/plugins/smime/` — **do not** vendor the upstream zip.
|
||
2. Fix findings **1, 2, 3** before any user imports a key. Findings 1 and 3 are small, localised changes; finding 2 is an allowlist.
|
||
3. Add `auth:observe` to the forked manifest so the wipe survives `B-09`.
|
||
4. Document findings 4 and 6 as accepted risks with rationale, or fix 4 by moving session keys back to memory and accepting the cross-iframe cost.
|
||
5. Re-audit the diff after fixes, then proceed to the self-signed-certificate spike.
|
||
|
||
Findings 1–3 are why `S-01` was scheduled before `A-01`. All three would have shipped.
|