stream: avoid swallowing unexpected eof condition. #1299

cpu · 2023-05-18T18:58:09Z

Description

Previously the Stream and OwnedStream type's read and read_buf functions had code to try and "prematurely signal EOF" by returning Ok(0) if no bytes were read from the underlying transport after calling complete_io.

In practice this has the effect of swallowing an error condition when the remote peer has closed their end of the connection without properly signalling their intent to do so. When this happens the client must read an UnexpectedEof error to prevent a truncation attack. Clients reading through a Stream type with the pre-existing code would always read a normal EOF, whereas using a connection directly without the stream abstraction would present the correct error.

There doesn't appear to be a reason to "prematurely" signal anything - if we read 0 bytes from complete_io() we should instead break our wants_read() loop to allow execution to continue to a self.conn.reader().read(...) or self.conn.reader().read_buf(...) call where the common connection code can properly handle this case (using the logic added in #790), turning the result into the desired UnexpectedEof error. Similarly there's no need to call process_new_packets() ourselves because complete_io() does this after reading TLS bytes.

Making this change resolves the problem of distinguishing unexpected EOF from expected EOF when using Stream or OwnedStream and doesn't appear to break any other functionality expected of the Stream types. I did some git spelunking and wasn't able to determine the original purpose of the premature signalling logic (It was added in c94c223 by @ctz.) but I suspect code churn elsewhere makes it no longer necessary. If I've missed an important detail I'm happy to revisit.

Testing

To emphasize the bug fix I've separated out the unit test as a commit added prior to the fix.

Before applying the fix an Ok 0 byte result from a read is returned where we expect an error:

$> cargo test --package rustls --all-features --test api 'unclean_close'

running 4 tests
test client_streamowned_read_unclean_close ... FAILED
test client_streamowned_readbuf_unclean_close ... FAILED
test client_stream_readbuf_unclean_close ... FAILED
test client_stream_read_unclean_close ... FAILED

failures:

---- client_streamowned_read_unclean_close stdout ----
thread 'client_streamowned_read_unclean_close' panicked at 'called `Result::unwrap_err()` on an `Ok` value: 0', rustls/tests/api.rs:189:37

---- client_streamowned_readbuf_unclean_close stdout ----
thread 'client_streamowned_readbuf_unclean_close' panicked at 'called `Result::unwrap_err()` on an `Ok` value: ()', rustls/tests/api.rs:211:10

---- client_stream_readbuf_unclean_close stdout ----
thread 'client_stream_readbuf_unclean_close' panicked at 'called `Result::unwrap_err()` on an `Ok` value: ()', rustls/tests/api.rs:211:10

---- client_stream_read_unclean_close stdout ----
thread 'client_stream_read_unclean_close' panicked at 'c
8000
alled `Result::unwrap_err()` on an `Ok` value: 0', rustls/tests/api.rs:189:37
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    client_stream_read_unclean_close
    client_stream_readbuf_unclean_close
    client_streamowned_read_unclean_close
    client_streamowned_readbuf_unclean_close

test result: FAILED. 0 passed; 4 failed; 0 ignored; 0 measured; 134 filtered out; finished in 0.00s

After applying the fix the tests begin to pass:

$> cargo test --package rustls --all-features --test api 'unclean_close'

running 4 tests
test client_stream_readbuf_unclean_close ... ok
test client_stream_read_unclean_close ... ok
test client_streamowned_readbuf_unclean_close ... ok
test client_streamowned_read_unclean_close ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 134 filtered out; finished in 0.01s

Resolves #1188, tokio-rs/tls#128, tokio-rs/tls#141

codecov · 2023-05-18T19:05:01Z

Codecov Report

Merging #1299 (fbe7859) into main (792045b) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1299      +/-   ##
==========================================
+ Coverage   95.55%   95.78%   +0.23%     
==========================================
  Files          60       60              
  Lines       14351    14339      -12     
==========================================
+ Hits        13713    13735      +22     
+ Misses        638      604      -34

Impacted Files	Coverage Δ
rustls/src/stream.rs	`69.66% <100.00%> (+18.17%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rustls/src/stream.rs

This commit introduces a unit tests that verify that a `Stream` or `StreamOwned` that receives an EOF from a peer that hasn't signalled the intent to close should read an `UnexpectedEOF` error. Additionally, since there was no existing coverage for `read_buf` this is added along with some small helpers.

Previously the `Stream` and `OwnedStream` types `read` and `read_buf` functions had code to try and "prematurely signal EOF" by returning `Ok(0)` if no bytes were read from the underlying transport. In practice this has the effect of swallowing an error condition when the remote peer has closed their end of the connection without properly signalling their intent to do so. When this happens the client must read an `UnexpectedEof` error to prevent a truncation attack. With the pre-existing code this condition is instead presented as a normal EOF. There doesn't appear to be a reason to "prematurely" signal anything - if we read 0 bytes from `complete_io()` we should instead break our `wants_read()` loop to allow execution to continue to a `self.conn.reader().read(...)` or `self.conn.reader().read_buf(...)` call where the common connection code can properly handle this case, turning the result into the desired `UnexpectedEof` error. This resolves the problem of distinguishing unexpected EOF from expected EOF and does not appear to break any other functionality expected of the `Stream` types.

cpu · 2023-05-23T12:41:15Z

@djc OK if I merge this one?

jsha · 2023-05-26T23:21:51Z

FYI I did a little additional git spelunking (git log -S prematurely) and found that the comment about "prematurely signal EOF" was #159 from 2018. Specifically the intent of that PR was to prevent prematurely returning 0. But I can see how the "Otherwise, we will prematurely signal EOF" comment winds up being read as something the code is trying to achieve rather than something it is trying to avoid doing.

cpu self-assigned this May 18, 2023

cpu mentioned this pull request May 18, 2023

stream: remove premature EOF signal. #1293

Closed

djc approved these changes May 18, 2023

View reviewed changes

kamadorueda reviewed May 20, 2023

View reviewed changes

rustls/src/stream.rs Show resolved Hide resolved

cpu added 2 commits May 20, 2023 12:03

cpu force-pushed the cpu-1188-stream-unexpected-eof-fix branch from 5eca6da to fbe7859 Compare May 20, 2023 16:07

djc merged commit ab685b5 into rustls:main May 23, 2023

cpu deleted the cpu-1188-stream-unexpected-eof-fix branch May 23, 2023 13:19

This was referenced May 23, 2023

Getting UnexpectedEof on read_to_end while the sync version works fine tokio-rs/tls#141

Closed

Handle EOF like rustls::Stream tokio-rs/tls#128

Closed

tests: start on de-duplicating stream API tests. #1306

Merged

djc mentioned this pull request Sep 8, 2023

rustls::Stream not working on some domains with rustls 0.21.7 #1452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

stream: avoid swallowing unexpected eof condition. #1299

stream: avoid swallowing unexpected eof condition. #1299

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh o 38CA h!

stream: avoid swallowing unexpected eof condition. #1299

stream: avoid swallowing unexpected eof condition. #1299

Uh oh!

Conversation

Uh oh!

Description

Testing

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh o 38CA h!