206 lines
19 KiB
Markdown
206 lines
19 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 |
|
||
| 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` |
|
||
| 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:** finding 4 is hardened but not eliminated (unlocked keys still persist to IndexedDB, not memory — see the remediation table above), findings 6/7/8/9 are open. Finding 5 (parser DoS) is fixed. Suitable for sandbox use.
|
||
|
||
### End-to-end verification (2026-08-04)
|
||
|
||
Every layer of the delivery path has now been exercised at least once:
|
||
|
||
| Layer | How verified |
|
||
|---|---|
|
||
| Crypto correctness | `roundtrip.mjs`, 28 assertions — real PKCS#12 → unlock → sign → verify → encrypt → decrypt against genuine self-signed certs |
|
||
| Parser hardening | Same suite, hostile MIME (5000-level nesting, 5000 siblings, 64 MB) fed to the real parser; confirmed the unpatched upstream parser crashes on identical input |
|
||
| Admin install path | Bundle uploaded via `POST /api/admin/plugins`, no scanner rejection, registry shows `tier=privileged`, Ed25519-signed |
|
||
| Client activation | `managed:true`, `adminApproved:true`, tier resolves to privileged, iframe spawns same-origin, `hooks=5, slots=3` with **zero refusals** under the `B-04` gate |
|
||
| **UI key import** | **User-performed**, real browser, real native file picker: `bernd.rodler.p12` imported via Settings → S/MIME → Import key, decrypted with the correct PKCS#12 passphrase (`spike`) and re-wrapped under a user-chosen storage passphrase. Succeeded. |
|
||
|
||
One near-miss during that last step, worth a minor UX note rather than a security finding: the PKCS#12-file-passphrase field has a "leave blank if the file has none" placeholder and no required marker, while the storage-passphrase field is marked required. The first attempt left the file passphrase blank and got pkijs's literal MAC-verification error — *"Integrity for the PKCS#12 data is broken!"* — which is the **correct** response to a wrong passphrase, not a bug. Most real CA-issued `.p12` files do have a passphrase, so the placeholder invites the mistake. Worth revisiting the copy in a later UI pass; out of scope for this audit.
|
||
|
||
### Finding 10 — banner slot can silently miss a resolved signature (found live, fixed)
|
||
|
||
The end-to-end send/receive test surfaced a real bug the crypto-level round trip could not have caught: sent a genuinely **signed+encrypted** message through the real composer, opened it in Sent, and the banner showed only *"Encrypted message"* — no signature row — despite both checkboxes being on and the body decrypting correctly.
|
||
|
||
**Root cause — a race, not a crypto bug.** `onRenderEmailBody` (decrypts, verifies, persists the full status) and `EmailBanner` (a separate plugin UI slot) mount independently. The banner read persisted status **exactly once**, on mount. If that read fired before the async decrypt+verify pipeline finished writing, it fell back to a header-derived guess that can see the message is encrypted but has no way to know it's *also* signed — that's only knowable after decryption.
|
||
|
||
**Why this is more than cosmetic:** the same race could just as easily hide an **invalid** signature — a tampered message, wrong signer — behind the generic "Encrypted message" banner, purely on timing, with no visual indication anything needs attention.
|
||
|
||
**Fixed:** the initial read now distinguishes a real persisted value from the header-only fallback, and only in the fallback case polls briefly (150ms × 20 = 3s) for the real result — the same pattern `unlockNow` already used after a manual key unlock, generalized to the initial mount. Verified live: re-opened the same Sent message after the fix, banner now shows both rows correctly ("Decrypted" + "Valid signature by bernd.rodler@sandbox.vnc.de · self-signed").
|
||
|
||
## 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.
|