Skip to content

fix: reject non-canonical ASN.1 lengths#44

Open
leanthebean wants to merge 1 commit into
base:mainfrom
leanthebean:security/reject-noncanonical-cert-der
Open

fix: reject non-canonical ASN.1 lengths#44
leanthebean wants to merge 1 commit into
base:mainfrom
leanthebean:security/reject-noncanonical-cert-der

Conversation

@leanthebean

Copy link
Copy Markdown
Contributor

Summary

Fixes CAT finding a35a941e-3a1a-45a5-a71f-c1a8ebcf5065.

CertManager keys cached certificates by keccak256(cert), so the verifier must reject alternate byte encodings of the same signed X.509 certificate. Trailing certificate bytes are already rejected on main; this PR closes the remaining non-canonical ASN.1 length-encoding path.

This change updates Asn1Decode.readNodeLength to enforce DER length rules:

  • reject BER indefinite lengths (0x80);
  • reject long-form encodings for lengths below 128;
  • reject leading-zero long-form length encodings;
  • reject truncated length headers and nodes whose declared length exceeds the input buffer.

Self Review

Reviewed the final diff before opening this PR. The production change is scoped to ASN.1 length decoding, so all certificate parsing paths get the same DER canonicalization. Existing canonical AWS Nitro certificates continue to verify, while a real Nitro CA cert re-encoded with a non-minimal outer SEQUENCE length now reverts before _saveVerified can write a new cache entry.

Tests

  • forge test --match-path test/Asn1Decode.t.sol -vvv
  • forge test --match-test test_HintedCACertRejectsNonCanonicalOuterLength -vvv
  • forge test

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