Clear the extension list when removing the last extension - #3344
Draft
nebeid wants to merge 4 commits into
Draft
Conversation
Cherry-picked from BoringSSL 40e035a9e5d721b3b7c15c46259d782ffe7d9e96.
The extensions list in a certificate, CRL, and CRL entry is defined as:
... extensions [3] EXPLICIT Extensions OPTIONAL ...
... crlEntryExtensions Extensions OPTIONAL ...
... crlExtensions [0] EXPLICIT Extensions OPTIONAL ...
Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension
This means that a present but empty extensions list is actually invalid.
Rather, if you have no extensions to encode, you are meant to omit the
list altogether. Fix the delete_ext functions to handle this correctly.
Adapted for AWS-LC:
- crypto/x509/x509_ext.c is a C file (upstream's is C++), so nullptr
becomes NULL in the new delete_ext helper
- Test added at the end of x509_test.cc after AWS-LC-only tests
- Fixed two upstream test nits: the "second extension" round now adds
ext2 (upstream created ext2 but never used it), and a comment typo
("but not list at all" -> "but no list at all")
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cherry-picked from BoringSSL dba32d768e2b48359f17ed2f094347f6a1dfc7f4. Older versions of Go did not encode extension-less certificates correctly. See https://go-review.googlesource.com/c/go/+/399827 Rerun the script now that the bug is fixed. Adapted for AWS-LC: - None. The six certificates are taken verbatim from the upstream commit (upstream reran crypto/x509/test/make_basic_constraints.go); they were not regenerated locally. Our previous copies were byte-identical to upstream's pre-refresh ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cherry-picked from BoringSSL f3230c304a6e77b78d43e52b3ab4da013272d318. Test that, signatures over unusual TBSCertificates are verified correctly. This tests that encoding is correctly round-tripped through the parser to the verifier. In principle, this should never happen because a DER parser will only accept the canonical encoding of an object. However, it is possible for encoding to not round-trip if we accept any BER inputs, or our in-memory representation does not capture the full range of abstract TBSCertificate values. X509 objects cache the encoded TBSCertificate, so all encoding variations should be captured. This test tries to exercise the cache's effects on signature verification. Adapted for AWS-LC: - Rewrote the "cache is barely load-bearing" paragraph of the test comment: unlike BoringSSL, AWS-LC supports BER inputs in the ASN1 macro parsers for OpenSSL compatibility (see PRs aws#2306 and aws#2310), so verification must use the cached TBSCertificate bytes - Registered the new test PEMs in sources.cmake (upstream uses gen/sources.*) - Added .c_str() for GetTestData() results; AWS-LC's CertFromPEM and PrivateKeyFromPEM take const char* Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3344 +/- ##
==========================================
+ Coverage 78.16% 78.19% +0.02%
==========================================
Files 693 693
Lines 123954 123982 +28
Branches 17212 17215 +3
==========================================
+ Hits 96892 96948 +56
+ Misses 26141 26113 -28
Partials 921 921 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
3 participants
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.
Description of changes:
Cherry-picks three BoringSSL commits in the x509 area:
1. 40e035a9e: "Clear the extension list when removing the last extension"
Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension— a present-but-empty extensions list is invalid DER. A new staticdelete_exthelper incrypto/x509/x509_ext.cfrees the list and nulls the pointer when the last extension is deleted, so re-encoding omits the field. Applied toX509_delete_ext,X509_CRL_delete_ext, andX509_REVOKED_delete_ext. Adds aDeleteLastExtensiontest covering all three paths.2. dba32d768: "Refresh basic constraints test certificates"
The six
basic_constraints_*.pemtest certs were generated by an old Go whose encoder emitted exactly the present-but-empty extensions field fixed above (go.dev/cl/399827). Certificates taken verbatim from upstream (upstream reranmake_basic_constraints.go); our previous copies were byte-identical to upstream's pre-refresh ones.3. f3230c304: "Test verifying signatures over 'unusual' TBSCertificates"
Adds 7 test certificates with legal-but-unusual or accepted-but-noncanonical TBS encodings (unique IDs, explicit v1, NULL sigalg params, empty extension list, inverted RDN order) plus the generator and key, verifying that signature verification uses the cached TBSCertificate bytes.
A fourth commit updates
generated-src/for the new test data (tests/check_generated_src.shpasses).Adaptations for AWS-LC:
crypto/x509/x509_ext.cis C, so upstream'snullptrbecomesNULLext2in the second round ofDeleteLastExtension(upstream created it but never used it) and fixed a comment typosources.cmake(upstream usesgen/sources.*); added.c_str()forGetTestData()(ourCertFromPEM/PrivateKeyFromPEMtakeconst char*)Testing:
Built with cmake/ninja (clang, Release); full
X509Testsuite passes (106 tests).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.