🐛 extractSignature: read DER length instead of stripping trailing 00 pairs (fixes #317) - #319
Open
razortrax wants to merge 1 commit into
Open
Conversation
…pairs The trailing-byte strip (replace(/(?:00|>)+$/)) cannot distinguish sign()'s zero-padding from a genuine signature whose last byte(s) happen to be 0x00 (~1/256 chance per signature), so it intermittently truncated valid DER and downstream parsers failed with "Too few bytes to read ASN.1 value." Read the total element length from the DER SEQUENCE header instead; fall back to the historical trailing-zero strip when the blob is not parseable DER. Fixes vbuch#317. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot stopped reviewing on behalf of
dhensby due to an error
August 5, 2026 07:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
extractSignature()strips the placeholder padding with:The regex cannot distinguish
sign()'s zero-padding from a genuine signature whose last byte(s) happen to be0x00. A PKCS#7SignedData's final bytes are the raw RSA signature — effectively random — so ~1/256 signatures genuinely end in0x00. When one does, the strip eats real DER bytes and downstream parsers throwToo few bytes to read ASN.1 value.on a perfectly valid signature.Full analysis in #317. We hit this as an intermittent (~0.4%) verification failure in CI; reproduced 9 truncations in 1500 sign/extract cycles against
@signpdf/utils3.x.Fix
DER is self-describing: read the total element length from the
SEQUENCEheader (short- and long-form lengths supported) and slice there — exactly the direction suggested in #317. If the blob does not parse as a DERSEQUENCE, fall back to the historical trailing-zero strip so non-DER edge cases keep their previous behavior.Also, the slice-then-strip previously relied on the regex to remove the trailing
>; the hex cleanup now strips only>and whitespace before decoding.Tests
0x00byte is kept (short-form DER, the extractSignature(): trailing-zero regex strip can truncate a genuine signature byte (~1/256 chance), corrupting DER #317 scenario)0x00with padding is trimmed correctlysigned.pdfsnapshot unchanged; full monorepolerna run testgreen (63/63 in utils)🤖 Generated with Claude Code