beef::Cow lacks a Sync bound on its Send trait allowing for data races

[ammaraskar]

I think the impl for Send on Cow should be bounded by Send + Sync or Sync like it is for references and the std::Cow itself. (This issue was found by the rust group @sslab-gatech).

beef/src/generic.rs

Line 531 in 0b46851

unsafe impl<T: Beef + Send + ?Sized, U: Capacity> Send for Cow<'_, T, U> {}

Without this it's possible to create references to a non-Sync object like Cell through the use of a borrowed Cow. For example, consider the following program (uses crossbeam_utils to make dealing with threads easier):

#![forbid(unsafe_code)]

use crossbeam_utils::thread;
use std::cell::Cell;

use beef::Cow;

fn main() {
    let x = [Cell::new(42)];

    // A simple proof-of-concept demonstrating how a loose bound on Cow's
    // Send trait allows non-`Sync` but `Send`able objects to be shared across
    // threads.
    thread::scope(|s| {
        let cow1: Cow<[Cell<i32>]> = Cow::borrowed(&x[..]);
        let cow2: Cow<[Cell<i32>]> = cow1.clone();

        let child = s.spawn(|_| {
            let smuggled = cow2.unwrap_borrowed();
            smuggled[0].set(2);

            // Print the value of the Cow-value and the address of the
            // underlying Cell.
            println!("{:?}, {:p}", smuggled, &smuggled[0]);
        });
        child.join().unwrap();

        // This should print the same address as above indicating the underlying
        // `Cell` in x is now shared across threads, a violation of `!Sync`.
        println!("{:?}, {:p}", x, &x[0]);
    });
}

This produces the output:

[Cell { value: 2 }], 0x7ffd1591f4cc
[Cell { value: 2 }], 0x7ffd1591f4cc

While this example is pretty benign, here's how it can lead to an arbitrary memory read from safe rust code:

Click to expand code snippet

#![forbid(unsafe_code)]

use crossbeam_utils::thread;
use std::cell::Cell;

use beef::Cow;

static SOME_INT: u64 = 123;

fn main() {
    // A simple tagged union used to demonstrate the problems with data races
    // in Cell. Cell is designed for single threads and has no synchronization
    // methods. Thus if it is allowed to be used simaltaneously by two threads,
    // it is possible to race its interior mutability methods to derference an
    // arbitrary pointer.
    #[derive(Debug, Clone, Copy)]
    enum RefOrInt<'a> {
        Ref(&'a u64),
        Int(u64),
    }

    let cell_array = [Cell::new(RefOrInt::Ref(&SOME_INT))];
    thread::scope(|s| {
        let cow1: Cow<[Cell<RefOrInt>]> = Cow::borrowed(cell_array.as_ref());
        let cow2: Cow<[Cell<RefOrInt>]> = cow1.clone();

        let child = s.spawn(move |_| {
            // We've now smuggled the cell from above into this thread.
            let smuggled_cell = cow2.unwrap_borrowed();
            loop {
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                smuggled_cell[0].set(RefOrInt::Ref(&SOME_INT));
                smuggled_cell[0].set(RefOrInt::Int(0xdeadbeef));
            }
        });

        loop {
            let main_thread_cell = (*cow1)[0].clone().into_inner();
            if let RefOrInt::Ref(addr) = main_thread_cell {
                // Hope that between the time we pattern match the object as a
                // `Ref`, it gets written to by the other thread.
                if addr as *const u64 == &SOME_INT as *const u64 {
                    continue;
                }

                // Due to the data race, obtaining Ref(0xdeadbeef) is possible
                println!("Pointer is now: {:p}", addr);
                println!("Dereferencing addr will now segfault: {}", *addr);
            }
        }
    });
}

This example produces:

Borrowed mutably! - Main
Initial destructure: 42
Borrowed mutably! - Thread
About to dereference ptr again

Return Code: -11 (SIGSEGV)

[maciejhirsz]

Great catch!

[CAD97]

You should probably consider yanking the versions with the incorrect bound.

I'd also recommend submitting a RustSec advisory; it should be accepted, RUSTSEC-2021-0004 is a similar incorrect Send/Sync bound.

[JOE1994]

I'll handle reporting to RustSec :)