Apply zeroize consistently

[d-sonuga]

For #3687

[moCello]

Why adding this wrapper type?

[d-sonuga]

Unlike public/secret keys which are used in specific areas of the code, passwords are used in multiple different places - it's easier to wrap them in a type that handles the zeroization instead of manually doing it everywhere they're used.

[moCello]

I'll rephrase:
Why adding a wrapper type for String, Vec and [u8] that implement the zeroizing traits when String, Vec and [u8] already have those traits implemented?
Sorry if I wasn't clear from the beginning.

[d-sonuga]

To zeroize on drop instead of having to call the zeroize function.

[Neotamandua]

For reference: dusk-network/bls12_381-bls#6 (comment), there have been some problems with using the traits.

Citing from this PR: "However, since the first example in this PR description (calling drop manually doesn't zeroize) still doesn't work, we will stick with the manual call of zeroize for now."

The preferred option would be to call zeroize manually everywhere. Personally, I am not a fan of this due to it being error prone. However, since we plan to make more changes to the wallet architecture, the occurrence of keys and passwords along with their zeroization should be greatly reduced in the future.

[moCello]

In any case: for behavior this critical for security, it is super important to add tests

[d-sonuga]

I've created some tests to show what I mean:

I did do something similar to this:

extern crate zeroize;

fn main() {
    use zeroize::Zeroize;

    #[derive(Debug, Zeroize)]
    struct ZeroizeOnDrop(Vec<u8>);
    impl Drop for ZeroizeOnDrop {
        fn drop(&mut self) {
            self.0.zeroize();
        }
    }
    let mut k = ZeroizeOnDrop(vec![255u8; 3]);
    let ptr = k.0.as_mut_ptr();
    println!("{:?}", unsafe {
        Vec::from_raw_parts(ptr, 3, 3)
    });
    unsafe { std::ptr::drop_in_place(&mut k); }
    println!("{:?}",  unsafe {
        Vec::from_raw_parts(ptr, 3, 3)
    });
}

On my computer, the output always prints out garbage looking numbers that have nothing to do with the original data. I'm not 100% sure but I believe it should have something to do with the fact that checking deallocated heap data is undefined behavior.
I wanted to write tests, too, but that isn't possible because accessing heap data after freeing it is undefined behavior.

Aside from that, I believe there are issues with the tests you have - they are potentially accessing deallocated heap data which is undefined behavior.

The first test zeroize_vec_fails checks if the memory is the same as the one that was there, but it doesn't check if it's not the same. I added two lines to it: one to check if the assertion is meaningful (assert_eq!(*stored_mem, vec![255u8, 255, 255]);) and the other to show what is actually in that area in memory: println!("{:?}", stored_mem);.
@moCello, please try running it on your own computer:

extern crate zeroize;

fn main() {
    use zeroize::{Zeroize, ZeroizeOnDrop};

    let vec: Vec<u8> = vec![1, 2, 3];
    let ptr: *const Vec<u8> = &vec;

    drop(vec);

    unsafe {
        let stored_mem: &Vec<u8> = &core::slice::from_raw_parts(ptr, 3)[0];
        // after calling `drop` the memory is still there
        println!("{:?}", stored_mem);
        assert_eq!(*stored_mem, vec![1u8, 2, 3]);
        assert_eq!(*stored_mem, vec![255u8, 255, 255]);
    };
}

On my computer, the output is garbage and this tests pass without any panicking. I'm not 100% sure, but I'm inclined to assume that it's because of the undefined behavior of accessing heap data that has been freed.

The second test, zeroize_vec_passes, shows that after calling zeroize manually and dropping, the vec is equal to an empty vec. But there are issues with this. The first is that when a vec is zeroized, its length is set to 0, so equality with an empty vec will definitely be true because the empty vec's length too is 0. It does not check that the underlying data has actually being zeroed. And even then, it's undefined behavior to try to test it. Anyways, I made a modified version of the test to actually try checking the backing buffer on the heap after manual zeroization and dropping:

extern crate zeroize;

fn main() {
    use zeroize::Zeroize;

    // Let's try again and call zeroize explicitly:
    let mut vec: Vec<u8> = vec![1, 2, 3];
    let ptr: *const u8 = vec.as_ptr();

    vec.zeroize();
    drop(vec);

    unsafe {
        let stored_mem: &[u8] = &core::slice::from_raw_parts(ptr, 3);
        println!("{:?}", stored_mem);
        // now the memory is zeroed
        assert_eq!(*stored_mem, Vec::<u8>::new());
        assert_eq!(stored_mem, [1u8, 2, 3]);
    };
}

This test gets the actual pointer to the heap data, outputs it for manual inspection, and checks if it is equivalent to an empty vector. This test fails. The backing buffer for the vector is not all zeroes - on my computer, I see random garbage values.

As for the third test, I ran this:

extern crate zeroize;

fn main() {
    use zeroize::{Zeroize, ZeroizeOnDrop};
    #[derive(Debug, Clone, PartialEq, Eq, Hash, Zeroize, ZeroizeOnDrop)]
    struct ZeroizingBytes(Vec<u8>);
    
    let mut bytes: ZeroizingBytes = ZeroizingBytes(vec![1, 2, 3]);
    let ptr: *const ZeroizingBytes = &bytes;

    drop(bytes);

    unsafe {
        let stored_mem: &ZeroizingBytes =
            &core::slice::from_raw_parts(ptr, 3)[0];
        // after calling `drop` the memory is still there even though
        // `ZeroizeOnDrop` is implemented for ZeroizingBytes
        assert_eq!(*stored_mem.0, vec![1u8, 2, 3]);
        assert_eq!(*stored_mem.0, vec![255u8, 254, 253]); // I added this
        println!("{:?}", stored_mem);
    };
}

On my computer, the output printed is random bytes that are definitely not [1,2,3] and the two assertions pass successfully. I mean, we know that that's impossible. Again, I'm not 100% sure but I'm inclined to believe that the reason for this is the undefined behavior of accessing freed heap data. I also don't know the specifics of how PartialEq for slices and vectors are implemented.

As for the fourth test, the problem with this one is the same problem with the second test: both vectors have a length of 0 and the underlying data is not checked. I modified it here to do that:

extern crate zeroize;

fn main() {
    use zeroize::{Zeroize, ZeroizeOnDrop};
    #[derive(Debug, Clone, PartialEq, Eq, Hash, Zeroize, ZeroizeOnDrop)]
    struct ZeroizingBytes(Vec<u8>);

    let mut bytes: ZeroizingBytes = ZeroizingBytes(vec![1, 2, 3]);
    let ptr: *const u8 = bytes.0.as_ptr();

    bytes.zeroize();
    drop(bytes);

    unsafe {
        let stored_mem: &[u8] =&core::slice::from_raw_parts(ptr, 3);
        // now the memory is zeroed
        println!("{:?}", stored_mem);
        assert_eq!(*stored_mem, vec![1,2,3]);
        assert_eq!(*stored_mem, vec![255u8, 255, 255]);
    };
}

This test passes on my computer: both contradicting assertions succeed. And the values printed to the terminal are certainly not what was there before. I believe it's because it's been freed.

For the purpose of testing/checking that the zeroing on drop works as expected, I've removed the ZeroizeOnDrop macro and added a manual implementation that does the zeroization and asserts that the vector is equal to the empty vector.

[moCello]

I still see no value added by adding the ZeriozingBytes type.
The tests above show that the behavior is different when zeroize is called manually (memory set to empty vec) compared to when zeroize is not specifically called (behavior undefined).

This is also true with zeroize manually implemented on Drop for ZeroizingBytes as per your last commit.
The information I gather from this is: that the only way to know for sure that the memory has been zeroized, is to call zeroize directly.

[moCello]

tests like these is what worries me here:

    #[test]
    fn zeroize_drop() {
        use super::Zeroize;
        use super::ZeroizingBytes;

        let bytes: ZeroizingBytes = vec![1, 2, 3].into();
        let ptr: *const ZeroizingBytes = &bytes;

        let mut other_bytes: ZeroizingBytes = vec![1, 2, 3].into();
        let other_ptr: *const ZeroizingBytes = &other_bytes;

        // drop should call zeroize
        drop(bytes);

        // calling zeroize before drop should be superfluous
        other_bytes.zeroize();
        drop(other_bytes);

        unsafe {
            let stored_mem: &ZeroizingBytes =
                &core::slice::from_raw_parts(ptr, 3)[0];
            let other_stored_mem: &ZeroizingBytes =
                &core::slice::from_raw_parts(other_ptr, 3)[0];
            // this assertion should pass but doesn't:
            // the memory that has been zeroized manually is set to the empty
            // vec, the other is not
            assert_eq!(*stored_mem, *other_stored_mem);
        };
    }

[d-sonuga]

Okay, I didn't realize this before but the outcome of this test happens for the same reason we can't implement zeroize on drop for secret keys - the data that holds the length information is copied.

Vectors are represented as:

pub struct Vec<T, A: Allocator = Global> {
    buf: RawVec<T, A>,
    len: usize,
}

And RawVec:

pub(crate) struct RawVec<T, A: Allocator> {
    ptr: Unique<T>,
    cap: usize,
    alloc: A,
}

So, the pointer, length and capacity are stored on the stack while the actual data is on the heap.

If you move this value into a function (like drop), although the value is semantically moved, the compiler actually emits instructions to copy it. So the struct is copied from the stack of the calling function into the stack of the called function (drop).
When drop is called on the copied value, the vec's buffer is zeroized (it's the same buffer pointed to by the original value, so this is correct) and the len field of the copied vec is set to 0. But the len field of the original vec is not set to 0. It remains as the original value because the length information is stored on the stack and in this test, the stack value is stale.

So, if you replace drop(bytes) with:

unsafe { core::ptr::drop_in_place(&mut bytes); }
core::mem::forget(bytes);

The test will pass because the struct holding the length and capacity info aren't copied - it is modified in place.

To show that this is indeed the reason, I've added another commit that prints out the pointer in the drop function and the original pointer on the stack. When drop is called, they are not the same (copied) but when core::ptr::drop_in_place is called, they are the same.

This copying thing should not be a problem for this type because the values being zeroed are in a single location on the heap, and those ones are never copied.