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

Content matching also accounts for the EOCD `comment_length` bump that
`signMcpbFile()` applies *after* signing (added in modelcontextprotocol#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: modelcontextprotocol#195 (championed verify fix, native crypto; lacks the EOCD handling
needed on current main), modelcontextprotocol#205, modelcontextprotocol#212. Fixes modelcontextprotocol#21.

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

@rm0nroe rm0nroe left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent verification of 77fc48367fa1f8f5a30a88f8525db4bcfbe97a5c on macOS 26.5.2 / Node 24.9.0:

  • yarn install --immutable: pass
  • yarn lint: pass
  • yarn build: pass
  • yarn test --runInBand --silent: 8 suites / 126 tests pass
  • git diff --check origin/main...HEAD: pass

I also reviewed the trust-boundary flow: this replaces the unimplemented node-forge verifier, binds messageDigest to the bundle content, verifies the RSA signature over authenticated attributes, and handles both stored EOCD bytes and the current post-sign comment-length mutation. It addresses the two root causes documented in #260 for the current MCPB signer.

The PR is mergeable against current main, but the upstream Actions matrix has not run and the source commit is unsigned. @bryan-anthropic @joan-anthropic @tobinsouth — could one of you trigger CI, review, and squash-merge if acceptable? Catalyst Edge is withholding its Claude one-click package until a released verifier and Desktop uptake can prove publisher trust.

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

2 participants