briansmith / ring

Safe, fast, small crypto using Rust
Other
3.7k stars 698 forks source link

Add support for MemorySanitizer (msan) to avoid false positives; Use msan in CI/CD #1227

Open briansmith opened 3 years ago

briansmith commented 3 years ago

See https://internals.rust-lang.org/t/should-core-assume-init-integrate-with-msan/14307 by @sayrer. Basically ring-based applications that use ring::aead aren't compatible with with msan because msan thinks we're using uninitialized memory when the memory is initialized by non-Rust code. Currently there is just one instance of this pattern:

#[inline]
fn encrypt_block_(
    f: unsafe extern "C" fn(&Block, *mut Block, &AES_KEY),
    a: Block,
    key: &Key,
) -> Block {
    let mut result = core::mem::MaybeUninit::uninit();
    unsafe {
        f(&a, result.as_mut_ptr(), &key.inner);
        result.assume_init()
    }
}

In the future I hope to change ring APIs to support MaybeUninit<[u8]> for all output buffer parameters. So, while it is tempting to just rewrite the above function to avoid MaybeUninit, that's not a long-term solution.

I think instead we should have a new feature that enables a call to __msan_unpoison here. The people's msan builds would need to enable this feature. We should abstract this into a wrapper of MaybeUninit<T> that exposes an assume_init_and_msan_unpoison function that we use.

sayrer commented 3 years ago

applications that use ring::aead aren't compatible with with msan because msan thinks we're using uninitialized memory when the memory is initialized by non-Rust code

I think msan would work fine if the memory was initialized in C or C++. It's initialized by assembly here, right?

briansmith commented 3 years ago

I think msan would work fine if the memory was initialized in C or C++. It's initialized by assembly here, right?

Right; it's important we avoid doing an unpoison if the initialization was done in C. Working on something to that effect now.

briansmith commented 3 years ago

Not sure how much I like it, but a sketch of this is in #1228.

sayrer commented 3 years ago

From the perspective of a ring consumer, I'd say the goal is to run these checks on all of the other libraries that are being included. The approach in the sketch seems ok, if you accept that ring gets a lot more scrutiny than most libraries.

briansmith commented 3 years ago

BTW, I considered just getting rid of MaybeUninit in encrypt_block and I'm wavering a bit. It seems like the performance cost of zeroing the 16 bytes is pretty much nothing but also the function is really supposed to be fast enough where almost nothing is still something.