From fbe143b142875977f49772d2905029b57b92e429 Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Tue, 21 Jan 2020 19:35:13 +0100 Subject: fix: Prevent undefined behaviour from malicious AsyncRead impl (#2030) `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())` --- tokio/src/io/async_read.rs | 4 +++- tokio/tests/io_read.rs | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/tokio/src/io/async_read.rs b/tokio/src/io/async_read.rs index d28280a5..24c1b4ef 100644 --- a/tokio/src/io/async_read.rs +++ b/tokio/src/io/async_read.rs @@ -123,7 +123,9 @@ pub trait AsyncRead { // Convert to `&mut [u8]` let b = &mut *(b as *mut [MaybeUninit] 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"); + n }; buf.advance_mut(n); diff --git a/tokio/tests/io_read.rs b/tokio/tests/io_read.rs index d18615e4..4791c9a6 100644 --- a/tokio/tests/io_read.rs +++ b/tokio/tests/io_read.rs @@ -36,3 +36,25 @@ async fn read() { assert_eq!(n, 11); assert_eq!(buf[..], b"hello world"[..]); } + +struct BadAsyncRead; + +impl AsyncRead for BadAsyncRead { + fn poll_read( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + buf: &mut [u8], + ) -> Poll> { + for b in &mut *buf { + *b = b'a'; + } + Poll::Ready(Ok(buf.len() * 2)) + } +} + +#[tokio::test] +#[should_panic] +async fn read_buf_bad_async_read() { + let mut buf = Vec::with_capacity(10); + BadAsyncRead.read_buf(&mut buf).await.unwrap(); +} -- cgit v1.2.3