fix: make mcpb verify/info actually verify PKCS#7 signatures - #255
fix: make mcpb verify/info actually verify PKCS#7 signatures#255andy-liner wants to merge 1 commit into
mcpb verify/info actually verify PKCS#7 signatures#255Conversation
`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>
622ff31 to
77fc483
Compare
rm0nroe
left a comment
There was a problem hiding this comment.
Independent verification of 77fc48367fa1f8f5a30a88f8525db4bcfbe97a5c on macOS 26.5.2 / Node 24.9.0:
yarn install --immutable: passyarn lint: passyarn build: passyarn test --runInBand --silent: 8 suites / 126 tests passgit 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.
Summary
mcpb verifyandmcpb infocurrently report every signed bundle asunsigned, regardless of how it was signed.Two distinct causes, both fixed here:
node-forge'sPkcsSignedData.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.The EOCD
comment_lengthpatch (fix: update ZIP EOCD comment_length when signing #204) breaks the content digest.signMcpbFile()bumps the ZIP EOCDcomment_lengthby 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 bymcpb signon currentmain.Fix
verifyMcpbFile()now verifies the detached PKCS#7 signature manually:messageDigestauthenticated attribute must equalSHA-256(content), andSET OF, as PKCS#7 requires), using the certificate's public key.Before hashing, it reverses the EOCD
comment_lengthpatch (subtracting the signature-block length) so the digest matches whatsignMcpbFile()actually signed.Trust levels:
signedself-signedunsignedunsignedReproduction (before this PR)
unsignrecognizes the signature whileverify/infodo not — because the only failure was the unimplementedp7.verify()throw (and, on currentmain, 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. Allsign.e2ecases pass (sign/verify self-signed, CA-signed, tamper detection, unsigned, unsign, EOCD comment_length).Relation to existing PRs
cryptoverify): same core idea. However, on currentmainit hashesoriginalContentas-extracted and so still returnsunsignedformcpb sign-produced bundles, because it does not reverse the post-sign EOCDcomment_lengthbump (fix: update ZIP EOCD comment_length when signing #204). This PR includes that reversal, so it works against the currentmcpb signwithout depending on the external-signing flow in feat: add prepare-for-signing and apply-signature for enterprise HSM signing #222.Fixes #21.
🤖 Generated with Claude Code