tikv / pprof-rs

A Rust CPU profiler implemented with the help of backtrace-rs
Apache License 2.0
1.26k stars 94 forks source link

[Unsound] Slice created from unaligned pointer #232

Open shinmao opened 9 months ago

shinmao commented 9 months ago

Hi, I am the security researcher from SunLab. I am testing our personal tools on open-source repositories and find the following unsound implemenation.

The source of unsoundness

https://github.com/tikv/pprof-rs/blob/4939f73f0cc2e8d92e3c2c50c9d02d6d4c205a86/src/collector.rs#L223-L225 The function would cast file_vec which is aligned to 1 byte to the specified type, and the function can be called by try_iter() on Collector. The specified type can be decided the items in Collector. In other words, if we add the element which is aligned to more than 1 byte to the Collector, the unaligned pointer would be created and used to build the slice, which breaks the safety guarantee of slice::from_raw_parts.

This is how it could work:

fn main() {
    let a: u16 = 1;     // aligned to 2 bytes
    let mut c = Collector::new().unwrap();
    let _ = c.add(a, 1isize);
    c.try_iter().unwrap().for_each(|entry| {
        println!("{:?}", entry);
    });
}

try_iter() will call next with T as u16.

shinmao commented 9 months ago

We consider that the public safe api addr_validate::validate could also have the unsoundness: https://github.com/tikv/pprof-rs/blob/4939f73f0cc2e8d92e3c2c50c9d02d6d4c205a86/src/addr_validate.rs#L91-L96 This function allows casting c_void input to u8 pointer. However, based on the safety guarantee of from_raw_parts, caller should make sure the pointer is initialized for consecutive length. Here, we could cast arbitrary types to c_void and pass to this function which will make uninitialized memory exposure.

This is how it works:

use core::ffi::c_void;
use pprof::validate;

#[repr(align(256))]
#[derive(Copy, Clone, Debug)]
struct Padding {
    a: u8,
    b: u32,
    c: u8,
}

fn main() {
    let pd = Padding { a: 10, b: 11, c: 12 };
    println!("{}", validate(&pd as *const Padding as *const c_void));
}
jdsaund commented 4 months ago

We're also experience this issue on macos m2. The PR solves it, will it get merged soon?

finnbear commented 2 months ago

After updating to a recent nightly compiler, I observed the following related panic on pprof@0.12.0 (debug build) as a result:

thread 'main' panicked at library/core/src/panicking.rs:215:5:
unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:110:18
   2: core::panicking::panic_nounwind_fmt
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:120:5
   3: core::panicking::panic_nounwind
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/panicking.rs:215:5
   4: core::slice::raw::from_raw_parts::precondition_check
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/ub_checks.rs:66:21
   5: core::slice::raw::from_raw_parts
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/slice/raw.rs:96:9
   6: <pprof::collector::TempFdArrayIterator<T> as core::iter::traits::iterator::Iterator>::next
             at /home/finnb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pprof-0.12.0/src/collector.rs:225:26
   7: core::iter::traits::iterator::Iterator::fold
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/traits/iterator.rs:2586:29
   8: <core::iter::adapters::chain::Chain<A,B> as core::iter::traits::iterator::Iterator>::fold
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/adapters/chain.rs:93:19
   9: core::iter::traits::iterator::Iterator::for_each
             at /rustc/f9b16149208c8a8a349c32813312716f6603eb6f/library/core/src/iter/traits/iterator.rs:817:9
  10: pprof::report::ReportBuilder::build
             at /home/finnb/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pprof-0.12.0/src/report.rs:110:17

Edit: The linked PR resolved the issue! :heart:

markmandel commented 1 month ago

Just ran into this issue on upgrade to Rust 1.78.0. We can't upgrade beyond 1.77.x at this time with this bug in place.

You can see it failing here: https://github.com/googleforgames/quilkin/actions/runs/9375621368/job/25814057518?pr=974 (since writing I've likely downgraded this update back to 1.77.1)