Skip to content

fix: sign the bytes that are actually written to disk - #1

Open
bryan-anthropic wants to merge 1 commit into
mainfrom
fix/sign-digest-covers-eocd-patch
Open

fix: sign the bytes that are actually written to disk#1
bryan-anthropic wants to merge 1 commit into
mainfrom
fix/sign-digest-covers-eocd-patch

Conversation

@bryan-anthropic

Copy link
Copy Markdown
Owner

Problem

signMcpbFile computes the detached PKCS#7 signature over the pre-patch buffer, then grows the ZIP EOCD comment_length afterwards:

p7.content = forge.util.createBuffer(mcpbContent);   // digest binds to THIS
p7.sign({ detached: true });                          // signing completes here
...
const updatedContent = Buffer.from(mcpbContent);      // copy, so mcpbContent is untouched
updatedContent.writeUInt16LE(..., eocdOffset + 20);   // only the copy is patched
writeFileSync(mcpbPath, Buffer.concat([updatedContent, signatureBlock]));

The two bytes at eocdOffset + 20 therefore differ between the content that was signed and the content on disk. verifyMcpbFile reconstructs the signed content as everything before the signature header — the patched version — and so does any other standards-compliant verifier.

Reproduction

Packing a bundle with mcpb pack, signing it, then verifying the detached signature with OpenSSL:

differing byte offsets  : [455,456]
EOCD offset             : 435  (comment_length field at 455)

against bytes ACTUALLY ON DISK          => FAILED: PKCS7_signatureVerify:digest failure
against pre-patch bytes (never on disk) => VERIFICATION SUCCESSFUL

The signature validates only against bytes that never reach disk. Every signed bundle is affected — the comment_length always grows by a nonzero amount.

This is a regression: at the parent of the commit that introduced the comment_length patch, the same reproduction gives VERIFICATION SUCCESSFUL against the on-disk bytes.

Fix

The comment_length depends on the signature block length, and the signature covers the patched bytes, so the two are mutually dependent. This patches the EOCD field first using the length of a trial signature, then re-signs, iterating until the block length stops changing. It converges immediately in practice, because the content is detached and does not affect the DER encoding size. A bound guards against a length that never stabilizes, and an oversized block now raises a clear error instead of silently throwing from writeUInt16LE.

Tests

Adds a regression test that pulls the messageDigest authenticated attribute out of the signature and asserts it equals the SHA-256 of the on-disk pre-signature bytes. It fails on the current code with the expected digest mismatch and passes with the fix. Extraction uses node-forge's ASN.1 parser rather than shelling out to OpenSSL, so it runs on all CI platforms.

The existing tests could not catch this — they accept ["self-signed", "unsigned"] as passing, and the tampering test asserts "unsigned", which holds trivially.

Full suite: 126 passed. tsc --noEmit and Prettier are clean.

Note on mcpb verify

Unrelated to this change and not addressed here: verifyMcpbFile calls node-forge's p7.verify(), which is a stub that throws PKCS#7 signature verification not yet implemented in both 1.3.3 and 1.4.0. It is caught and turned into {status: "unsigned"}, so mcpb verify reports "Extension is not signed" for every signed bundle and the digest comparison below it is unreachable. That is worth a separate fix.

signMcpbFile computed the detached PKCS#7 signature over the pre-patch
buffer and only afterwards grew the ZIP EOCD comment_length to account
for the appended signature block. Because Buffer.from() copies, the two
bytes at eocdOffset + 20 differ between the content that was signed and
the content written to disk, so the messageDigest authenticated
attribute never matches the file's pre-signature bytes.

verifyMcpbFile reconstructs the signed content as everything before the
signature header, which is the patched version, and any other
standards-compliant verifier does the same. OpenSSL rejects such a file
with PKCS7_signatureVerify:digest failure. Bundles signed before the
comment_length patch was introduced verify cleanly, so this is a
regression.

The comment_length depends on the signature block length and the
signature covers the patched bytes, so the two are mutually dependent.
Patch the EOCD field first using the length of a trial signature, then
re-sign, iterating until the block length stops changing. In practice
this converges immediately because the content is detached and does not
affect the DER encoding size.

Adds a regression test that pulls the messageDigest authenticated
attribute out of the signature and asserts it equals the SHA-256 of the
on-disk pre-signature bytes. The existing tests could not catch this:
they accept either "signed" or "unsigned" as a passing status.
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.

1 participant