Skip to content
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

fix: prevent enabling ktls with a buffered record header fragment #4426

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 18, 2024

Description of changes:

Minor ktls fix. We shouldn't enable ktls with ANY unhandled record data, including a partial header.
conn->header_in could contain data but conn->in not contain data if we blocked while trying to read the full record.

(Turning on ktls switches s2n-tls from parsing encrypted records to decrypted application data, so any partial data won't be handled right after the switch)

Callouts

There are a couple other places where we make similar checks:

  • s2n_renegotiate_wipe, where we also check for "pending data" and fail if it exists. We already check header_in there.
  • s2n_connection_release_buffers, where we check our IO buffers. However, not checking header_in is correct there. We're just releasing buffers for memory savings, and header_in doesn't alloc memory. Even if a partial header exists in header_in, we'll be just fine continuing the partial read that created it.

Testing:

Updated existing test

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

@github-actions github-actions bot added the s2n-core team label Feb 18, 2024
@lrstewart lrstewart marked this pull request as ready for review February 19, 2024 15:54
Comment on lines +106 to 107
RESULT_ENSURE(s2n_stuffer_data_available(&conn->header_in) == 0, S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);
RESULT_ENSURE(s2n_stuffer_data_available(&conn->in) == 0, S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use s2n_stuffer_is_consumed? It would avoid the the == 0 and read better. Some of the tests could also be simplified.

Suggested change
RESULT_ENSURE(s2n_stuffer_data_available(&conn->header_in) == 0, S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);
RESULT_ENSURE(s2n_stuffer_data_available(&conn->in) == 0, S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);
RESULT_ENSURE(s2n_stuffer_is_consumed(&conn->header_in), S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);
RESULT_ENSURE(s2n_stuffer_is_consumed(&conn->in), S2N_ERR_RECORD_STUFFER_NEEDS_DRAINING);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two methods are equivalent. However, from looking at how both are used, s2n_stuffer_is_consumed seems to be used when the memory associated with the buffer is the concern, and s2n_stuffer_data_available seems to be used when whether we can read from the buffer is the concern. From that perspective, we're using the right method here. Dropping "==0" isn't much of a gain, and s2n_stuffer_is_consumed doesn't simplify the tests at all.

@lrstewart lrstewart enabled auto-merge (squash) February 23, 2024 12:16
@lrstewart lrstewart merged commit fd30be6 into aws:main Feb 23, 2024
31 checks passed
@lrstewart lrstewart deleted the fix3 branch March 4, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants