verify_cert: bound name constraint comparisons. #165
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 96.61% 96.69% +0.07%
==========================================
Files 15 15
Lines 4463 4570 +107
==========================================
+ Hits 4312 4419 +107
Misses 151 151
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cpu
force-pushed
the
cpu-bound-name-constraint-checks
branch
from
80e080e
to
47e2de1
Compare
djc
reviewed
Sep 6, 2023
cpu
force-pushed
the
cpu-bound-name-constraint-checks
branch
from
47e2de1
to
131c680
Compare
This commit renames the `check_signatures` fn, as it is doing more than simply verifying signatures. It also checks revocation status w/ CRLs when appropriate.
cpu
force-pushed
the
cpu-bound-name-constraint-checks
branch
from
131c680
to
870b7fa
Compare
djc
approved these changes
Sep 6, 2023
ctz
reviewed
Sep 6, 2023
This commit updates the name constraint validation done during path building to apply a budget for the maximum allowed number of name constraint checks. We use the same limit that golang crypto/x509 applies by default: 250,000 comparisons. Note: this commit applies the budget during path building in a manner that means certificates _not_ part of the built path can consume comparisons from the budget even though they will not be present in the complete validated path. Similarly name constraints are evaluated before signatures, meaning a certificate that doesn't verify to a trusted root still has its constraints parsed and evaluated. A subsequent commit will adjust these shortcomings.
This commit updates the path building process such that name constraints are only evaluated against a complete path where signatures on the chain have been checked successfully to a trust anchor. This avoids: * Parsing name constraints before signatures are validated. * Evaluating name constraints and consuming name constraint comparison budget for certificates that are not part of the built path. In the future it could be possible to interleave the name constraint checking with the signature checking, however the logic for this is more complicated. For an initial fix let's prefer a simpler solution that walks the built + validated path to check name constraints from the trust anchor to the end entity certificate.
cpu
force-pushed
the
cpu-bound-name-constraint-checks
branch
from
870b7fa
to
a90f075
Compare
ctz
approved these changes
Sep 7, 2023
This was referenced Sep 1, 2023
Closed
Merged
Merged
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.
Enforcing name constraints during path building is quadratic with the number of names, and the number of name constraints. To avoid the possibility of excessive CPU consumption enforcing name constraints TLS libraries typically enforce a maximum number of comparisons while evaluating a constrained certificate against subsequent certificate names. Note that unlike #152 the possible runtime of crafted certificate chains within the limits imposed by Rustls/webpki prior to this commit is O(seconds) and not O(hours+), making this a less grave concern generally.
In this branch we move name constraint validation to after the path signatures have been successfully validated, limiting the exposure of name constraint processing to certificates that chain to a known trust anchor. We also adopt a limit for name constraint comparisons inspired by the default limit used by Go's x509 package. However, unlike Go we bound total comparisons for verifying a validated path meets name constraints to 250,000 comparisons overall, not per-certificate in the built path. Since we're applying this check only on a path that has been validated successfully from end entity to trust anchor, with a depth of at most 6 issuers, it seems like a reasonable limit.
Along the way (I recommend reviewing commit-by-commit) I performed a little bit of tidying.
As mentioned in the final commit it is possible to interleave the name constraint validation and the signature validation, but it will require a more complex algorithm and, to be honest, I've had trouble wrapping my head around the logic as-is (there's a lot of recursion, parsing, and iteration involved). This is a pretty complicated intersection of critical features and I think we should tread carefully and start simple.