Potential Memory Safety Issue Discovered in `SeqRead` Trait

[lewismosciski]

Hello,

First, thank you for your work on the vmap crate.

We are developing a static analysis tool for Rust, and during our testing, our tool flagged a potential memory safety issue in the design of the SeqRead trait. We investigated further and believe we can confirm the finding.

The core of the issue lies in the default implementation of SeqRead::as_read_slice. This function contains an unsafe block that creates a slice using a pointer and a length provided by the trait's implementer (as_read_ptr, read_offset, read_len).

https://github.com/kalamay/vmap-rs/blob/05a00f0ecdabd1cab92c9dc5ef066fa4854b75a3/src/io/mod.rs#L40-L58

The unsafe code trusts that read_len will return a length that is within the bounds of the buffer provided by as_read_ptr. However, this contract is not enforced by the trait's signature.

This can lead to a buffer over-read, which is Undefined Behavior and can cause information disclosure or crashes.

PoC

use vmap::io::*;
use std::io::{self, BufRead};

struct VulnerableBuffer {
    data: [u8; 16],             
    read_pos: usize,             
}

impl VulnerableBuffer {
    fn new() -> Self {
        Self {
            data: *b"0123456789ABCDEF",
            read_pos: 0,
        }
    }
}

impl io::Read for VulnerableBuffer {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        self.read_from(buf)
    }
}

impl BufRead for VulnerableBuffer {
    fn fill_buf(&mut self) -> io::Result<&[u8]> {
        Ok(self.as_read_slice(usize::MAX))
    }
    fn consume(&mut self, amt: usize) {
        self.read_pos += amt;
    }
}

impl SeqRead for VulnerableBuffer {
    fn as_read_ptr(&self) -> *const u8 {
        self.data.as_ptr()
    }

    fn read_offset(&self) -> usize {
        self.read_pos
    }

    fn read_len(&self) -> usize {
        32 - self.read_pos
    }
}

fn main() {
    let mut buffer = VulnerableBuffer::new();
    let mut leaked_data = [0u8; 32];
    let _bytes_read = buffer.read_from(&mut leaked_data).unwrap();
}

Test with miri

ccuu@ccuu:~/rust/rtest/nnl_test$ cargo +nightly miri run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `/home/ccuu/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/nnl_test`
error: Undefined Behavior: pointer not dereferenceable: pointer must be dereferenceable for 32 bytes, but got alloc130 which is only 24 bytes from the end of the allocation
  --> /home/ccuu/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/vmap-0.6.3/src/io/mod.rs:42:13
   |
42 | /             slice::from_raw_parts(
43 | |                 self.as_read_ptr().add(self.read_offset()),
44 | |                 cmp::min(self.read_len(), max),
45 | |             )
   | |_____________^ Undefined Behavior occurred here
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc130 was allocated here:
  --> src/main.rs:48:9
   |
48 |     let mut buffer = VulnerableBuffer::new();
   |         ^^^^^^^^^^
   = note: BACKTRACE (of the first span):
   = note: inside `<VulnerableBuffer as vmap::io::SeqRead>::as_read_slice` at /home/ccuu/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/vmap-0.6.3/src/io/mod.rs:42:13: 45:14
   = note: inside `<VulnerableBuffer as vmap::io::SeqRead>::read_from` at /home/ccuu/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/vmap-0.6.3/src/io/mod.rs:52:23: 52:53
note: inside `main`
  --> src/main.rs:50:23
   |
50 |     let _bytes_read = buffer.read_from(&mut leaked_data).unwrap();
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error