-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: Prevent undefined behaviour from malicious AsyncRead impl #2030
fix: Prevent undefined behaviour from malicious AsyncRead impl #2030
Conversation
`AsyncRead` is safe to implement but can be implemented so that it reports that it read more bytes than it actually did. `poll_read_buf` on the other head implicitly trusts that the returned length is actually correct which makes it possible to advance the buffer past what has actually been initialized. An alternative fix could be to avoid the panic and instead advance by `n.min(b.len())`
@@ -123,7 +123,9 @@ pub trait AsyncRead { | |||
// Convert to `&mut [u8]` | |||
let b = &mut *(b as *mut [MaybeUninit<u8>] as *mut [u8]); | |||
|
|||
ready!(self.poll_read(cx, b))? | |||
let n = ready!(self.poll_read(cx, b))?; | |||
assert!(n <= b.len(), "Bad AsyncRead implementation, more bytes were reported as read than the buffer can hold"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should be debug assert.
Writing bad implemention is unlikely to be intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to be enforced at all times, since it needs to be upheld to prevent UB. Similar to bounds checking slices. Conversely though it might be optimized away for many cases!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we do not.
If we do not trust the contract, then this trait itself is UB and should be marked as unsafe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It specifically needs to be checked here because it's being passed to advance_mut
below, which itself is an unsafe fn similar to Vec::set_len
. Consider if this using a slice instead:
let n = ready!(self.poll_read(cx, &mut buf))?;
let data = &buf[..n];
Accessing the data from the slice (&buf[..n]
) would be similarly bounds checked, even if the implementation of poll_read
was incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the contract of BufMut::advance_mut
does seem to prevent this scenario:
Panics
This function may panic if cnt > self.remaining_mut().
Implementer notes
It is recommended for implementations of advance_mut to panic if cnt > self.remaining_mut(). If the implementation does not panic, the call must behave as if cnt == self.remaining_mut().
A call with cnt == 0 should never panic and be a no-op.
@Marwes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't panic and the Vec
passed here will call reserve
to expand to the correct length. But it will also follow that by calling set_len
https://docs.rs/bytes/0.5.3/src/bytes/buf/buf_mut.rs.html#999 which then allows uninitialized bytes to be accessed.
Separately, the advance_mut
implementation seems dubious. resize(cnt); set_len(len + cnt);
will always result in the Vec
exposing undefined values to safe callers.
Either panicking (as the doc suggests) or just trusting that the passed cnt
is correct and not calling reserve
seems better by either being safer or by avoiding the reserve
overhead.
This is a really interesting PR. On a similar note, is an implementation that incorrectly reports the length (without overflowing) to be considered sound? It's fairly straight forward to reason about zero-initialized buffers, because even if the read underflows, the contents of the buffer is well-defined at all points. Or an unsafe implementation which uses an uninitialized buffer and then makes sure that the length is appropriately updated. But here we have a situation where safe code has a fair amount of control over the argument to |
@DoumanAsh Reading uninitialized bytes from safe code without the assert! (which is why it can't be a debug_assert!). |
@Marwes Defensive programming is all good, but your scenario implies that trait itself is broken. I know there were like tons of useless convo about broken AsyncRead and Read traits, but I feel like this PR is just work-around for it. |
The trait isn't broken, but an implementation of it can be. Since neither the implementation of the trait, nor the caller of I am aware that this may not be the best way to fix this but fact of the matter is that it needs to be fixed in one way or another so I'd prefer suggestions for other, better fixes instead of sweeping it away. The current fixes that I am aware of is:
|
Since reading the panic note on the contract to Worst case it seems like
This would however cause uninitialized arrays of bytes to be propagated through the application, which might be UB, but of this I'm not sure. It's certainly a bug though. But this could also happen if an implementer of So in my view, the question is if returning wholly or partially uninitialized arrays of u8 is UB or not? If yes, it seems to me as if |
Opened tokio-rs/bytes#354 |
I was thinking about how this problem generalizes to std types and uninitialized buffers, and found this internals thread which is very much relevant: https://internals.rust-lang.org/t/uninitialized-memory/1652/10 @carllerche commented a fair bit in it, so maybe they can comment on this issue as well? |
This isn't an issue because |
Ah, that's right. Thanks for pointing that out! |
Any movement on this? Does not feel great to leave a soundness hole open, even if the trait is likely going to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, this got lost in my inbox...
AsyncRead
is safe to implement but can be implemented so that itreports that it read more bytes than it actually did.
poll_read_buf
onthe other hand implicitly trusts that the returned length is actually
correct which makes it possible to advance the buffer past what has
actually been initialized.
An alternative fix could be to avoid the panic and instead advance by
n.min(b.len())