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

Unsound BufWriter copy_to specialization with the unstable read_buf feature #93305

Closed
paolobarbolini opened this issue Jan 25, 2022 · 4 comments · Fixed by #97015
Closed

Unsound BufWriter copy_to specialization with the unstable read_buf feature #93305

paolobarbolini opened this issue Jan 25, 2022 · 4 comments · Fixed by #97015
Labels
C-bug Category: This is a bug.

Comments

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Jan 25, 2022

Code

#![feature(read_buf)]

use std::io::{self, BufWriter, Read, ReadBuf};

struct BadReader;

impl Read for BadReader {
    fn read(&mut self, _buf: &mut [u8]) -> io::Result<usize> {
        unreachable!()
    }

    fn read_buf(&mut self, read_buf: &mut ReadBuf<'_>) -> io::Result<()> {
        let vec = vec![0; 2048];

        let mut buf = ReadBuf::new(vec.leak());
        buf.append(&[123; 2048]);
        *read_buf = buf;

        Ok(())
    }
}

fn main() {
    let mut buf = Vec::new();

    {
        let mut buf_ = BufWriter::with_capacity(1024, &mut buf);

        let mut input = BadReader.take(4096);
        io::copy(&mut input, &mut buf_).unwrap();
    }

    // read the memory to trigger miri to realize that the memory is uninitialized
    println!("buf[0]: {}", buf[0]);
}

Playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=483f0b4f2f881519f459c3cd273a50f4 (run miri on it)

Unsound implementation

fn copy_to<R: Read + ?Sized>(reader: &mut R, writer: &mut Self) -> Result<u64> {
if writer.capacity() < DEFAULT_BUF_SIZE {
return stack_buffer_copy(reader, writer);
}
let mut len = 0;
let mut init = 0;
loop {
let buf = writer.buffer_mut();
let mut read_buf = ReadBuf::uninit(buf.spare_capacity_mut());
// SAFETY: init is either 0 or the initialized_len of the previous iteration
unsafe {
read_buf.assume_init(init);
}
if read_buf.capacity() >= DEFAULT_BUF_SIZE {
match reader.read_buf(&mut read_buf) {
Ok(()) => {
let bytes_read = read_buf.filled_len();
if bytes_read == 0 {
return Ok(len);
}
init = read_buf.initialized_len() - bytes_read;
// SAFETY: ReadBuf guarantees all of its filled bytes are init
unsafe { buf.set_len(buf.len() + bytes_read) };
len += bytes_read as u64;
// Read again if the buffer still has enough capacity, as BufWriter itself would do
// This will occur if the reader returns short reads
continue;
}
Err(ref e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e),
}
}
writer.flush_buf()?;
}
}

(we're basically tricking it into calling set_len with a length higher than what has actually been filled)

Documentation

Should this be considered a possible footgun when implementing something that calls read_buf? Should it be documented?

@paolobarbolini paolobarbolini added the C-bug Category: This is a bug. label Jan 25, 2022
@Urgau
Copy link
Member

Urgau commented Jan 25, 2022

This was already mention in the PR that added ReadBuf #81156 (comment)
cc @drmeepster @joshtriplett

@beepster4096
Copy link
Contributor

There's two possible solutions I see here.

  1. The easiest thing to do to fix this is just check if the ReadBuf points to the same buffer as before and return an error if it doesn't. This doesn't seem very "rusty" to me though, relying on a runtime check.
  2. The other solution is to have a ReadBufRef type passed by value to read_buf instead of &mut ReadBuf. This solves the issue in general at compile time, but could make this api a lot more complicated to use.

@nrc
Copy link
Member

nrc commented Apr 8, 2022

I think another example of unsound code in std for a similar reason is in the read_buf implementation for Take.

@paolobarbolini
Copy link
Contributor Author

I think another example of unsound code in std for a similar reason is in the read_buf implementation for Take.

Yes it seems like a similar attack could be done against it

rust/library/std/src/io/mod.rs

Lines 2584 to 2601 in 18f32b7

let mut sliced_buf = ReadBuf::uninit(ibuf);
// SAFETY: extra_init bytes of ibuf are known to be initialized
unsafe {
sliced_buf.assume_init(extra_init);
}
self.inner.read_buf(&mut sliced_buf)?;
let new_init = sliced_buf.initialized_len();
let filled = sliced_buf.filled_len();
// sliced_buf / ibuf must drop here
// SAFETY: new_init bytes of buf's unfilled buffer have been initialized
unsafe {
buf.assume_init(new_init);
}

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 28, 2022
std::io: migrate ReadBuf to BorrowBuf/BorrowCursor

This PR replaces `ReadBuf` (used by the `Read::read_buf` family of methods) with `BorrowBuf` and `BorrowCursor`.

The general idea is to split `ReadBuf` because its API is large and confusing. `BorrowBuf` represents a borrowed buffer which is mostly read-only and (other than for construction) deals only with filled vs unfilled segments. a `BorrowCursor` is a mostly write-only view of the unfilled part of a `BorrowBuf` which distinguishes between initialized and uninitialized segments. For `Read::read_buf`, the caller would create a `BorrowBuf`, then pass a `BorrowCursor` to `read_buf`.

In addition to the major API split, I've made the following smaller changes:

* Removed some methods entirely from the API (mostly the functionality can be replicated with two calls rather than a single one)
* Unified naming, e.g., by replacing initialized with init and assume_init with set_init
* Added an easy way to get the number of bytes written to a cursor (`written` method)

As well as simplifying the API (IMO), this approach has the following advantages:

* Since we pass the cursor by value, we remove the 'unsoundness footgun' where a malicious `read_buf` could swap out the `ReadBuf`.
* Since `read_buf` cannot write into the filled part of the buffer, we prevent the filled part shrinking or changing which could cause underflow for the caller or unexpected behaviour.

## Outline

```rust
pub struct BorrowBuf<'a>

impl Debug for BorrowBuf<'_>

impl<'a> From<&'a mut [u8]> for BorrowBuf<'a>
impl<'a> From<&'a mut [MaybeUninit<u8>]> for BorrowBuf<'a>

impl<'a> BorrowBuf<'a> {
    pub fn capacity(&self) -> usize
    pub fn len(&self) -> usize
    pub fn init_len(&self) -> usize
    pub fn filled(&self) -> &[u8]
    pub fn unfilled<'this>(&'this mut self) -> BorrowCursor<'this, 'a>
    pub fn clear(&mut self) -> &mut Self
    pub unsafe fn set_init(&mut self, n: usize) -> &mut Self
}

pub struct BorrowCursor<'buf, 'data>

impl<'buf, 'data> BorrowCursor<'buf, 'data> {
    pub fn clone<'this>(&'this mut self) -> BorrowCursor<'this, 'data>
    pub fn capacity(&self) -> usize
    pub fn written(&self) -> usize
    pub fn init_ref(&self) -> &[u8]
    pub fn init_mut(&mut self) -> &mut [u8]
    pub fn uninit_mut(&mut self) -> &mut [MaybeUninit<u8>]
    pub unsafe fn as_mut(&mut self) -> &mut [MaybeUninit<u8>]
    pub unsafe fn advance(&mut self, n: usize) -> &mut Self
    pub fn ensure_init(&mut self) -> &mut Self
    pub unsafe fn set_init(&mut self, n: usize) -> &mut Self
    pub fn append(&mut self, buf: &[u8])
}
```

## TODO

* ~~Migrate non-unix libs and tests~~
* ~~Naming~~
  * ~~`BorrowBuf` or `BorrowedBuf` or `SliceBuf`? (We might want an owned equivalent for the async IO traits)~~
  * ~~Should we rename the `readbuf` module? We might keep the name indicate it includes both the buf and cursor variations and someday the owned version too. Or we could change it. It is not publicly exposed, so it is not that important~~.
  * ~~`read_buf` method: we read into the cursor now, so the `_buf` suffix is a bit weird.~~
* ~~Documentation~~
* Tests are incomplete (I adjusted existing tests, but did not add new ones).

cc rust-lang#78485, rust-lang#94741
supersedes: rust-lang#95770, rust-lang#93359
fixes rust-lang#93305
@bors bors closed this as completed in b9306c2 Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
4 participants