Skip to content

fix: make mcpb verify/info actually verify PKCS#7 signatures#255

Open
andy-liner wants to merge 1 commit into
modelcontextprotocol:mainfrom
andy-liner:fix/pkcs7-verify-eocd-reversal
Open

fix: make mcpb verify/info actually verify PKCS#7 signatures#255
andy-liner wants to merge 1 commit into
modelcontextprotocol:mainfrom
andy-liner:fix/pkcs7-verify-eocd-reversal

Conversation

@andy-liner
Copy link
Copy Markdown

Summary

mcpb verify and mcpb info currently report every signed bundle as unsigned, regardless of how it was signed.

Two distinct causes, both fixed here:

  1. node-forge's PkcsSignedData.verify() is not implemented — it always throws "PKCS#7 signature verification not yet implemented" (forge#1088). verifyMcpbFile() caught the throw and returned { status: "unsigned" }, so verification never actually ran. The "self-signed" status was also unreachable, because the OS trust-store check returned "unsigned" before it.

  2. The EOCD comment_length patch (fix: update ZIP EOCD comment_length when signing #204) breaks the content digest. signMcpbFile() bumps the ZIP EOCD comment_length by the signature-block length after computing the signature. So the bytes stored on disk differ from the bytes that were signed by exactly those two bytes, and a digest check over the stored content never matches — for any bundle produced by mcpb sign on current main.

Fix

verifyMcpbFile() now verifies the detached PKCS#7 signature manually:

  • the signed messageDigest authenticated attribute must equal SHA-256(content), and
  • the signer's signature must validate over the DER-encoded authenticated attributes (re-tagged as a SET OF, as PKCS#7 requires), using the certificate's public key.

Before hashing, it reverses the EOCD comment_length patch (subtracting the signature-block length) so the digest matches what signMcpbFile() actually signed.

Trust levels:

condition status
valid signature + OS-trusted chain signed
valid signature + self-signed cert (issuer CN == subject CN) self-signed
valid signature + untrusted, non-self-signed chain unsigned
no/invalid signature, tampered content unsigned

Reproduction (before this PR)

mcpb pack . ext.mcpb
openssl req -x509 -newkey rsa:2048 -nodes -keyout key.pem -out cert.pem -days 825 \
  -subj "/O=Example/CN=Example"
mcpb sign ext.mcpb --cert cert.pem --key key.pem   # "Successfully signed"
mcpb verify ext.mcpb                                # ERROR: Extension is not signed
mcpb unsign ext.mcpb                                # "Signature removed"  <- the sig *was* there

unsign recognizes the signature while verify/info do not — because the only failure was the unimplemented p7.verify() throw (and, on current main, the EOCD digest mismatch).

Tests

The existing self-signed e2e test accepted both "self-signed" and "unsigned", which is exactly why this regression went unnoticed. It now requires "self-signed" and checks the publisher. All sign.e2e cases pass (sign/verify self-signed, CA-signed, tamper detection, unsigned, unsign, EOCD comment_length).

Relation to existing PRs

Fixes #21.

🤖 Generated with Claude Code

`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 modelcontextprotocol#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: modelcontextprotocol#195 (championed verify fix, native crypto; lacks the EOCD reversal
needed on current main), modelcontextprotocol#205, modelcontextprotocol#212. Fixes modelcontextprotocol#21.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ERROR: Extension is not signed

1 participant