ANSSI-FR / rust-guide

Recommendations for secure applications development with Rust
https://anssi-fr.github.io/rust-guide
Other
594 stars 47 forks source link

Reliable Zeroing Requires Pinning #11

Open mmstick opened 5 years ago

mmstick commented 5 years ago

It's not enough to zero memory with a Drop, it's also required that the type contains a Pinned marker, which would forbid the type from being moved. Types which move do not execute Drop on the value that was moved.

It would also be a good idea to contribute to Redox OS's ralloc, which supports a security flag that zeroes all frees.

danielhenrymantilla commented 5 years ago

The zeroing part in general looks weird to me, justification-wise. I think the problem is more general than just zeroing structs on Drop; it's more about carefully crafting the right Drop semantics when doing FFI.

Take, for instance, the following made up example:

FFI-wise, there was no need to zero rust's stack struct StackChars since, as @mmstick said, something can be moved out without triggering Drop. The FFI could still be able to read UAF bytes. On the other hand, using drop to clear / reset pointers given to FFI is actually the good strategy (if the C code does not provide an API to clear / reset, that's a big problem for the C code on its own (even if it did not do any FFI), and in that case there is no other choice but to leak the pointed to data).

::ralloc

Although ralloc does look interesting, it is not able to zero stale stack data; it just provides something along these lines:

use ::std::mem::ManuallyDrop;

pub struct ZeroDropBox<T> /* = */ (Box< ManuallyDrop<T> >);

impl<T> Drop for ZeroDropBox<T>
{
    fn drop (
        self: &mut Self,
    )
    { unsafe { // This is actually UB, see last point
        ManuallyDrop::drop(&mut self.0);
        ::std::ptr::write_volatile(
          &mut self.0 as &mut T,
          ::std::mem::zeroed(),
        );
        // we could even go as far as to zero the pointer itself?
    }}
}

meaning that, although a user does not need to implement by himself a zeroing Drop for data that is put behind such "smart ptr", the data in the stack cannot be guaranteed to be zeroed (same move issue).

::std::mem::zeroed()

Finally, despite the good intention, zeroing is a very dangerous operation in rust!! From the previous example, if I had zeroed the Box pointer itself, it would have been Undefined Behavior. Indeed, rustc/llvm optimises the machine code using memory layout invariants, such as a Box being always NonNull. Break that invariant, and all hell can break loose.

This is specially dangerous for generics (take, for instance, ManuallyDrop<T>) : T can also have memory layout constraints (e.g., it can be a NonNull<U>) which means that the above code will lead to UB in those cases! (See the code in the Playground for a less unsound example)

mneumann commented 3 years ago

If the struct that you wanna zero out implements either Copy or !Copy (aka "Move"), but not Pin, Rust will happily memcpy the underlying memory of your struct whenever you assign it to a new variable, pass it to a function or pattern match on it. While the compiler guarantees that you cannot access that memory from safe Rust, it still won't zero it out. The Drop will only operate on the very last "copy" of that memory.

Putting your struct into a Box, i.e. heap allocate it, will help a bit. At least, you are not creating n copies of the contents of your struct when you move your struct around (note that in this case your struct has to be !Copy, which is the default). BUT, to get your struct into the Box you have to first create it in a temporary. The compiler might optimize the temporary away and might generate code that directly writes to the heap-allocated memory region, but AFAIK there is no guarantee that the compiler does exactly that.

I remember years ago (my memory still not zeroed out completely ;), we had an Placement trait RFC. This would have solved this problem to some degree.

The safest choice is to simply stick with a Vec<u8> and wrap that in a Zeroize struct.

You can verify yourself by running this program:

struct S(usize);

impl S {
    fn new() -> Self {
        Self(42)
    }
}

impl Drop for S {
    fn drop(&mut self) {
        println!("drop");
        unsafe{ ::std::ptr::write_volatile(&mut self.0, 0) };
    }
}

struct Pointers {
    a_ptr: *const S,
    b_ptr: *const S,
}

fn test() -> Pointers {
    let a = S::new();
    let a_ptr: *const S = &a;
    println!("&a = {:p}", a_ptr);

    let b = a; // moved here
    let b_ptr: *const S = &b;
    println!("&b = {:p}", b_ptr);

    println!("*a = {}", unsafe { (*a_ptr).0 });
    println!("*b = {}", unsafe { (*b_ptr).0 });

    Pointers { a_ptr, b_ptr }
}

fn main() {
    let Pointers { a_ptr, b_ptr } = test();
    let a = unsafe { (*a_ptr).0 };
    let b = unsafe { (*b_ptr).0 };

    println!("*a = {}", a);
    println!("*b = {}", b);
}

On my machine (FreeBSD), this outputs:

&a = 0x7fffffffdfd8
&b = 0x7fffffffe030
*a = 42
*b = 42
drop
*a = 42
*b = 0

Look at the last two lines, *a = 42 and *b = 0. Here, a is not zeroed out.