Fix/add security policy validation checks #5565
Open
Conversation
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
Introduce a new error code S2N_ERR_ILLEGAL_PARAMETER to distinguish between unexpected message types (S2N_ERR_BAD_MESSAGE) and invalid message content. This improves TLS alert compliance by mapping the new error to S2N_TLS_ALERT_ILLEGAL_PARAMETER. Changes: - Add S2N_ERR_ILLEGAL_PARAMETER to error enum in s2n_errno.h - Add error description in s2n_errno.c - Map S2N_ERR_ILLEGAL_PARAMETER to S2N_TLS_ALERT_ILLEGAL_PARAMETER - Update comments to clarify error-to-alert mappings This resolves the TODO comment about incomplete alert mappings and provides a foundation for more precise error reporting throughout the codebase.
Add consistency checks for cipher_preferences, signature_preferences, and ecc_preferences to ensure count and list pointer are in sync, following the same pattern as existing kem_preferences validation. Changes: - Add s2n_validate_cipher_preferences() in s2n_security_policies.c - Add s2n_validate_signature_preferences() in s2n_security_policies.c - Add s2n_validate_ecc_preferences() in s2n_security_policies.c - Add function declarations to s2n_security_policies.h Each function validates that count is 0 iff the associated list is NULL, preventing inconsistent state in security policy configuration. Files changed: - tls/s2n_security_policies.c (lines 1866-1910: added 3 new functions) - tls/s2n_security_policies.h (lines 281-284: added 3 declarations)
jmayclin
reviewed
Oct 29, 2025
| return S2N_SUCCESS; | ||
| } | ||
|
|
||
| int s2n_validate_cipher_preferences(const struct s2n_cipher_preferences *cipher_preferences) |
Contributor
There was a problem hiding this comment.
I see that you're adding these functions, but I don't think they're actually getting called anywhere?
Comment on lines
+1881
to
+1892
| int s2n_validate_signature_preferences(const struct s2n_signature_preferences *signature_preferences) | ||
| { | ||
| POSIX_ENSURE_REF(signature_preferences); | ||
|
|
||
| /* checks to assert that the count is 0 */ | ||
| /* iff */ | ||
| /* the associated list is null */ | ||
| POSIX_ENSURE(S2N_IFF(signature_preferences->count == 0, signature_preferences->signature_schemes == NULL), | ||
| S2N_ERR_INVALID_SECURITY_POLICY); | ||
|
|
||
| return S2N_SUCCESS; | ||
| } |
Contributor
There was a problem hiding this comment.
Nit: I think we could make better use of the whitespace.
Suggested change
| int s2n_validate_signature_preferences(const struct s2n_signature_preferences *signature_preferences) | |
| { | |
| POSIX_ENSURE_REF(signature_preferences); | |
| /* checks to assert that the count is 0 */ | |
| /* iff */ | |
| /* the associated list is null */ | |
| POSIX_ENSURE(S2N_IFF(signature_preferences->count == 0, signature_preferences->signature_schemes == NULL), | |
| S2N_ERR_INVALID_SECURITY_POLICY); | |
| return S2N_SUCCESS; | |
| } | |
| int s2n_validate_signature_preferences(const struct s2n_signature_preferences *signature_preferences) | |
| { | |
| POSIX_ENSURE_REF(signature_preferences); | |
| POSIX_ENSURE(S2N_IFF(signature_preferences->count == 0, signature_preferences->signature_schemes == NULL), | |
| S2N_ERR_INVALID_SECURITY_POLICY); | |
| return S2N_SUCCESS; | |
| } |
| S2N_ERR_ENCRYPT = S2N_ERR_T_PROTO_START, | ||
| S2N_ERR_DECRYPT, | ||
| S2N_ERR_BAD_MESSAGE, | ||
| S2N_ERR_ILLEGAL_PARAMETER, |
Contributor
There was a problem hiding this comment.
I think this is unrelated to the security policy validation checks? I haven't totally read through what this is doing, but probably better to do in separate PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Release Summary:
Adds validation functions to prevent inconsistent security policy configs where preference counts and list pointers are out of sync.
Resolved issues:
N/A - Proactive bug prevention following existing validation pattern in s2n_validate_kem_preferences()
Description of changes:
Current behavior: Only
kem_preferencesvalidates that count is 0 iff the associated list is NULL. Other preference structures (cipher, signature, ecc) lack this validation, allowing inconsistent states.Now Added three validation funcns following the same pattern:-
(i)
s2n_validate_cipher_preferences()- validates cipher_preferences consistencysimilarly
(ii)
s2n_validate_signature_preferences()(iii)
s2n_validate_ecc_preferences()Call-outs:
these are validation functions only they are not yet called anywhere. Follow-up work should integrate these into security policy initialisation/config paths where
s2n_validate_kem_preferences()is currently called.Testing:
Existing test coverage: The pattern matches
s2n_validate_kem_preferences()which is tested in the security policies test suite.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.