From 77fc48367fa1f8f5a30a88f8525db4bcfbe97a5c Mon Sep 17 00:00:00 2001 From: andy-liner Date: Mon, 1 Jun 2026 18:14:01 +0900 Subject: [PATCH] fix: make `mcpb verify`/`info` actually verify PKCS#7 signatures `verifyMcpbFile()` called node-forge's `PkcsSignedData.verify()`, which is not implemented and always throws "PKCS#7 signature verification not yet implemented". The throw was caught and mapped to `status: "unsigned"`, so `mcpb verify` and `mcpb info` reported *every* signed bundle as unsigned, regardless of how it was signed. The `"self-signed"` status was also unreachable: the OS trust-store check returned "unsigned" before it. This implements the detached PKCS#7 verification manually: - the signed `messageDigest` attribute must equal SHA-256 of the content, and - the signer signature must validate over the DER-encoded authenticated attributes (re-tagged as a SET OF). Content matching also accounts for the EOCD `comment_length` bump that `signMcpbFile()` applies *after* signing (added in #204): the stored content can differ from the signed content by those two bytes. Verification accepts a digest match against either the stored content or the comment_length-reversed content, so it works for bundles from both current and older signers (and never underflows the reversal when the comment_length was not bumped). Trust levels: OS-trusted chain -> "signed"; self-signed (issuer CN == subject CN) -> "self-signed"; valid signature but untrusted, non-self-signed chain -> "unsigned". The self-signed e2e test accepted both "self-signed" and "unsigned", so it never caught this; it now requires "self-signed". Adds a regression test for a signed bundle whose EOCD comment_length was not bumped. Related: #195 (championed verify fix, native crypto; lacks the EOCD handling needed on current main), #205, #212. Fixes #21. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/sign.ts | 139 +++++++++++++++++++++++++++++------------- test/sign.e2e.test.ts | 46 +++++++++++++- 2 files changed, 141 insertions(+), 44 deletions(-) diff --git a/src/node/sign.ts b/src/node/sign.ts index bd4c8c4..037ee23 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -137,16 +137,13 @@ export async function verifyMcpbFile( return { status: "unsigned" }; } - // Now we know it's PkcsSignedData. The types are incorrect, so we'll - // fix them there + // node-forge's TS types omit `rawCapture`, which holds the parsed signer + // fields we need for manual signature verification (see below). const p7 = p7Message as unknown as forge.pkcs7.PkcsSignedData & { - signerInfos: Array<{ - authenticatedAttributes: Array<{ - type: string; - value: unknown; - }>; - }>; - verify: (options?: { authenticatedAttributes?: boolean }) => boolean; + rawCapture: { + authenticatedAttributes?: forge.asn1.Asn1[]; + signature?: string; + }; }; // Extract certificates from PKCS#7 @@ -158,37 +155,99 @@ export async function verifyMcpbFile( // Get the signing certificate (first one) const signingCert = certificates[0]; - // Verify PKCS#7 signature - const contentBuf = forge.util.createBuffer(originalContent); + // Manually verify the PKCS#7 detached signature. + // + // node-forge does not implement `PkcsSignedData.verify()` — it throws + // "PKCS#7 signature verification not yet implemented" — so calling it would + // make every signed file report as unsigned. Instead we verify the signer + // ourselves: (1) the signed `messageDigest` attribute must equal SHA-256 of + // the content, and (2) the signature must validate over the DER-encoded + // authenticated attributes (re-tagged as a SET OF, as PKCS#7 requires). + const { authenticatedAttributes, signature: signerSignature } = + p7.rawCapture; + if (!authenticatedAttributes || !signerSignature) { + return { status: "unsigned" }; + } - try { - p7.verify({ authenticatedAttributes: true }); - - // Also verify the content matches - const signerInfos = p7.signerInfos; - const signerInfo = signerInfos?.[0]; - if (signerInfo) { - const md = forge.md.sha256.create(); - md.update(contentBuf.getBytes()); - const digest = md.digest().getBytes(); - - // Find the message digest attribute - let messageDigest = null; - for (const attr of signerInfo.authenticatedAttributes) { - if (attr.type === forge.pki.oids.messageDigest) { - messageDigest = attr.value; - break; - } - } + // (1) content digest must match the signed messageDigest attribute. + let signedMessageDigest: string | null = null; + for (const attr of authenticatedAttributes) { + const attrSeq = attr.value as forge.asn1.Asn1[]; + const attrOid = forge.asn1.derToOid(attrSeq[0].value as string); + if (attrOid === forge.pki.oids.messageDigest) { + signedMessageDigest = (attrSeq[1].value as forge.asn1.Asn1[])[0] + .value as string; + break; + } + } + if (signedMessageDigest === null) { + return { status: "unsigned" }; + } - if (!messageDigest || messageDigest !== digest) { - return { status: "unsigned" }; - } + // The signature covers the bytes before the signature block. Depending on + // the version that produced the file, `signMcpbFile()` may have bumped the + // ZIP EOCD comment_length by the signature-block length *after* signing + // (added in #204) — so the stored bytes can differ from the signed bytes by + // those two bytes. Accept a digest match against either the stored content + // or the comment_length-reversed content. + const sha256 = (buf: Buffer): string => + forge.md.sha256 + .create() + .update(buf.toString("binary")) + .digest() + .getBytes(); + + const candidates: Buffer[] = [originalContent]; + const eocdOffset = findEocdOffset(originalContent); + if (eocdOffset !== -1) { + const sigBlockLength = fileContent.length - originalContent.length; + const patchedCommentLength = originalContent.readUInt16LE( + eocdOffset + 20, + ); + if (patchedCommentLength >= sigBlockLength) { + const reversed = Buffer.from(originalContent); + reversed.writeUInt16LE( + patchedCommentLength - sigBlockLength, + eocdOffset + 20, + ); + candidates.push(reversed); } - } catch (error) { + } + + const contentMatches = candidates.some( + (buf) => sha256(buf) === signedMessageDigest, + ); + if (!contentMatches) { return { status: "unsigned" }; } + // (2) signature must validate over the authenticated attributes + const attrSet = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SET, + true, + authenticatedAttributes, + ); + const attrMd = forge.md.sha256.create(); + attrMd.update(forge.asn1.toDer(attrSet).getBytes()); + + let signatureValid = false; + try { + signatureValid = ( + signingCert.publicKey as forge.pki.rsa.PublicKey + ).verify(attrMd.digest().getBytes(), signerSignature); + } catch { + signatureValid = false; + } + if (!signatureValid) { + return { status: "unsigned" }; + } + + // The signature is cryptographically valid. Determine the trust level. + const isSelfSigned = + signingCert.issuer.getField("CN")?.value === + signingCert.subject.getField("CN")?.value; + // Convert forge certificate to PEM for OS verification const certPem = forge.pki.certificateToPem(signingCert); const intermediatePems = certificates @@ -201,18 +260,14 @@ export async function verifyMcpbFile( intermediatePems, ); - if (!chainValid) { - // Signature is valid but certificate is not trusted + // A valid signature whose chain is neither OS-trusted nor self-signed is + // reported as unsigned, since we cannot attest to the publisher identity. + if (!chainValid && !isSelfSigned) { return { status: "unsigned" }; } - // Extract certificate info - const isSelfSigned = - signingCert.issuer.getField("CN")?.value === - signingCert.subject.getField("CN")?.value; - return { - status: isSelfSigned ? "self-signed" : "signed", + status: chainValid ? "signed" : "self-signed", publisher: signingCert.subject.getField("CN")?.value || "Unknown", issuer: signingCert.issuer.getField("CN")?.value || "Unknown", valid_from: signingCert.validity.notBefore.toISOString(), diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts index 9af52a0..5b30682 100755 --- a/test/sign.e2e.test.ts +++ b/test/sign.e2e.test.ts @@ -341,8 +341,13 @@ async function testSelfSignedSigning() { // Verify the signature const result = await verifyMcpbFile(testFile); - // Self-signed certs may not be trusted by OS, so we accept either status - expect(["self-signed", "unsigned"]).toContain(result.status); + // A validly self-signed file must be detected as "self-signed" (not + // "unsigned"). This guards against the regression where verification relied + // on node-forge's pkcs7.verify() — which is unimplemented and throws — making + // every signed file appear unsigned, and left the "self-signed" status + // unreachable behind the OS trust-store check. + expect(result.status).toBe("self-signed"); + expect(result.publisher).toBe("Test MCPB Publisher"); // Clean up fs.unlinkSync(testFile); @@ -427,6 +432,39 @@ async function testSignatureRemoval() { fs.unlinkSync(testFile); } +/** + * Test a signed file whose EOCD comment_length was NOT bumped after signing + * (as produced by signers predating the comment_length bump). The stored + * content equals the signed content, so verification must match it directly — + * and must not underflow the comment_length reversal. + */ +async function testSignedFileWithUnbumpedComment() { + const testFile = path.join(TEST_DIR, "test-unbumped.dxt"); + fs.copyFileSync(TEST_MCPB, testFile); + signMcpbFile(testFile, SELF_SIGNED_CERT, SELF_SIGNED_KEY); + + // Zero the EOCD comment_length in the stored (pre-signature) content to mimic + // a signer that did not bump it. The signature was computed over the original + // content (comment_length 0), so this is the form such a signer would store. + const buf = fs.readFileSync(testFile); + const headerIndex = buf.indexOf(Buffer.from("MCPB_SIG_V1", "utf-8")); + let eocd = -1; + for (let i = headerIndex - 22; i >= 0; i--) { + if (buf.readUInt32LE(i) === 0x06054b50) { + eocd = i; + break; + } + } + expect(eocd).toBeGreaterThanOrEqual(0); + buf.writeUInt16LE(0, eocd + 20); + fs.writeFileSync(testFile, buf); + + const result = await verifyMcpbFile(testFile); + expect(result.status).toBe("self-signed"); + + fs.unlinkSync(testFile); +} + describe("MCPB Signing E2E Tests", () => { beforeAll(() => { // Ensure test directory exists @@ -469,6 +507,10 @@ describe("MCPB Signing E2E Tests", () => { await testSignatureRemoval(); }); + it("should verify a signed file whose EOCD comment_length was not bumped", async () => { + await testSignedFileWithUnbumpedComment(); + }); + it("should update EOCD comment_length after signing", async () => { const testFile = path.join(TEST_DIR, "test-eocd.mcpb"); fs.copyFileSync(TEST_MCPB, testFile);