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

index() allows out-of-bound read and remove() has off-by-one error #2

Closed
Qwaz opened this issue Sep 4, 2020 · 0 comments
Closed

index() allows out-of-bound read and remove() has off-by-one error #2

Qwaz opened this issue Sep 4, 2020 · 0 comments

Comments

@Qwaz
Copy link

Qwaz commented Sep 4, 2020

Hello, we have noticed a soundness issue and/or a potential security vulnerability in this crate while performing a security scan on crates.io.

simple-slab/src/lib.rs

Lines 160 to 165 in f1b18e1

impl<T> Index<usize> for Slab<T> {
type Output = T;
fn index(&self, index: usize) -> &Self::Output {
unsafe { &(*(self.mem.offset(index as isize))) }
}
}

simple-slab/src/lib.rs

Lines 82 to 103 in f1b18e1

#[inline]
pub fn remove(&mut self, offset: usize) -> T {
assert!(offset < self.len, "Offset out of bounds");
let elem: T;
let last_elem: T;
let elem_ptr: *mut T;
let last_elem_ptr: *mut T;
unsafe {
elem_ptr = self.mem.offset(offset as isize);
last_elem_ptr = self.mem.offset(self.len as isize);
elem = ptr::read(elem_ptr);
last_elem = ptr::read(last_elem_ptr);
ptr::write(elem_ptr, last_elem);
}
self.len -= 1;
return elem;
}

Description

Slab::index() does not perform the boundary checking, which leads to out-of-bound read access. Slab::remove() copies an element from an invalid address due to off-by-one error, resulting in memory leakage and uninitialized memory drop.

Demonstration

  • Crate: simple-slab
  • Version: 0.3.2
  • OS: Ubuntu 18.04.5 LTS
  • Rust: rustc 1.46.0 (04488afe3 2020-08-24)
#![forbid(unsafe_code)]

mod boilerplate;

use simple_slab::Slab;

#[derive(Debug, PartialEq)]
struct DropDetector(u32);

impl Drop for DropDetector {
    fn drop(&mut self) {
        println!("Dropping {}", self.0);
    }
}
fn main() {
    boilerplate::init();

    let mut slab = Slab::with_capacity(2);
    slab.insert(DropDetector(123));
    slab.insert(DropDetector(456));

    // 1. No boundary checking leads to OOB read in `index()`
    println!("{:?}", slab[20]);

    // 2. Memory leak / uninitialized memory access in `remove()`
    // element should be copied from `len - 1`, not `len`
    assert_eq!(slab.remove(0).0, 123);
    assert_eq!(slab[0].0, 456); // copied from uninitialized region `slab[2]`
}

Output:

DropDetector(3090534592)
Dropping 123
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `456`', src/main.rs:49:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Dropping 0

Return Code: 101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant