diff --git a/vnc/plugins/smime/src/certificate-utils.js b/vnc/plugins/smime/src/certificate-utils.js index 12288ce2..eb0a67e3 100644 --- a/vnc/plugins/smime/src/certificate-utils.js +++ b/vnc/plugins/smime/src/certificate-utils.js @@ -131,14 +131,23 @@ function extractExtendedKeyUsage(cert) { return ext.parsedValue.keyPurposes; } +// Order is load-bearing, not cosmetic. Callers identify a certificate by +// `emailAddresses[0]` — it becomes the storage key an encryption certificate is +// filed under, and the address `signerEmailMatch` compares against the `From` +// header. Under RFC 5280 / 8550 the authoritative address is the SAN +// `rfc822Name`; the Subject DN `emailAddress` attribute (OID +// 1.2.840.113549.1.9.1) is legacy and retained only for old clients, so it must +// never outrank the SAN. Collect SAN first, DN second, de-duplicated. +// +// This was the other way round until a real CA was in the picture: EJBCA +// populates BOTH fields, and the two need only differ in case or in domain form +// for the DN value to win and every signature to read as "signer ≠ From". function extractEmailAddresses(cert) { const emails = []; - - for (const tv of cert.subject.typesAndValues) { - if (tv.type === '1.2.840.113549.1.9.1') { - emails.push(tv.value.valueBlock.value); - } - } + const push = (value) => { + if (typeof value !== 'string' || !value) return; + if (!emails.some((e) => e.toLowerCase() === value.toLowerCase())) emails.push(value); + }; const sanExt = cert.extensions?.find((e) => e.extnID === OID_SAN); if (sanExt) { @@ -156,16 +165,34 @@ function extractEmailAddresses(cert) { } if (names) { for (const name of names) { - if (name.type === 1 && typeof name.value === 'string' && !emails.includes(name.value)) { - emails.push(name.value); - } + if (name.type === 1) push(name.value); } } } + for (const tv of cert.subject.typesAndValues) { + if (tv.type === '1.2.840.113549.1.9.1') push(tv.value.valueBlock.value); + } + return emails; } +/** + * True iff `address` is asserted by the certificate, comparing against EVERY + * address it carries rather than only the first. + * + * A certificate may legitimately name several addresses — an alias, a role + * mailbox, a maiden name — and there is no ordering guarantee that puts the one + * a given message was sent from at position 0. Comparing only `[0]` reports a + * genuine signer as a mismatch, which since the fix-1 auto-import gate is not + * cosmetic: it stops the encryption certificate from ever being stored. + */ +export function certAssertsAddress(emailAddresses, address) { + if (!address || !Array.isArray(emailAddresses)) return false; + const want = address.toLowerCase(); + return emailAddresses.some((e) => typeof e === 'string' && e.toLowerCase() === want); +} + /** Determine signing/encryption capabilities from KU / EKU. Tolerant of absent extensions. */ export function classifyCapabilities(cert) { const ku = extractKeyUsage(cert); diff --git a/vnc/plugins/smime/src/smime-verify.js b/vnc/plugins/smime/src/smime-verify.js index 417620bd..2908c2fd 100644 --- a/vnc/plugins/smime/src/smime-verify.js +++ b/vnc/plugins/smime/src/smime-verify.js @@ -5,7 +5,7 @@ import * as pkijs from 'pkijs'; import * as asn1js from 'asn1js'; -import { extractCertificateInfo } from './certificate-utils.js'; +import { extractCertificateInfo, certAssertsAddress } from './certificate-utils.js'; import { nativeEngine } from './crypto-engine.js'; import { arraysEqual, toHex } from './util.js'; @@ -58,7 +58,12 @@ export async function smimeVerify(cmsBytes, fromHeader) { if (certExpired && !signatureError) signatureError = 'Signer certificate has expired'; if (certNotYetValid && !signatureError) signatureError = 'Signer certificate is not yet valid'; - const signerEmail = certInfo.emailAddresses[0] ?? ''; + // File the certificate under the address the message actually came from when + // the certificate asserts it. That address is the key encryption lookups use + // later, so picking a different one of the certificate's addresses stores a + // usable certificate where nothing will ever look for it. + const asserted = certAssertsAddress(certInfo.emailAddresses, fromHeader); + const signerEmail = (asserted ? fromHeader : certInfo.emailAddresses[0]) ?? ''; const signerPublicCert = { id: `signer-${certInfo.fingerprint}`, email: signerEmail.toLowerCase(), @@ -72,8 +77,8 @@ export async function smimeVerify(cmsBytes, fromHeader) { }; let signerEmailMatch; - if (fromHeader && signerEmail) { - signerEmailMatch = fromHeader.toLowerCase() === signerEmail.toLowerCase(); + if (fromHeader && certInfo.emailAddresses.length > 0) { + signerEmailMatch = asserted; } const issuerDer = new Uint8Array(signerCert.issuer.toSchema().toBER(false)); diff --git a/vnc/plugins/smime/verify-address-binding.mjs b/vnc/plugins/smime/verify-address-binding.mjs new file mode 100644 index 00000000..c17496de --- /dev/null +++ b/vnc/plugins/smime/verify-address-binding.mjs @@ -0,0 +1,147 @@ +// Finding 11 — which address does a certificate actually bind to? +// +// node vnc/plugins/smime/verify-address-binding.mjs +// +// Self-contained: generates its own certificates with openssl, so this runs +// without the spike cert directory and without a browser. +// +// The case that matters is a certificate carrying BOTH a SAN `rfc822Name` and a +// Subject DN `emailAddress` attribute that disagree with it — which is not +// exotic, it is what EJBCA emits by default once the end-entity profile has an +// email field. Under RFC 5280/8550 the SAN is authoritative and the DN attribute +// is legacy; reading the DN one instead makes a genuine signature report as +// "signer ≠ From", and since the fix-1 auto-import gate that is not cosmetic — +// it stops the correspondent's encryption certificate from ever being stored. +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +let pass = 0, fail = 0; +const check = (name, ok, extra = '') => { + console.log(` ${ok ? 'PASS' : 'FAIL'} ${name}${extra ? ' — ' + extra : ''}`); + ok ? pass++ : fail++; +}; + +const { extractCertificateInfo, certAssertsAddress } = await import('./src/certificate-utils.js'); +const pkijs = await import('pkijs'); +const asn1js = await import('asn1js'); + +const dir = mkdtempSync(join(tmpdir(), 'smime-addr-')); +const openssl = (args, opts = {}) => execFileSync('openssl', args, { cwd: dir, ...opts }); + +// SAN carries the real address plus a second alias; the DN carries a DIFFERENT, +// stale address. A naive reader takes the DN value and mismatches on both. +const SAN_PRIMARY = 'bernd.rodler@sandbox.vnc.de'; +const SAN_ALIAS = 'br@sandbox.vnc.de'; +const DN_LEGACY = 'legacy.address@old.example'; + +writeFileSync(join(dir, 'cert.cnf'), ` +[ req ] +default_md = sha256 +prompt = no +distinguished_name = dn +x509_extensions = ext + +[ dn ] +C = CH +O = VNC AG +CN = Bernd Rodler +emailAddress = ${DN_LEGACY} + +[ ext ] +basicConstraints = critical,CA:FALSE +keyUsage = critical,digitalSignature,keyEncipherment +extendedKeyUsage = emailProtection +subjectAltName = email:${SAN_PRIMARY},email:${SAN_ALIAS} +`); + +console.log('\n0. Generate a certificate whose SAN and DN disagree'); +openssl(['genrsa', '-out', 'k.pem', '2048'], { stdio: 'ignore' }); +openssl(['req', '-new', '-x509', '-config', 'cert.cnf', '-key', 'k.pem', + '-days', '365', '-out', 'c.pem'], { stdio: 'ignore' }); +const der = openssl(['x509', '-in', 'c.pem', '-outform', 'DER']); +check('certificate generated', der.length > 300, `${der.length} bytes DER`); + +// Confirm openssl really put both forms in, otherwise the test proves nothing. +const dump = openssl(['x509', '-in', 'c.pem', '-noout', '-text']).toString(); +check('DN really carries the legacy emailAddress', dump.includes(DN_LEGACY)); +check('SAN really carries both rfc822Names', + dump.includes(SAN_PRIMARY) && dump.includes(SAN_ALIAS)); + +const abOf = (b) => b.buffer.slice(b.byteOffset, b.byteOffset + b.byteLength); +const parse = async (b) => { + const a = asn1js.fromBER(abOf(b)); + return extractCertificateInfo(new pkijs.Certificate({ schema: a.result }), abOf(b)); +}; +const info = await parse(der); + +console.log('\n1. SAN outranks the legacy DN attribute'); +check('emailAddresses[0] is the SAN address, not the DN one', + info.emailAddresses[0] === SAN_PRIMARY, info.emailAddresses[0]); +check('the legacy DN address is still retained (old-client compat)', + info.emailAddresses.includes(DN_LEGACY)); +check('no duplicates', new Set(info.emailAddresses.map((e) => e.toLowerCase())).size + === info.emailAddresses.length, info.emailAddresses.join(', ')); + +console.log('\n2. Matching considers every address, not just position 0'); +check('primary SAN address matches', certAssertsAddress(info.emailAddresses, SAN_PRIMARY)); +check('SECOND SAN alias also matches', certAssertsAddress(info.emailAddresses, SAN_ALIAS)); +check('legacy DN address also matches', certAssertsAddress(info.emailAddresses, DN_LEGACY)); +check('match is case-insensitive', + certAssertsAddress(info.emailAddresses, SAN_PRIMARY.toUpperCase())); + +console.log('\n3. It still refuses what it should'); +check('an address the cert does NOT assert is refused', + certAssertsAddress(info.emailAddresses, 'attacker@evil.example') === false); +check('empty address is refused', certAssertsAddress(info.emailAddresses, '') === false); +check('substring of a real address is refused', + certAssertsAddress(info.emailAddresses, 'sandbox.vnc.de') === false); +check('lookalike domain is refused', + certAssertsAddress(info.emailAddresses, 'bernd.rodler@sandbox.vnc.de.evil.example') === false); + +console.log('\n4. Regression — the single-address case is unaffected'); +writeFileSync(join(dir, 'simple.cnf'), ` +[ req ] +default_md = sha256 +prompt = no +distinguished_name = dn +x509_extensions = ext +[ dn ] +CN = Solo +[ ext ] +basicConstraints = critical,CA:FALSE +keyUsage = critical,digitalSignature,keyEncipherment +extendedKeyUsage = emailProtection +subjectAltName = email:${SAN_PRIMARY} +`); +openssl(['req', '-new', '-x509', '-config', 'simple.cnf', '-key', 'k.pem', + '-days', '365', '-out', 's.pem'], { stdio: 'ignore' }); +const der2 = openssl(['x509', '-in', 's.pem', '-outform', 'DER']); +const info2 = await parse(der2); +check('SAN-only cert yields exactly one address', + info2.emailAddresses.length === 1 && info2.emailAddresses[0] === SAN_PRIMARY, + info2.emailAddresses.join(', ')); +check('and it matches', certAssertsAddress(info2.emailAddresses, SAN_PRIMARY)); + +console.log('\n5. A cert with NO address asserts nothing'); +writeFileSync(join(dir, 'none.cnf'), ` +[ req ] +default_md = sha256 +prompt = no +distinguished_name = dn +[ dn ] +CN = No Address +`); +openssl(['req', '-new', '-x509', '-config', 'none.cnf', '-key', 'k.pem', + '-days', '365', '-out', 'n.pem'], { stdio: 'ignore' }); +const der3 = openssl(['x509', '-in', 'n.pem', '-outform', 'DER']); +const info3 = await parse(der3); +check('no addresses extracted', info3.emailAddresses.length === 0); +check('asserts nothing', certAssertsAddress(info3.emailAddresses, SAN_PRIMARY) === false); + +rmSync(dir, { recursive: true, force: true }); +console.log(fail === 0 + ? `\nADDRESS BINDING OK — ${pass} passed, 0 failed\n` + : `\nFAILURES — ${pass} passed, ${fail} FAILED\n`); +process.exit(fail === 0 ? 0 : 1);