Reject present-but-empty name constraint subtrees - #15560
Open
avalyset wants to merge 1 commit into
Open
Conversation
GeneralSubtrees is SEQUENCE SIZE (1..MAX), so a subtree field that is present must not be empty. An empty permittedSubtrees was accepted and then had no effect, because the loop over the subtrees never runs, so every name passed the permitted side instead of none.
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.
Reported privately as GHSA-fw9w-89pp-fpmg. The advisory is not public, so there is nothing to link. The response there was that this is a regular bug rather than a security issue, with a request to open a public PR instead; this is that PR. We had offered no severity assessment either way.
The rule is already enforced in the Python layer
src/cryptography/x509/extensions.py:1344:That check came from #6982, "Possible bug: empty sequence in NameConstraints" (opened and closed 2022-03-19), where the conclusion was explicit:
The Rust path validator did not enforce it
policy/extension.rsrejected only when both subtree fields were empty, so an emptypermittedSubtreescombined with a non-emptyexcludedSubtreespassed that check. Inlib.rs:258-274the permit flag then keeps its default:With an empty sequence the loop body never runs, so
permitstaystrueand every SAN is accepted by the permitted side.Basis
RFC 5280 6.1.4 (g)(1) sets
permitted_subtreesto the intersection of its previous value and the value in the extension; the intersection with an empty set is empty. RFC 5280 6.1.3 (b) requires the name to lie withinpermitted_subtrees, so with an empty one no name qualifies and every certificate below that CA should be rejected. It was accepting all of them instead.Reaching this requires a signed CA certificate carrying such an extension, and RFC 5280 4.2.1.10 says conforming CAs must not issue one.
The change
GeneralSubtrees ::= SEQUENCE SIZE (1..MAX)constrains both fields, so this rejects a present-but-emptypermittedSubtreesand a present-but-emptyexcludedSubtrees. Only the first has any consequence; an emptyexcludedSubtreesexcludes nothing, which is harmless. But it is the same ASN.1 requirement on the same type, and enforcing it for one field and not the other seemed arbitrary. Happy to drop the second check if you would rather keep the diff to the field that matters.The existing both-empty check is kept. I checked whether it became redundant: it does not. It is the only one that covers a
NameConstraintswhere neither field is present, whichis_some_andby construction never matches.Behaviour
The probe from the advisory, through the public API. The CA's
NameConstraintshas to be built as raw DER, because the Python API refuses to construct this shape at all, which is itself part of the point.Before, against 50.0.1 from PyPI:
After, with this branch:
The second line is the control, and it is unchanged.
Tests
Three tests in
policy::extension::tests, using raw DER because an empty sequence cannot be built through the writing API: emptypermittedSubtreeswith non-emptyexcludedSubtrees, emptyexcludedSubtreeswith non-emptypermittedSubtrees, and a control with both non-empty that must still be accepted.Verified they fail without the fix: reverting the check and keeping the tests turns both rejection tests red while the control stays green.
Checks
cargo test --allgreen across the workspace, including the 43 tests incryptography-x509-verificationcargo fmt --all -- --checkcleancargo clippy -p cryptography-x509-verification --all-targets: 8 warnings, the same 8 as onmain, none on the changed linespytest tests/: 4490 passed, 199 skipped (wycheproof_rootandx509_limbo_rootnot available locally)