Skip to content

[US-1967] Decompose BER-encoded CMS signatures and show signature problems - #36

Merged
unidoc-anom merged 5 commits into
devfrom
fix/ber-tolerant-cms-parsing
Oct 21, 2026
Merged

unidoc-anom merged 5 commits into
devfrom
fix/ber-tolerant-cms-parsing

Conversation

@unidoc-anom

Copy link
Copy Markdown
Contributor

Description and links

DocuSign writes the CMS in a signature's /Contents as BER with indefinite lengths. Go's encoding/asn1 only takes DER, so dump signatures and the Signatures panel stopped at CMS parse failed: asn1: syntax error: indefinite length found (not DER) and listed no signer, chain or algorithms, while openssl and unipdf read the same file (unipdf model/testdata/DocuSign_signature.pdf).

This adds a BER to DER normalizer used only for parsing, reports which encoding was found, and reports stray bytes after the CMS instead of failing the signature. Testing the panel on real files turned up a few display bugs that are fixed here too: hex-encoded field names shown as hex digits, a signer name cut at an escaped comma, signature problems that were easy to miss, and the detail tab jumping back to Object on every document switch.

Jira: US-1967

Technical changes

  1. internal/pdfcore/ber.go (new): normalizeBER reads one top-level element. A check-only first pass validates tags and lengths and detects BER (indefinite lengths, non-minimal lengths, constructed universal strings). Clean DER returns a copy from that pass with no other allocation. BER gets a recording pass, which notes each constructed element's normalized length, and an emit pass that writes once into a buffer of the final size. Constructed strings are flattened (BIT STRING unused-bits rules included; character strings accept OCTET STRING segments per X.690 8.23.5), element order is never changed, and nesting deeper than 64 levels is an error. There is no size limit: the input is already in memory and the output can only exceed it by a few bytes per element. Error texts carry the byte offset.
  2. internal/pdfcore/signature.go: parseCMSSignedData normalizes first, then parses. SignatureField gains CMSEncoding (DER, BER, or empty when the payload did not decode) and CMSTrailingBytes (offset, count, set when non-zero bytes follow the CMS). Bytes after the SignedData inside the ContentInfo become a note. The signature still decomposes in full in both cases. /T, /Name, /Reason, /Location, /ContactInfo and /M go through the text-string decoder, so hex strings and UTF-16 decode.
  3. CLI dump signatures: a CMS encoding: line, and a Trailing bytes: line when there are any. --json gains cmsEncoding and cmsTrailingBytes.
  4. Signatures panel: CMS encoding row, amber Trailing bytes row. A signature that cannot be decomposed, or whose coverage cannot be checked, shows a red notice at the top of its card instead of a gray row at the bottom, and the Signatures tab label gets a dot (red for errors, amber for trailing bytes). A failed GetSignatures now shows the tab with the error; it used to hide the tab. The notice and the dot are new components (ProblemCallout, TabIssueMarker) meant for the other tabs next. Signer and issuer rows parse the DN on unescaped separators, so CN=DocuSign\, Inc. shows DocuSign, Inc.; the expanded chain shows DNs without escapes and quotes values that contain a comma.
  5. DetailPanel: the active detail tab is remembered per document tab, so switching back returns to it. A new open or a reload (new tab id) starts on Object, and a restored Signatures tab waits for the signature list.
  6. Fixtures from testdata/generate_test.go: signed-ber.pdf (openssl cms -sign -stream, needs OpenSSL 3 only to regenerate), signed-ber-detached.pdf (Go-built, the DocuSign layout), signed-ber-badeoc.pdf (bad end-of-contents) and signed-ber-trailing.pdf (non-zero bytes after the CMS). CI never needs openssl.
  7. New acceptance suite tests/ber-cms-signatures (CLI output shape and exit codes). CHANGELOG entries under Unreleased; docs/cli/document-data.md updated.

Considerations

The normalizer is in-house rather than pdfcpu's vendored hhrutter/pkcs7 ber2der, so there is no new direct dependency and the bounds and error offsets are ours.

Stray bytes after the CMS used to be ignored without a word. Making them an error would have hidden the signer for a recoverable problem, and pdfdebug shows structure rather than judging validity, so they are reported and decomposition goes on.

An 8 MiB cap was in the first version and was taken out: it rejected large DER signatures that embed revocation data, which decomposed before, and it guarded memory that was already allocated.

How was this tested?

  • GOFLAGS=-count=1 scripts/test-all.sh: go vet, golangci-lint 0 issues, root module, every tests/*/ suite including tests/ber-cms-signatures, frontend typecheck, lint and Vitest (100 files, 1500 tests). All suites passed.
  • go test -race ./internal/pdfcore/... passed.
  • ber_test.go: table cases for every accepted form and every error (with offsets), a fully indefinite re-encoding of the signed.pdf CMS that must normalize back to the original bytes, a 64-deep 12 MiB BER input with proportional allocation, DER taking exactly one allocation, and FuzzNormalizeBER (no panic, input unchanged, idempotent, output length bound). The fuzzer ran for 30s several times with no failure.
  • New tests fail on the code before their fix (hex text strings, escaped-comma CN, per-document tab).
  • Manual on a macOS production build (task package): DocuSign_signature.pdf shows BER, signer DocuSign, Inc., 3 certificates, hole matches; signed-ber-badeoc.pdf shows the red notice and the tab dot; signed-ber-trailing.pdf shows the amber row; switching documents keeps each one's tab.

What could go wrong?

A signature whose only non-DER features are ones the normalizer does not rewrite (non-minimal INTEGERs, SET OF order) still fails in encoding/asn1, as before. A constructed [0] IMPLICIT SubjectKeyIdentifier is not flattened, so such a signer shows as not identified.

Without a size limit, memory for a BER payload grows with the /Contents size, about 8 bytes of bookkeeping per constructed element on top of the output. Clean DER pays only one copy.

Windows and Linux production builds were not checked by hand.

3ace added 3 commits October 19, 2026 14:37
dump signatures and the Signatures panel now accept BER CMS (indefinite
lengths, constructed strings) as DocuSign writes it, via an in-house
bounded BER->DER normalizer used only for parsing. Output reports the
encoding (CMS encoding line, cmsEncoding in --json). Non-zero bytes after
the CMS, and CMS over 8 MiB or 64 levels deep, are decompose errors.
…cument

A signature that cannot be decomposed shows a red notice on its card and
marks the Signatures tab; a failed signature load shows the error instead
of hiding the tab. Hex text strings (/T, /Name, /Reason, ...) are decoded,
signer/issuer names keep escaped commas, and the chain shows readable DNs.
The detail panel restores each document's last tab on switch.

@anovik anovik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The BER normalizer looks correct to me. I went through ber.go in full: the pre-order length and unused-bits records stay in step between the record and emit passes, non-minimal long-form lengths are detected, the BIT STRING segment rules hold, and the depth cap covers both elements and segments. No blockers. Three behaviour issues are inline.

One more that is outside the diff: the stringValue doc comment in internal/pdfcore/embedded.go (around line 311) still lists "the signature /T, /Name, /Reason, /Location, /ContactInfo" as fields that are only unwired. This PR moves them to textStringOrRaw, so that sentence is now wrong. Per CLAUDE.md, please edit the comment to match.

Optional nits:

  • The trailing-bytes text has no singular, so a single stray byte reads 1 bytes after the CMS ... in both dump signatures and the panel Row (signaturesIssue already handles the plural).
  • On the DER path normalizeBER returns bytes.Clone(in[:end]). The only caller passes a freshly decoded /Contents buffer and asn1.Unmarshal does not write to its input, so in[:end] would avoid one full-size copy per signature.

f.Reason = stringValue(dereference(doc, sig["Reason"]))
f.Location = stringValue(dereference(doc, sig["Location"]))
f.ContactInfo = stringValue(dereference(doc, sig["ContactInfo"]))
f.Name = textStringOrRaw(dereference(doc, sig["Name"]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With these fields (and /T at line 212, /M at 282) now decoded as text strings, the plain presenter in cmd/cli/cmd_signatures.go (printSignaturesPlain: Field, Signing time, Name, Reason, Location, Contact) writes the decoded values with w.Add and no asciiSafe. A UTF-16 hex /Name <FEFF00E9...> used to print as ASCII hex digits and now prints raw UTF-8, which breaks the "Output is ASCII-only" contract in cmd/cli/output.go. cmd_metadata.go:83 already wraps the same kind of decoded text with asciiSafe(v). The same wrapping on those six rows would fix it, plus a plain-output test with a non-ASCII /Name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d82f23. Field, Signing time, Name, Reason, Location and Contact go through asciiSafe now, and so do the Signer, Issuer and chain lines, which had the same gap (pkix DN strings keep UTF-8). TestPrintSignaturesPlain_NonASCIITextIsEscaped in cmd/cli/cmd_signatures_test.go checks every output byte is ASCII and fails without the change.

Comment thread frontend/src/components/DetailPanel.tsx Outdated
for (const id of viewByTabRef.current.keys()) {
if (!open.has(id)) viewByTabRef.current.delete(id);
}
setDetailView((activeTabId && viewByTabRef.current.get(activeTabId)) || 'object');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When the remembered view is 'signatures' it is restored here straight away, but the same effect resets signatures to null and the fetch effect runs again on every switch. While signatures === null, showSignaturesTab is false, so neither the Signatures trigger nor its Tabs.Content renders. Tabs.Root then has a value with no matching tab: until GetSignatures resolves the panel is blank, no tab is highlighted, and the Object pane (with its find listener) is inactive. This repeats on every switch back, and it lasts longer on large files where the hole check reads from disk.

DetailPanel.tabMemory.test.tsx only waitFors the eventual restore, so it does not catch the intermediate state. Two possible fixes: render the Signatures trigger and content (with a loading state) while the list is unresolved and the restored view is 'signatures', or keep 'object' active until the list resolves and then apply the remembered view.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5d82f23 with your second option. The panel renders a shownView that is Object while the restored view is Signatures and the list is still unresolved, so Tabs.Root always has a matching tab and the Object pane stays live; once the list resolves the remembered Signatures tab comes back. New test while the signature list loads after a switch back, Object is shown holds the fetch on a deferred promise and checks the in-between state.

setEmbeddedCount(null);
setSignatures(null);
setSignaturesError(null);
setDiffRightTabId(null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A remembered 'diff' is restored above, but this effect also clears diffRightTabId, diffError and the disk copy (whose backend document is closed). After running Compare with File on Disk on doc A, switching to B and back to A, the Diff tab is selected but shows the empty "Compare with another PDF..." picker, not the comparison the user left. The CHANGELOG says switching back "returns to the tab you left". Either keep the comparison per document, don't remember 'diff' (fall back to Object), or reword the CHANGELOG entry so it does not promise this for Diff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Took the "don't remember Diff" route in 5d82f23: the comparison is still cleared on every switch, so a remembered Diff now comes back on Object instead of the empty picker. CHANGELOG says Diff is the exception. Keeping the comparison per document is a bigger change; happy to do it separately if we want it.

@unidoc-anom

unidoc-anom commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

@anovik Outside-the-diff items, all in 5d82f23:

  • stringValue doc comment in embedded.go: the signature fields are gone from the "only unwired" list.
  • Singular: 1 byte after the CMS at offset N contains non-zero data in both dump signatures and the panel row, with a test on each side.
  • bytes.Clone on the DER path: kept. TestNormalizeBER_ReturnsANewBuffer and the story spec pin that the output never aliases the input, so a caller can never write through it into the decoded /Contents; the cost is one copy of a few KB per signature.

@unidoc-anom
unidoc-anom requested a review from anovik October 20, 2026 16:34
@unidoc-anom
unidoc-anom merged commit 98d1cc9 into dev Oct 21, 2026
3 checks passed
@unidoc-anom
unidoc-anom deleted the fix/ber-tolerant-cms-parsing branch October 21, 2026 07:45
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.

3 participants