Bernd RodlerandClaude Opus 4.8
f7e487171c
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>
74 lines
4.3 KiB
JavaScript
74 lines
4.3 KiB
JavaScript
// Standalone proof for the two VNC hardening fixes. Reimplements only the
|
|
// decision logic under test (no pkijs/DOM needed) so it runs with plain node.
|
|
// node vnc/plugins/smime/verify-fixes.mjs
|
|
import { readFileSync } from 'node:fs';
|
|
import { fileURLToPath } from 'node:url';
|
|
import { dirname, join } from 'node:path';
|
|
|
|
const here = dirname(fileURLToPath(import.meta.url));
|
|
let pass = 0, fail = 0;
|
|
const check = (name, got, want) => {
|
|
const ok = got === want;
|
|
console.log(`${ok ? ' PASS' : ' FAIL'} ${name}${ok ? '' : ` (got ${JSON.stringify(got)}, want ${JSON.stringify(want)})`}`);
|
|
ok ? pass++ : fail++;
|
|
};
|
|
|
|
// ── Fix 1: auto-import gate ─────────────────────────────────────────
|
|
// Mirrors the guard order in index.js maybeAutoImportSigner.
|
|
function wouldImport(status, autoImport = true) {
|
|
if (autoImport === false) return false;
|
|
const cert = status && status.signerCert;
|
|
if (!cert || !status.signatureValid || !cert.email) return false;
|
|
if (status.signerEmailMatch !== true) return false;
|
|
if (status.selfSigned) return false;
|
|
return true;
|
|
}
|
|
const cert = { email: 'a@b.com', fingerprint: 'ff' };
|
|
|
|
console.log('\nFix 1 — certificate auto-import gate');
|
|
check('CA-signed, address matches -> import',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: false }), true);
|
|
check('THE ATTACK: self-signed, address matches -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: true }), false);
|
|
check('address mismatch -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: false, selfSigned: false }), false);
|
|
check('signerEmailMatch undefined (no From) -> REFUSE (fail closed)',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: undefined, selfSigned: false }), false);
|
|
check('invalid signature -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: false, signerEmailMatch: true, selfSigned: false }), false);
|
|
check('setting off -> REFUSE',
|
|
wouldImport({ signerCert: cert, signatureValid: true, signerEmailMatch: true, selfSigned: false }, false), false);
|
|
|
|
// ── Fix 2 (finding 3): CRLF stripping ───────────────────────────────
|
|
function stripCrlf(value) {
|
|
return String(value).replace(/[\r\n]+[ \t]*/g, ' ');
|
|
}
|
|
console.log('\nFinding 3 — CRLF header sanitisation');
|
|
check('BCC injection via display name',
|
|
stripCrlf('Evil\r\nBcc: attacker@evil.com'), 'Evil Bcc: attacker@evil.com');
|
|
check('bare LF', stripCrlf('a\nb'), 'a b');
|
|
check('bare CR', stripCrlf('a\rb'), 'a b');
|
|
check('folded continuation collapsed', stripCrlf('a\r\n\tb'), 'a b');
|
|
check('multiple injected headers',
|
|
stripCrlf('x\r\nBcc: a@b.c\r\nReply-To: d@e.f'), 'x Bcc: a@b.c Reply-To: d@e.f');
|
|
check('clean value untouched', stripCrlf('Normal Subject'), 'Normal Subject');
|
|
check('non-ASCII untouched', stripCrlf('Grüße büro'), 'Grüße büro');
|
|
|
|
// ── Source assertions: guard against silent regression ──────────────
|
|
console.log('\nSource assertions');
|
|
const idx = readFileSync(join(here, 'src/index.js'), 'utf8');
|
|
const mb = readFileSync(join(here, 'src/mime-builder.js'), 'utf8');
|
|
check('index.js checks signerEmailMatch !== true', idx.includes('status.signerEmailMatch !== true'), true);
|
|
check('index.js checks selfSigned', /if \(status\.selfSigned\)/.test(idx), true);
|
|
check('formatHeader sanitises its value', /function formatHeader\(name, rawValue\)[\s\S]{0,80}stripCrlf\(rawValue\)/.test(mb), true);
|
|
check('attachment Content-Type sanitised', mb.includes('stripCrlf(att.contentType)'), true);
|
|
check('Content-ID sanitised', mb.includes('stripCrlf(att.cid)'), true);
|
|
// Every header assembled outside formatHeader must use a literal or a sanitised value.
|
|
const bypass = [...mb.matchAll(/lines\.push\(`([A-Za-z-]+): ([^`]*)`\)/g)]
|
|
.filter(([, , v]) => /\$\{/.test(v) && !/stripCrlf|encodeHeaderValue|boundary|altBoundary|disposition/.test(v));
|
|
check('no unsanitised interpolated headers remain', bypass.length, 0);
|
|
if (bypass.length) bypass.forEach(([m]) => console.log(' >>', m));
|
|
|
|
console.log(`\n${fail === 0 ? 'ALL PASS' : 'FAILURES'} — ${pass} passed, ${fail} failed\n`);
|
|
process.exit(fail === 0 ? 0 : 1);
|