-
Notifications
You must be signed in to change notification settings - Fork 722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check to s2n_signature_scheme_valid_to_accept #3728
Merged
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
TODO: anwser open question about code in to_offer.
lrstewart
reviewed
Jan 3, 2023
harrisonkaiser
commented
Jan 4, 2023
goatgoose
reviewed
Jan 4, 2023
lrstewart
reviewed
Jan 4, 2023
goatgoose
approved these changes
Jan 4, 2023
lrstewart
approved these changes
Jan 4, 2023
goatgoose
approved these changes
Jan 5, 2023
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.
Resolved issues:
#1686
Description of changes:
This PR adds a check to the function
s2n_signature_scheme_valid_to_accept
which is used both in the negotiation and and validation phases of s2n's signature algorithm/scheme handling.Specifically this check ensures the following for TLS1.3:
For TLS1.2 and before there are slightly different requirements:
These checks mirror those in: tests/unit/s2n_security_policies_test.c. By moving them to runtime we gain a greater level of assurance that our checks aren't violated. For instance we can check against the actual_protocol_version.
These checks rely on the actual_protocol_version being set therefore it needed to be added in a few unit tests.
Call-outs/Questions:
Should these checks be in
s2n_signature_scheme_valid_to_offer
? It seems to me that either place is sufficient as both calls have to pass in order for the scheme to be used in a negotiation.Quite a few tests don't set the
actual_protocol_version_established
flag, despite setting theactual_protocol_version
. Is this merely a quirk of the tests or does theactual_protocol_version_established
flag have some non-obvious meaning s.t. the stateactual_protocol_version != 0 && actual_protocol_version_established == 0
has a meaning?This is a partial implementation of the 3rd option listed in #1686:
By keeping the min/max protocol version on the sig schemes we keep the transparency; while safe guarding against the worst possible outcomes. The intent of this PR is to make it possible to remove the min/max protocol version in a future PR should we choose to.
It is also a implementation of the 1st option listed in #1686:
s2n_client_cert_verify_recv
ands2n_tls13_cert_verify_recv
both calls2n_get_and_validate_negotiated_signature_scheme
which will only choose a candidate scheme for whichs2n_signature_scheme_valid_to_accept
is 0 (meaning it passes the checks).Testing:
Ran unit tests locally + CI.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.