Implement zeroize for secret values

[moCello]

Resolves #5

[moCello]

This is a tricky situation because even when we implement Zeroize on Drop for SecretKey, the memory still won't be erased:

impl Zeroize for SecretKey {
    fn zeroize(&mut self) {
        self.0 0.zeroize();
    }
}

impl Drop for SecretKey {
    fn drop(&mut self) {
        self.zeroize();
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn zeroize() {
        let sk = SecretKey::from(BlsScalar::from(42));
        let ptr = sk.as_ref().0.as_ptr();
        drop(sk);

        // We would expect that the memory is erased during `drop` but it is still there
        unsafe {
            assert_eq!(
                core::slice::from_raw_parts(ptr, 4),
                BlsScalar::from(42).0
            );
        };

        // Let's try again and call zeroize explicitly:
        let mut sk = SecretKey::from(BlsScalar::from(42));
        let ptr = sk.as_ref().0.as_ptr();
        sk.zeroize();
        drop(sk);

        // now the memory is zeroed
        unsafe {
            assert_eq!(core::slice::from_raw_parts(ptr, 4), [0; 4]);
        };

    }
}

As this blogpost describes this has to do with the memory being copied under the hood

The answer is that in Rust, moving a value compiles into a memory copy in the general case. Sometimes, the compiler can be smart and optimize away a memory copy, but other times it is impossible. For example [..], in the manual drop() the value is moved into the drop() function, but the value is copied in memory while doing so.

This means that the copied value in drop is zeroized, but not the value in our program (the value we want to be zeroized).

One solution, as suggested in the blogpost, is to Box the BlsScalar in the SecretKey and therefore have it heap-allocated. This seems to erase the allocated memory even without calling zeroized on drop. This approach comes with a performance penalty.

The other is to leave it up to the user to call zeroize on any SecretKey before it goes out of scope.

And in any case the Copy trait should not be derived on any struct that carries sensitive data.

[herr-seppia]

Wasn't the issue about adding zeroize to secret data?
Is there a reason to derive zeroize for public structure (like public key, apk etc)

[moCello]

Wasn't the issue about adding zeroize to secret data? Is there a reason to derive zeroize for public structure (like public key, apk etc)

the issue was regarding secret values indeed. But while implementing I figured that it also doesn't hurt to implement it for other types as well.
Imagine for example someone being able to link a public key to a specific person because it has not been erased from a shared machine.

[ureeves]

I'm totally fine with introducing this and remove the Copy trait. I always found it strange that SecretKeys would be Copy. That said, while it does mitigate the problem, it doesn't solve it, since a move will produce the same results. It is, however, the best we can do for now.

[moCello]

In my opinion it doesn't make sense to have both the Copy and Zerioze traits on types where the user is responsible to make sure to erase the data locally before it goes out of scope.

I understand that removing the Copy on the public types (PublicKey, APK, Signature) simply as a precaution so we can add Zeroize might be overkill for now and can cause issues downstream. So I decided to implement Zeroize (and remove Copy) only for SecretKey.

If, at a later point, we decide that we need Zeroize on the other types too, we can still add it then.

[herr-seppia]

LGTM