tls_codec: Read/write all available data in quic_vec

[pcapriotti]

Read::read is not guaranteed to fill the whole buffer, and similarly, Write::write is not guaranteed to write everything. This commit replaces them with read_exact and write_all respectively, and removes the corresponding assertions.

[franziskuskiefer]

How do you ensure that fuzzing still works here?

[pcapriotti]

Sorry, I don't quite understand what the problem with fuzzing is here. Can you clarify?

[franziskuskiefer]

Sorry, I don't quite understand what the problem with fuzzing is here. Can you clarify?

Sure (sorry for the slow response).
Doing an exact read/write will always fail when fuzzing on a higher level. I guess we need to add something in here to see how this behaves.
Can you check how this works on a simple fuzzing target like the following?

#[derive(Deserialize, TlsSize)]
struct MyStruct {
    m: u8,
    o: Vec<u8>,
}

fuzz_target!(|data: &[u8]| {
    let _ = MyStruct::tls_deserialize(&mut &data[..]);
});

[franziskuskiefer]

Closing this. Let me know if you want to reopen the discussion @pcapriotti.

[augustocdias]

Hello @franziskuskiefer
I have just tried the snipped you posted and it worked fine. Should it fail?

Could you share your concerns on this so we could address them and have this merged?

[franziskuskiefer]

Right now there's no fuzzing in the crate here. We should add a simple target (like that snippet). And run a couple iterations on CI.
I'd have to look at the code again, but the question was whether the code is still nicely fuzzeable with these changes.

[stefanwire]

Hi @franziskuskiefer,

I propose to reopen and merge this PR.

The patch proposed by @pcapriotti fixes the issue created by the use of read() in tls_codec/src/quic_vec.rs which may stop reading from the reader before filling the entire buffer. This behaviour is documented and it depends on the environment (OS, etc). So, it cannot be captured in a unit test to the best of my knowledge. Quoting the relevant paragraph of the current documentation:

It is not an error if the returned value n is smaller than the buffer size, even when the reader is not at the end of the stream yet. This may happen for example because fewer bytes are actually available right now (e. g. being close to end-of-file) or because read() was interrupted by a signal.

When read() stops before filling up the buffer to the end, it needs to be called again in a loop until the buffer is filled. This could be done manually, but it already is implemented in read_exact() which was also proposed by @pcapriotti.

As to fuzzing, I think that should be a side show, considering that the issue at hand already bleeds into production systems. Important, but not first priority, imho.

Thanks! 🙏

[franziskuskiefer]

Nothing keeps you from reopening the PR.
But what's in here is outdated and will need to be rebased/rewritten. Adding a fuzzing target can be quickly done so I don't see a reason not to do that.
The reason this is closed is because of inactivity. I'm happy to review and merge any such changes.

[stefanwire]

I probably can rebase the code, but I don't think I can reopen the PR. 🤔

[stefanwire]

Merging should be possible without rebasing, since there are no conflicts with more recent commits. If still relevant, I'm probably obtaining the permissions to push to Paolo's branch in the course of the day.

[stefanwire]

The PR is now rebased onto the current upstream master branch. @franziskuskiefer

[franziskuskiefer]

The PR is now rebased onto the current upstream master branch. @franziskuskiefer

Thanks, can you add the fuzzing target?

Also, I had a quick look again at this and don't see any fix in this. The checks are manual for easier handling of fuzzing targets. The main difference is that with the change you can read from streams that don't give you all available data right away.

[franziskuskiefer]

I'll fix this up later.

[stefanwire]

Also, I had a quick look again at this and don't see any fix in this. The checks are manual for easier handling of fuzzing targets. The main difference is that with the change you can read from streams that don't give you all available data right away.

So, what the library currently done is (1) attempt to read and (2) fail if not enough bytes are read, even if more bytes could be read. What should be done instead is repeatedly read until all bytes are read.

To be clear, this is a bug which has the potential to universally break protocols and it cannot be fixed from the outside of this library. The bug is fixed by this PR.

Thanks, can you add the fuzzing target?

I'm not feeling comfortable enough in Rust yet to add new test features. There's no fuzzing target so for, is there? Also, bug fixes should go first, imho.

Thanks!