fix: add const correctness to S2N_BLOB_LABEL macro #5566
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
The S2N_BLOB_LABEL macro creates const blob structures but the underlying data array was not marked const, breaking const-correctness. This allows the data to be modified even though the blob is const. Changes: - Mark the data array as const in S2N_BLOB_LABEL macro - Add explicit cast to maintain compatibility with non-const data pointer This ensures that blobs created with S2N_BLOB_LABEL have truly immutable data, preventing accidental modifications to what should be constant string literals used throughout the TLS 1.3 implementation. File changed: - utils/s2n_blob.h (line 72-75: added const to data array)
Contributor
|
Hello @akshatsinha0, Thanks for cutting these PRs to us! I will assign reviewer for your PRs. |
Contributor
|
Hey there! It looks like your test is failing CI, unfortunately. I haven't looked at this part of the codebase deeply, but could you expand on the why of this PR, and also fix the failing CI? |
Author
|
@boquan-fang |
akshatsinha0
force-pushed
the
fix/blob-label-const-correctness
branch
from
e6c1883 to
131f3f2
Compare
Author
|
If the update doesnot work, I will close this pr becaue originally const and non-const desings both work |
Contributor
|
Hey @akshatsinha0 ! Are you still interested in working on this? We'll go ahead and close this next week if there isn't a response. |
Author
|
@jmayclin |
I) Introduced struct s2n_ro_blob and S2N_RO_BLOB_LABEL. II) Added s2n_hkdf_expand_label_ro for const labels. III) Migrated TLS 1.3 static labels and call sites to the RO path. IV) Kept S2N_BLOB_LABEL for mutable use cases. Signed-off-by: Akshat Sinha
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.
The S2N_BLOB_LABEL macro creates const blob structures but the underlying data array was not marked const, breaking const-correctness. so the data will be modified even though the blob is const. CHange the data array as const in S2N_BLOB_LABEL macro & add cast to maintain surity with non-const data pointer
Release Summary:
Fixes const-correctness in S2N_BLOB_LABEL macro to prevent modification of immutable TLS protocol label strings.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? What manual testing was performed? Are there any testing steps to be verified by the reviewer?
How can you convince your reviewers that this PR is safe and effective?
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
all tests using S2N_BLOB_LABEL validate this change (s2n_tls13_prf_test.c, s2n_tls13_record_aead_test.c, all TLS 1.3 tests using labels from crypto/s2n_tls13_keys.c)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.