Conversation
There was a problem hiding this comment.
Pull request overview
Introduces configurable decoding limits for BER_Decoder and switches many ASN.1-consuming code paths to require canonical DER encodings by default.
Changes:
- Add
BER_Decoder::Limits(DER-only vs BER-accepting) and enforce DER canonicalization rules during decoding. - Update X.509, OCSP, TLS session, and various public/private key parsers to decode with
Limits::DER()and add additionalverify_end()calls. - Add DER-vs-BER decoding regression tests and extend ASN.1 pretty printer to optionally reject non-canonical BER.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/test_asn1.cpp | Adds text-based DER/BER acceptance tests via ASN.1 pretty printer |
| src/tests/data/asn1_oid_invalid.vec | Adds invalid OID case for indefinite-length primitive encoding |
| src/tests/data/asn1_decoding.vec | New vector set covering DER canonicalization checks (bool/int/bitstring/length/tag/eoc) |
| src/lib/x509/x509cert.cpp | Require DER parsing for certificate body and SPKI bits; tighten end verification |
| src/lib/x509/x509_obj.cpp | Require DER when loading X.509 objects from BER/PEM sources |
| src/lib/x509/x509_ext.cpp | Require DER for extension decoding; add verify_end() and spec references |
| src/lib/x509/x509_dn.cpp | Propagate decoder limits when decoding nested DN content |
| src/lib/x509/x509_crl.cpp | Require DER for CRL body; propagate limits into nested decoders |
| src/lib/x509/pkcs10.cpp | Require DER for PKCS#10 CSR decoding; propagate limits |
| src/lib/x509/ocsp.cpp | Require DER for OCSP response parsing; add end-verification and documentation comments |
| src/lib/x509/name_constraint.cpp | Propagate decoder limits into nested GeneralName DN decoding |
| src/lib/x509/alt_name.cpp | Propagate decoder limits into nested AlternativeName decoding |
| src/lib/utils/data_src.cpp | Adjust stream EOF detection by peeking to trigger EOF state |
| src/lib/tls/tls_session.cpp | Require DER for serialized TLS session decoding |
| src/lib/pubkey/xmss/xmss_publickey.cpp | Require DER for XMSS key encapsulated OCTET STRING decoding |
| src/lib/pubkey/xmss/xmss_privatekey.cpp | Require DER for XMSS key encapsulated OCTET STRING decoding |
| src/lib/pubkey/x509_key.cpp | Require DER for SubjectPublicKeyInfo decoding and verify end-of-input |
| src/lib/pubkey/x25519/x25519.cpp | Require DER for X25519 private key inner OCTET STRING decoding |
| src/lib/pubkey/sm2/sm2_enc.cpp | Require DER for SM2 ciphertext ASN.1 decoding and verify end-of-input |
| src/lib/pubkey/rsa/rsa.cpp | Require DER for RSA public/private key decoding and verify end-of-input |
| src/lib/pubkey/pubkey.cpp | Rework DER signature pair decoding to use DER-limited decoder |
| src/lib/pubkey/pkcs8.cpp | Require DER for PKCS#8 PrivateKeyInfo decoding |
| src/lib/pubkey/pbes2/pbes2.cpp | Require DER for PBES2 parameter decoding and verify end-of-input |
| src/lib/pubkey/mce/mceliece_key.cpp | Require DER for McEliece key decoding |
| src/lib/pubkey/gost_3410/gost_3410.cpp | Require DER for GOST parameter/public key decoding; tighten parsing |
| src/lib/pubkey/ed25519/ed25519_key.cpp | Require DER for Ed25519 private key inner OCTET STRING decoding |
| src/lib/pubkey/ecc_key/ecc_key.cpp | Require DER for EC private key structure decoding and verify end-of-input |
| src/lib/pubkey/dl_algo/dl_scheme.cpp | Require DER for single-bigint decoding and verify end-of-input |
| src/lib/pubkey/curve448/x448/x448.cpp | Require DER for X448 private key inner OCTET STRING decoding |
| src/lib/pubkey/curve448/ed448/ed448.cpp | Require DER for Ed448 private key inner OCTET STRING decoding |
| src/lib/prov/pkcs11/p11_ecc_key.cpp | Require DER for PKCS#11 EC point OCTET STRING decoding and verify end |
| src/lib/asn1/pss_params.cpp | Require DER for PSS params and propagate decoder limits to nested MGF params |
| src/lib/asn1/ber_dec.h | Add BER_Decoder::Limits API and plumb limits through optional decoding helpers |
| src/lib/asn1/ber_dec.cpp | Implement DER canonicalization checks (tags/lengths/integers/bitstrings/EOC) and limit handling |
| src/lib/asn1/asn1_print.h | Add optional DER-requirement flag to ASN.1 formatter/printer APIs |
| src/lib/asn1/asn1_print.cpp | Use BER_Decoder::Limits in ASN.1 formatter and propagate limits into nested decoders |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cd041e5 to
31e4d38
Compare
Allows restricting the sort of DER/BER inputs which will be accepted. In practice upon examination it seems nearly everything decoded by the library can be required to be DER. It seems possible some partial revert of this will be required (eg to support BER encoded RSA private keys; unlike PKIX, PKCS8 does not explicitly spell out that DER is required) but lacking any test cases or even indications this is an issue, just require DER.
C++ streams are not my favorite IO library
31e4d38 to
ac6c662
Compare
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.
Allows restricting the sort of DER/BER inputs which will be accepted.
In practice upon examination it seems nearly everything decoded by the library can be required to be DER. It seems possible some partial revert of this will be required (eg to support BER encoded RSA private keys; unlike PKIX, PKCS8 does not explicitly spell out that DER is required) but lacking any test cases or even indications this is an issue, just require DER.