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

s2n-tls poll_flush doesn't work #4803

Open
lrstewart opened this issue Sep 27, 2024 · 0 comments
Open

s2n-tls poll_flush doesn't work #4803

lrstewart opened this issue Sep 27, 2024 · 0 comments

Comments

@lrstewart
Copy link
Contributor

lrstewart commented Sep 27, 2024

Problem:

poll_flush looks like it should work: it just calls s2n_send with a zero-length buffer, which is a standard way to flush IO.

However, s2n-tls has some unusual s2n_send expectations: when re-trying a send, you MUST send the same buffer. This is checked in code, and s2n_send will error if the buffer size doesn't match up with any data already read:

s2n-tls/tls/s2n_send.c

Lines 173 to 174 in cf91291

/* Defensive check against an invalid retry */
POSIX_ENSURE(conn->current_user_data_consumed <= total_size, S2N_ERR_SEND_SIZE);
That means that if poll_flush uses a zero-length buffer, it will flush but then produce an error.

Solution:

  • We could write a real s2n_flush method?
  • We could ignore any fatal errors from s2n_send? But that seems risky, since they could be fatal errors while flushing.

I'm leaning towards a real s2n_flush method. We can put it in s2n_internal.h if necessary.

The s2n_send retry validation behavior also doesn't seem well documented. We should improve the documentation as part of this task.

Also, careful: audit everywhere we check whether the send buffer is empty or not. We might mean that as a shorthand for "is there a pending send", which a real flush method would break. This is definitely a problem for s2n_renegotiate_wipe.

  • Does this change what S2N sends over the wire? No
  • Does this change any public APIs? Makes an existing Rust api actually work
  • Which versions of TLS will this impact? All

Requirements / Acceptance Criteria:

What must a solution address in order to solve the problem? How do we know the solution is complete?

  • RFC links: Links to relevant RFC(s)
  • Related Issues: Link any relevant issues
  • Will the Usage Guide or other documentation need to be updated?
  • Testing: How will this change be tested? Call out new integration tests, functional tests, or particularly interesting/important unit tests.
    • Will this change trigger SAW changes? Changes to the state machine, the s2n_handshake_io code that controls state transitions, the DRBG, or the corking/uncorking logic could trigger SAW failures.
    • Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.

Out of scope:

Is there anything the solution will intentionally NOT address?

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

No branches or pull requests

2 participants