Skip to content

helpers: do not trim whitespace bytes off DER input - #1465

Open
lukevalenta wants to merge 1 commit into
cloudflare:masterfrom
lukevalenta:fix-parsecsr-trimspace
Open

lukevalenta wants to merge 1 commit into
cloudflare:masterfrom
lukevalenta:fix-parsecsr-trimspace

Conversation

@lukevalenta

Copy link
Copy Markdown
Contributor

ParseCSR and ParseCertificatesDER ran bytes.TrimSpace on DER input. The last byte of a DER encoding is a signature or key byte, so about 1 in 75 valid inputs ended in a byte that TrimSpace removes (0x09 to 0x0d, 0x20) and then failed to parse, for example with asn1: syntax error: data truncated.

On master, every DER input I generated that ended in such a byte failed: 8 of 8 CSRs and 5 of 5 certificates.

Change

Both functions now remove only leading whitespace and whitespace that follows a complete sequence of DER elements. Input that is not a sequence of DER elements is trimmed on both sides as before. PEM handling is unchanged.

  • Trailing garbage and truncated input still fail.
  • ParseCertificatesDER still accepts several concatenated certificates, as the existing x509.ParseCertificates call did.
  • A simpler "parse as given, then retry trimmed" version rejects input with a trailing newline, because the strict parser reports trailing data. The trailing-newline test covers that case.

Tests

The new tests fail on master. Reverting each part of the fix (either function, the leading trim, or the trailing trim) fails at least one test. go test ./helpers ./csr ./bundler/... ./cli/... ./api/bundle/... ./api/generator/... passes. I did not run scan, which also calls ParseCertificatesDER.

ParseCSR and ParseCertificatesDER ran bytes.TrimSpace on DER input. The
last byte of a DER encoding is a signature or key byte, so about 1 in 75
valid inputs ended in a byte that TrimSpace removes (0x09 to 0x0d, 0x20)
and then failed to parse, for example with "asn1: syntax error: data
truncated".

Both functions now remove only whitespace that follows a complete
sequence of DER elements, as well as leading whitespace. Input that is
not a sequence of DER elements is trimmed on both sides as before.
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