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

rustls: fix handling of EOF on read #8003

Closed
wants to merge 2 commits into from
Closed

rustls: fix handling of EOF on read #8003

wants to merge 2 commits into from

Conversation

@jsha
Copy link
Contributor

@jsha jsha commented Nov 13, 2021

The update to rustls-ffi 0.8.0 changed handling of EOF and close_notify.
From the CHANGELOG:

Handling of unclean close and the close_notify TLS alert. Mirroring upstream changes,
a rustls_connection now tracks TCP closed state like so: rustls_connection_read_tls
considers a 0-length read from its callback to mean "TCP stream was closed by peer."
If that happens before the peer sent close_notify, rustls_connection_read will return
RUSTLS_RESULT_UNEXPECTED_EOF once the available plaintext bytes are exhausted. This is
useful to protect against truncation attacks. Note:
some TLS implementations don't send close_notify. If you are already getting length
information from your protocol (e.g. Content-Length in HTTP) you may choose to
ignore UNEXPECTED_EOF so long as the number of plaintext bytes was as
expected.

That means we don't need to check for unclean EOF in cr_recv(), because
process_new_packets() will give us an error if appropriate.

Also, we had code in cr_recv that would return immediately if we got a read of zero
bytes, but actually what we should do in that case is break out of the loop, and
return either CURLE_OK or CURLE_AGAIN depending on whether we had read
some plaintext on a previous iteration.

The update to rustls-ffi 0.8.0 changed handling of EOF and close_notify.
From the CHANGELOG:

> Handling of unclean close and the close_notify TLS alert. Mirroring upstream changes,
> a rustls_connection now tracks TCP closed state like so: rustls_connection_read_tls
> considers a 0-length read from its callback to mean "TCP stream was closed by peer."
> If that happens before the peer sent close_notify, rustls_connection_read will return
> RUSTLS_RESULT_UNEXPECTED_EOF once the available plaintext bytes are exhausted. This is
> useful to protect against truncation attacks. Note:
> some TLS implementations don't send close_notify. If you are already getting length
> information from your protocol (e.g. Content-Length in HTTP) you may choose to
> ignore UNEXPECTED_EOF so long as the number of plaintext bytes was as
> expected.

That means we don't need to check for unclean EOF in `cr_recv()`, because
`process_new_packets()` will give us an error if appropriate.
@jsha jsha changed the title rustls: remove incorrect EOF check rustls: fix handling of EOF on read Nov 13, 2021
When we're reading out plaintext from rustls' internal buffers, we might
get a read of zero bytes (meaning a clean TCP close, including
close_notify). However, we shouldn't return immediately when that
happens, since we may have already copied out some plaintext bytes.
Break out of the loop when we get a read of zero bytes, and figure out
which path we're dealing with.
@bagder
Copy link
Member

@bagder bagder commented Nov 13, 2021

@kevinburke, does it look good to you as well?

Loading

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Nov 13, 2021

Yes thanks!

Loading

@bagder bagder closed this in be8d77b Nov 13, 2021
bagder added a commit that referenced this issue Nov 13, 2021
When we're reading out plaintext from rustls' internal buffers, we might
get a read of zero bytes (meaning a clean TCP close, including
close_notify). However, we shouldn't return immediately when that
happens, since we may have already copied out some plaintext bytes.
Break out of the loop when we get a read of zero bytes, and figure out
which path we're dealing with.

Acked-by: Kevin Burke

Closes #8003
@bagder
Copy link
Member

@bagder bagder commented Nov 13, 2021

Thanks both!

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants