Skip to content

Comments

fix(psk): Prevent internal error with uninitialized PSK#5676

Open
Ignacio-Trottaa wants to merge 1 commit intoaws:mainfrom
Ignacio-Trottaa:issue-5085-fix
Open

fix(psk): Prevent internal error with uninitialized PSK#5676
Ignacio-Trottaa wants to merge 1 commit intoaws:mainfrom
Ignacio-Trottaa:issue-5085-fix

Conversation

@Ignacio-Trottaa
Copy link

fix: Handle uninitialized PSK in choose_psk

When s2n_offered_psk_list_choose_psk was called with an s2n_offered_psk
struct that was allocated but not initialized, it would result in a
NULL pointer dereference and an internal error.

This change adds a check to ensure the psk's identity data is not
NULL before proceeding, returning a usage error (S2N_ERR_NULL) instead.
A unit test is also added to verify this behavior.

Fixes: #5085

Goal

Prevent an internal error when s2n_offered_psk_list_choose_psk is called with an uninitialized PSK.

Why

Calling the function with a PSK that has a NULL identity data pointer causes a crash. The function should fail gracefully with a usage error instead of an internal one, improving the library's robustness. This addresses issue #5085.

How

A POSIX_ENSURE_REF check is added at the beginning of the function to validate that psk->identity.data is not NULL. If it is, the function now returns S2N_ERR_NULL.

Callouts

None. The fix is straightforward.

Testing

A new unit test, s2n_offered_psk_list_choose_psk_with_uninitialized_psk, was added. This test replicates the failing scenario and asserts that the function now correctly returns a failure with the S2N_ERR_NULL error code. All existing tests pass.

Related

Fixes #5085

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When s2n_offered_psk_list_choose_psk was called with an s2n_offered_psk
struct that was allocated but not initialized, it would result in a
NULL pointer dereference and an internal error.

This change adds a check to ensure the psk's identity data is not
NULL before proceeding, returning a usage error (S2N_ERR_NULL) instead.
A unit test is also added to verify this behavior.

Fixes: aws#5085
@CarolYeh910
Copy link
Contributor

Hi @Ignacio-Trottaa, thanks for your contribution! The testing section mentioned you added a new unit test, but I didn't see it in the file diff. Do you plan to add the test to this PR? Your fix looks correct though!

@Ignacio-Trottaa
Copy link
Author

Hi @CarolYeh910, thanks for your feedback! Sorry about the unit test; I mentioned I uploaded it, but... I forgot to upload it along with the correction.

@maddeleine
Copy link
Contributor

Hey are you still working on this? We don't want to merge this without a unit test. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

better error when calling s2n_offered_psk_list_choose_psk with empty PSK

3 participants