Skip to content

ecparams: accept zero-padded curve constants and non-DER cA BOOLEAN - #37

Merged
leifj merged 3 commits into
mainfrom
fix/ecparams-padded-constants-bool
Sep 30, 2026
Merged

leifj merged 3 commits into
mainfrom
fix/ecparams-padded-constants-bool

Conversation

@leifj

@leifj leifj commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the two certificates left unparsed by the ecparams Master List run (#36), both opt-in (nothing changes unless ecparams.Register is called):

  1. Zero-padded curve constants. The curve constants A and B are compared as integers, so a leading 0x00 pad (49-byte A/B on P-384, UAE CSCA 02) still matches the known curve. Every value must still equal the known number exactly. The Verifier path re-derives the key on the table curve and handles the certificate (tested).
  2. Non-DER cA BOOLEAN. A basicConstraints extension whose cA BOOLEAN is BER-style TRUE (any non-zero octet, e.g. 0x01 in CSCA-UKRAINE) is normalised to 0xFF for the stdlib parse only. Same-length patch of that one octet in a copy; the original extension value, Raw, RawTBSCertificate, Signature are restored, nothing is re-encoded in the result.

What stays rejected

  • Unknown curves and any non-matching A/B/prime/order/cofactor/generator, with or without padding (tested on P-384 and brainpoolP384r1); padding on the base point.
  • Malformed basicConstraints is never repaired: truncated, BOOLEAN of wrong length, non-BOOLEAN cA, trailing data, bad pathLen. The plugin never changes the stdlib verdict for these.
  • Tampered variants of both real certificates (changed constants, cA, signature, TBS).

Tests

Hand-built padded-A/B certificates on all 7 curves (P-384 and a brainpool curve included) parse and verify; BER-boolean certificates (named curve, explicit params, with pathLen); both real CSCAs added to ecparams/testdata with provenance headers (sha256 checked in the test); fuzz seeds extended (fuzzed 30s, no findings). ecparams coverage 97.2%; golangci-lint/vet/gofmt clean.

Real numbers (BSI German Master List, 608 certificates)

Before: 606 parsable, 2 failing. After:

certificates: 608
stdlib parsable: 433; rejected: 175
parsable with extensions: 608; still failing: 0
explicit-parameter keys recovered, by curve: P-256=24; P-384=31; P-521=22; brainpoolP256r1=22; brainpoolP384r1=54; brainpoolP512r1=17
deliberately declined explicit-parameter certificates: {}
self-signed certificates verified: 386 of 386

Docs: ecparams package doc and README section updated.

Note for release: v0.7.1

Closes #36

🤖 Generated with Claude Code

Compare the explicit-parameter curve constants A and B as numbers so a leading
0x00 pad (UAE CSCA 02, 49-byte P-384 A/B) still matches a known curve, and
normalise a BER-style TRUE cA BOOLEAN in basicConstraints (CSCA-UKRAINE) for the
stdlib parse while restoring the original bytes. Adds both real CSCAs as
testdata, padded/BER tests and fuzz seeds.

Closes #36

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 13:18
Split normalizeBasicConstraints into readExtensions/patchBasicConstraints and
factor shared test helpers.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Some fuzz seeds cannot reach the intended parser path, and public documentation overstates the accepted padding behavior.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Adds opt-in support for padded EC constants and BER-style basicConstraints booleans.

Changes:

  • Compares EC constants numerically.
  • Normalizes non-DER cA booleans while preserving original bytes.
  • Adds fixtures, tests, fuzz seeds, and documentation.
File Description
README.md Documents accepted encodings.
ecparams/​ecparams.go Implements normalization and numeric comparison.
ecparams/​helpers_test.go Extends certificate test builders.
ecparams/​padding_bool_test.go Adds regression and tampering tests.
ecparams/​fuzz_test.go Adds fuzz seeds.
ecparams/​testdata/​csca_are_padded_constants.pem Adds UAE certificate fixture.
ecparams/​testdata/​csca_ukr_ber_boolean.pem Adds Ukraine certificate fixture.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ecparams/ecparams.go Resolved
@leifj
leifj requested a balanced review from Copilot September 30, 2026 13:23
…claim to A and B

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation is narrowly scoped, preserves original signed data, and thoroughly tests valid and malformed cases.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@leifj
leifj merged commit f79520f into main Sep 30, 2026
14 checks passed
@leifj
leifj deleted the fix/ecparams-padded-constants-bool branch September 30, 2026 13:33
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.

ecparams: accept zero-padded curve constants (UAE CSCA) and non-DER BOOLEAN in basicConstraints (Ukraine CSCA)

2 participants