From 937d10df1c709c8b2f5af8d9406ef735d0b3a2dc Mon Sep 17 00:00:00 2001 From: Kaoru0822 Date: Thu, 12 Nov 2026 16:33:31 -0800 Subject: [PATCH] fix(pe): enforce DER extension critical encoding (#10212) --- src/native/pe/PeCertificateAsn1.ts | 5 ++ src/native/pe/PeCertificateParser.test.ts | 4 ++ src/native/pe/PeCertificates.fixture.ts | 13 ++-- tests/acceptance/peCertificatesCli.test.ts | 70 ++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/native/pe/PeCertificateAsn1.ts b/src/native/pe/PeCertificateAsn1.ts index f86521101f..90b26652f0 100644 --- a/src/native/pe/PeCertificateAsn1.ts +++ b/src/native/pe/PeCertificateAsn1.ts @@ -442,6 +442,11 @@ const requireExtensions = (extensions: readonly Asn1BerNode[]): void => { return fail( "TBSCertificate extension is not an OID, optional BOOLEAN, and OCTET STRING.", ); + // X.690 11.1 requires DER TRUE=0xff; 11.5 omits DEFAULT FALSE. + if (critical !== undefined && critical.content[0] !== 0xff) + return fail( + "TBSCertificate extension critical must be omitted for FALSE or encoded as DER TRUE (0xff).", + ); const oid = decodeOid(extnId.content); if (seen.has(oid)) return fail(`TBSCertificate repeats extension ${oid}.`); seen.add(oid); diff --git a/src/native/pe/PeCertificateParser.test.ts b/src/native/pe/PeCertificateParser.test.ts index eed0e0b526..44531102b2 100644 --- a/src/native/pe/PeCertificateParser.test.ts +++ b/src/native/pe/PeCertificateParser.test.ts @@ -1274,6 +1274,10 @@ it("rejects malformed and repeated certificate extensions", () => { derSeq(ski, derInt(1)), derSeq(ski, derInt(1), skiValue), derSeq(ski, critical(0xff, 0xff), skiValue), + // X.690 11.1 requires DER TRUE=0xff; 11.5 omits DEFAULT FALSE. + ...[0x00, 0x01, 0x7f, 0xfe].map((value) => + derSeq(ski, critical(value), skiValue), + ), derSeq(ski, skiValue, critical(0xff)), derSeq(ski, skiValue, der(0x05, Buffer.alloc(0))), ]) diff --git a/src/native/pe/PeCertificates.fixture.ts b/src/native/pe/PeCertificates.fixture.ts index 17f95253bf..d51df8fe04 100644 --- a/src/native/pe/PeCertificates.fixture.ts +++ b/src/native/pe/PeCertificates.fixture.ts @@ -68,7 +68,9 @@ export const pkcs7SignedDataFixture = ( organization = "REA Tests", signature = octet(Buffer.from("sig")), unsignedAttributes?: readonly (readonly number[])[], - names: Partial> = {}, + fields: Partial< + Record<"issuer" | "subject" | "signerIssuer" | "extensions", Buffer> + > = {}, ): Buffer => { const serial = Buffer.from([0x11, 0x22, 0x33, 0x44]); const subject = name(commonName, organization); @@ -76,13 +78,14 @@ export const pkcs7SignedDataFixture = ( context(0, integer(2)), integer(serial), algorithm(SHA256_RSA), - names.issuer ?? subject, + fields.issuer ?? subject, seq(utc("250101000000Z"), utc("260101000000Z")), - names.subject ?? subject, + fields.subject ?? subject, seq(algorithm(RSA), bitString(Buffer.from("pubkey"))), context( 3, - seq(seq(oid([2, 5, 29, 14]), octet(Buffer.from([0x04, 0x01, 0xaa])))), + fields.extensions ?? + seq(seq(oid([2, 5, 29, 14]), octet(Buffer.from([0x04, 0x01, 0xaa])))), ), ); const certificate = seq( @@ -92,7 +95,7 @@ export const pkcs7SignedDataFixture = ( ); const signer = seq( integer(1), - seq(names.signerIssuer ?? subject, integer(serial)), + seq(fields.signerIssuer ?? subject, integer(serial)), algorithm(SHA256), algorithm(SHA256_RSA), signature, diff --git a/tests/acceptance/peCertificatesCli.test.ts b/tests/acceptance/peCertificatesCli.test.ts index 18531c68e5..9f01c5328f 100644 --- a/tests/acceptance/peCertificatesCli.test.ts +++ b/tests/acceptance/peCertificatesCli.test.ts @@ -10,9 +10,79 @@ import { pkcs7SignedDataIndefiniteEnvelopeFixture, } from "../../src/native/pe/PeCertificates.fixture.js"; import { repeatedCertificateSignedData } from "../fixtures/peCertificateAllowance.js"; +import { parseMcpToolError } from "../fixtures/mcpToolError.js"; import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; import { cliTest, runFileCli } from "../support/cli/cliFixture.js"; +cliTest.for([0x00, 0x01, 0x7f, 0xfe, 0xff, null])( + "requires canonical extension critical encoding %s through CLI and MCP", + async (critical, { cli }) => { + const root = await createTestTempDirectory("rea-pe-cert-critical-"); + const path = join(root, "signed.exe"); + // Extension ::= SEQUENCE { extnID, critical BOOLEAN DEFAULT FALSE, extnValue }. + // The extension OID and opaque value stay identical in every control. + const fields = Buffer.concat([ + Buffer.from([0x06, 0x03, 0x55, 0x1d, 0x0e]), + critical === null ? Buffer.alloc(0) : Buffer.from([0x01, 0x01, critical]), + Buffer.from([0x04, 0x03, 0x04, 0x01, 0xaa]), + ]); + const extension = Buffer.concat([ + Buffer.from([0x30, fields.length]), + fields, + ]); + const extensions = Buffer.concat([ + Buffer.from([0x30, extension.length]), + extension, + ]); + const bytes = peCertificateFixture( + false, + pkcs7SignedDataFixture(undefined, undefined, undefined, undefined, { + extensions, + }), + ); + await writeFile(path, bytes); + const result = await runFileCli(cli, { + arguments: ["inspect-pe-certificates", path, "--json"], + }); + const call = await cli.sharedMcpCall!(); + const response = await call("inspect_pe_certificates", { path }); + const valid = critical === null || critical === 0xff; + expect(result.exitCode, result.stdout).toBe(valid ? 0 : 1); + if (valid) + expect(response.isError, JSON.stringify(response)).not.toBe(true); + else expect(response.isError, JSON.stringify(response)).toBe(true); + for (const value of [ + result.json, + valid ? response.structuredContent : parseMcpToolError(response).error, + ]) { + if (valid) { + const report = toolContract( + "inspect_pe_certificates", + ).outputSchema.parse(value).normalized_result; + expect(report.coverage.status).toBe("complete"); + expect(report.entries[0]?.pkcs7?.certificates[0]?.serial_hex).toBe( + "11223344", + ); + } else { + expect(value).toMatchObject({ + code: "invalid_request", + details: { + operation: "inspect_pe_certificates", + issues: [ + expect.objectContaining({ + path: ["path"], + reason: "invalid_format", + message: expect.stringContaining("critical"), + }), + ], + }, + }); + } + } + expect((await readFile(path)).equals(bytes)).toBe(true); + }, +); + cliTest.for(["extra-field", "empty-rdn"] as const)( "rejects a malformed certificate name %s through CLI and MCP", async (kind, { cli }) => {