Unsoundness from panic on lock failure.

[noamtashma]

The problem

When the with_cryo function completes, it must guarantee that any outstanding CryoRef is gone. If any are left, it can't safely continue execution under any circumstances. In some cases (LocalLock and AtomicLock for example), this is done by panicking (specifically, unwinding). However, panics don't actually guarantee that execution will freeze, and this is unsound. There are a few ways to exploit this:

When a panic is thrown in a thread and it isn't caught, it deallocated the Cryo and stops the execution of the thread. However, the rest of the threads keep running, which is unsafe because they might still hold the CryoRef. This is Exploit through multithreading.

Altertnatively, execution can continue using the catch_unwind, which is a safe, std function. It is guarded by UnwindSafe, but it's merely a heuristic helper, not a safety guard (see UnwindSafe). This leads to unsoundness even in single threaded contexts. This is exploit through catch_unwind.

In addition, we can use the destructors called by the panic to use the leaked CryoRef. This is Exploit through destructors.

Examples

Exploit through multithreading

use std::sync::{Arc, Mutex};
use std::{panic::*, pin::Pin};
use cryo::*;

fn illegal_pointer_atomic() -> CryoRef<usize, AtomicLock> {
    let to_leak: Arc<Mutex<Option<CryoRef<usize, AtomicLock>>>> = Arc::new(Mutex::new(None));
    let to_leak_clone = to_leak.clone();

    let _join_res = std::thread::spawn(move || {
        let cell: usize = 42;

        // This call will panic
        with_cryo(
            (&cell, lock_ty::<AtomicLock>()),
            |cryo: Pin<&Cryo<'_, usize, _>>| {
                // leak a reference outside
                *to_leak_clone.lock().unwrap() = Some(cryo.borrow());
                // `with_cryo` will panic because the `Cryo` still has outstanding references to it
            },
        )
        // The panic will kill the thread
    })
    .join() // Join the thread to ensure it completed (panicked). Alternatively, just wait a bit.
    .expect_err("expected join failure");

    // Obtain and return the leaked `CryoRef<usize, AtomicLock>`
    let cryo_ref: CryoRef<usize, AtomicLock> = to_leak
        .lock()
        .expect("Mutex not available")
        .take()
        .expect("expected Leaked CryoRef");
    cryo_ref
}

fn main() {
    let illegal_cryo = illegal_pointer_atomic();
    println!("Printing address: {}", &*illegal_cryo as *const _ as i32);
    println!("Uninitialized read from address: {}", *illegal_cryo); // reading from the stack of a thread that terminated, i.e., an uninitialized read.
}

Exploit through catch_unwind

use std::{panic::*, pin::Pin};
use cryo::*;

fn illegal_pointer_local() -> CryoRef<usize, LocalLock> {
    let cell: usize = 42;

    let mut to_leak: Option<CryoRef<usize, LocalLock>> = None;
    let mut to_leak_unwind_safe = AssertUnwindSafe(&mut to_leak);

    // `with_cryo` uses `LocalLock` by default
    let err = catch_unwind(move || {
        with_cryo(&cell, |cryo: Pin<&Cryo<'_, usize, _>>| {
            // leak a reference outside
            **to_leak_unwind_safe = Some(cryo.borrow());
            // `with_cryo` will panic because the `Cryo` still has outstanding references to it
        });
    });
    // The panic is caught, and we can proceed, with the `CryoRef` still inside `r`
    println!("Recovered from error: {:?}", err);

    // Return the now-leaked CryoRef
    to_leak.unwrap()
}



fn main() {
    let illegal_cryo = illegal_pointer_local();
    println!("Printing address: {}", &*illegal_cryo as *const _ as i32);
    println!("Uninitialized read from address: {}", *illegal_cryo); // reading from the stack of a function that returned, i.e., an uninitialized read.
}

Exploit through destructors

struct ReadOnDrop(Option<CryoRef<usize, LocalLock>>);

impl Drop for ReadOnDrop {
    fn drop(&mut self) {
        if let Some(illegal_cryo) = &self.0 {
            println!("Printing address: {}", &*illegal_cryo as *const _ as i32);
            println!("Uninitialized read from address: {}", **illegal_cryo);
        }
    }
}

fn main() {
    let mut to_leak = ReadOnDrop(None);

    let mut cell = 42;

    // `with_cryo` uses `LocalLock` by default
    with_cryo(&mut cell, |cryo: Pin<&CryoMut<'_, usize, _>>| {
        // leak a reference outside
        to_leak = ReadOnDrop(Some(cryo.read()));
        // `with_cryo` will panic because the `Cryo` still has outstanding references to it
    });
    // The panic will drop the `to_leak` value
}

Other locks

This is most obvious in LocalLock and AtomicLock, which panic on purpose. However, any edgecase that triggers a panic during lock acquisition will lead to the same unsoundness.

In addition, this also applies to user-defined locks from outside this library, implemented through the Cryo::Lock trait. It is an unsafe trait, but its interface doesn't restrict panicking in any way (and in fact, panicking is the expected behavior of the lock_exclusive function). And, with the lock_api feature, all the locks implementing RawRwLock are also available, which can also panic freely.

[noamtashma]

Mitigation

Generally, I think panicking should be replaced with either

• Infinite loops / blocking until the lock can be taken
• Aborting (aborting should abort the whole process, and therefore not allow any execution to continue at all)

The current thread must continue living, but it can't continue any kind of execution at all, or the whole process should collapse.

If supporting all of the RawRwLock locks is still wanted, or just to be sure there aren't any panics that were missed, the locks can be wrapped in a wrapper lock type, that will wrap the inner calls to lock_exclusive in a catch_unwind something like this:

// Safety: `lock_exclusive` is prevented from unwinding. See [issue #1](https://github.com/yvt/cryo/issues/1)
let unwind_safe_lock = std::panic::AssertUnwindSafe(lock);
match std::panic::catch_unwind(|| unsafe {
    // I would have preferred to use a method returning a `Result`.
    // However, the regular lock blocks on `lock_exclusive` but returns an error
    // on `try_lock_exclusive`, so this doesn't work.
    unwind_safe_lock.lock_exclusive()
}) {
    Ok(_) => (),
    Err(_) => std::process::abort(),
}

or alternatively, loop until it succeeds.

The Cryo::Lock trait should be changed to reflect that panicking in the lock_exclusive method is unsafe, or alternatively, every lock will need to be wrapped in the aforementioned wrapper before use.