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).

It also reverses the EOCD `comment_length` patch that `signMcpbFile()` applies
*after* signing (added in #204): the stored content differs from the signed
content by those two bytes, so verification must restore them before hashing
or the digest never matches for bundles produced by `mcpb sign`.

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".

Related: #195 (championed verify fix, native crypto; lacks the EOCD reversal
needed on current main), #205, #212. Fixes #21.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: andy-liner

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,84 @@ 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.
+    //
+    // signMcpbFile() increases the ZIP EOCD comment_length by the signature
+    // block length *after* computing the signature, so the stored content
+    // differs from what was signed by those two bytes. Reverse that patch to
+    // recover the exact bytes the signature covers before hashing.
+    const signedContentBytes = Buffer.from(originalContent);
+    const eocdOffset = findEocdOffset(signedContentBytes);
+    if (eocdOffset !== -1) {
+      const sigBlockLength = fileContent.length - originalContent.length;
+      const patchedCommentLength = signedContentBytes.readUInt16LE(
+        eocdOffset + 20,
+      );
+      signedContentBytes.writeUInt16LE(
+        patchedCommentLength - sigBlockLength,
+        eocdOffset + 20,
+      );
+    }
 
-        if (!messageDigest || messageDigest !== digest) {
-          return { status: "unsigned" };
-        }
+    const contentMd = forge.md.sha256.create();
+    contentMd.update(signedContentBytes.toString("binary"));
+    const contentDigest = contentMd.digest().getBytes();
+
+    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;
       }
-    } catch (error) {
+    }
+    if (signedMessageDigest === null || signedMessageDigest !== contentDigest) {
       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 +245,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(),

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);