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: s2n_shutdown should handle partial records #4421

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

lrstewart
Copy link
Contributor

Description of changes:

Fix yet another shutdown bug :/

When our bindings test failed, we realized that s2n_shutdown couldn't handle malformed records left in the input stuffers. I fixed that by just wiping the input stuffers: #4350

However, that broke another case we don't have tests for: If we have a partial record, we need to continue reading it rather than just wiping it. For example, we might have read just a header. If we wipe that header and attempt to start reading over, we'll be reading fragment data as header data. Since we instruct applications to call s2n_recv until it blocks, this is likely to be a common case. This is also unfortunately difficult to distinguish from "malformed record" from just the state of the input stuffers: we might have partial header data because of a partial read, or because we failed halfway through parsing the header, or both.

I fixed this problem by making two changes:

  1. I fixed the original issue (s2n_shutdown after a malformed record) differently. Now we just don't attempt to read at all if we ran into an issue with record parsing or decryption. That makes more sense anyway: we're probably not going to be able to recover from that kind of failure enough to successfully read the next record. The original bindings test could go on to successfully receive the close_notify because the malformed record was 0-length so could just be skipped without consequence. That's an unrealistic edge case.
  2. I switched to only wiping the inputs if the pending record has been decrypted but not fully consumed, which means that it's application data that the application hasn't finished draining yet. This was previously covered by an older version of s2n_shutdown.

Call-outs:

Are there any other possible states for the record stuffers that s2n_shutdown could encounter? I keep missing cases :/ Please suggest states even if you think they're impossible / nonsensical. Let's get creative. s2n_shutdown is difficult because unlike other APIs, it is supposed to be called after an error so can't necessarily rely on sane state.

Testing:

I modified the existing test from #4350 to reflect the new behavior.
The possible states before s2n_shutdown is called that I'm now covering:

  • No record (plenty of existing tests cover this)
  • Malformed record (updated test)
  • Partial but valid record (new test)
  • Partial and invalid record (new test)
  • Partial close_notify (new test)
  • Decrypted, partially consumed app data record (new 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 15, 2024
@lrstewart
Copy link
Contributor Author

The rust failures are due to an ongoing, unrelated issue.
The rust tests pass locally (including shutdown_with_blinding), so I'm going to mark ready for review.

@lrstewart lrstewart marked this pull request as ready for review February 15, 2024 18:20
tls/s2n_shutdown.c Outdated Show resolved Hide resolved
tests/unit/s2n_shutdown_test.c Show resolved Hide resolved
@lrstewart lrstewart enabled auto-merge (squash) February 16, 2024 19:37
@lrstewart lrstewart merged commit 20010e6 into aws:main Feb 16, 2024
31 checks passed
@lrstewart lrstewart deleted the shutdown branch February 16, 2024 22:40
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