Skip to content

verifyMcpbFile always reports signed extensions as "unsigned" (node-forge pkcs7.verify() is an unimplemented stub) #276

@tekun

Description

@tekun

Summary

verifyMcpbFile() in src/node/sign.ts can never report a .mcpb as signed or self-signed — it always returns { status: "unsigned" },
even for a file that was just signed with mcpb sign.

The function delegates signature verification to node-forge's PKCS#7 signed-data verify(). In node-forge that method is an unimplemented stub
that throws:

// node_modules/node-forge/lib/pkcs7.js
verify: function() {
  throw new Error('PKCS#7 signature verification not yet implemented.');
}

This is present in every published node-forge 1.x, including 1.4.0 (the version the current "node-forge": "^1.3.2" range resolves to
today) — confirmed at lib/pkcs7.js:381.

Impact

mcpb is fail-closed here (nothing ever verifies as trusted, so this is not a signature-forgery/bypass), but the signing feature is
effectively non-functional: a signed publisher identity is indistinguishable from no signature at all on macOS, Windows, and Linux. The
certificate-chain / OS-trust verification (sign.ts:192-227) is unreachable dead code.

Steps to reproduce

mcpb sign extension.mcpb --cert cert.pem --key key.pem   # succeeds
mcpb verify extension.mcpb                                # => unsigned

Control flow:

  • src/node/sign.ts:165 p7.verify({ authenticatedAttributes: true }) throws.
  • caught at src/node/sign.ts:188-190return { status: "unsigned" }.

Root cause

Reliance on forge.pkcs7.PkcsSignedData.verify(), which is not implemented in node-forge. The cast at sign.ts:142-150 adds a verify member to
the type, but the runtime method still throws.

Note: on a parsed message, p7.signers is also empty, so the signerInfos[0]-shaped access the current code assumes would not work even if
verify() were implemented. The parsed signer fields live on p7.rawCapture (signature, authenticatedAttributes).

Suggested fix

Verify the signer manually with node-forge's lower-level primitives instead of the stub: (1) confirm the messageDigest authenticated attribute
equals SHA-256(content), then (2) verify the signer's RSA signature over the DER of the authenticated attributes re-tagged as a SET
(PKCS#7/CMS rule).

const rc = (p7 as any).rawCapture as {
  signature: string;
  authenticatedAttributes: forge.asn1.Asn1[];
};
if (!rc?.signature || !rc?.authenticatedAttributes) {
  return { status: "unsigned" };
}

// 1) messageDigest attribute must equal SHA-256(content)
const md = forge.md.sha256.create();
md.update(contentBuf.getBytes());
const contentDigest = md.digest().getBytes();

let messageDigest: string | null = null;
for (const attrSeq of rc.authenticatedAttributes) {
  const oid = forge.asn1.derToOid((attrSeq as any).value[0].value);
  if (oid === forge.pki.oids.messageDigest) {
    messageDigest = (attrSeq as any).value[1].value[0].value;
    break;
  }
}
if (messageDigest === null || messageDigest !== contentDigest) {
  return { status: "unsigned" };
}

// 2) RSA-verify the signature over the authenticatedAttributes SET
const attrsSet = forge.asn1.create(
  forge.asn1.Class.UNIVERSAL,
  forge.asn1.Type.SET,
  true,
  rc.authenticatedAttributes,
);
const attrsMd = forge.md.sha256.create();
attrsMd.update(forge.asn1.toDer(attrsSet).getBytes());

let signatureValid = false;
try {
  signatureValid = (signingCert.publicKey as forge.pki.rsa.PublicKey)
    .verify(attrsMd.digest().getBytes(), rc.signature);
} catch {
  signatureValid = false;
}
if (!signatureValid) {
  return { status: "unsigned" };
}
// ...existing cert-chain / OS-trust verification then runs as before.

I verified this approach locally against node-forge 1.4.0 with positive and negative test vectors:

  • Current stubbed verify() on a valid signature → throws, forcing unsigned (bug reproduced)
  • Good signed content → verifies ✅
  • Tampered content → rejected (messageDigest mismatch) ✅
  • Flipped signature byte → rejected (RSA verify fails) ✅
  • Attacker-signed blob for different content → rejected ✅

Caveats / for maintainer decision

  • The snippet handles RSA signers only — which matches what signMcpbFile currently produces. ECDSA signing certs would need a different
    publicKey.verify path.
  • Hand-rolled PKCS#7/CMS verification is delicate; please pair any fix with committed positive + negative test vectors. An alternative worth
    considering is verifying via Node's built-in crypto (X509Certificate / crypto.verify) or the OS toolchain already used in
    verifyCertificateChain, whichever you can best test against the signing pipeline.

Happy to open a PR with the fix + tests if you'd like.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions