Add feature zeroize

[tcharding]

It is useful for downstream users to be able to zeroize secrets. Doing so is supported by the zeroize library.

Add feature zeroize which, if enabled, implements Zeroize for all our types that use impl_array_newtype (includes SecretKey and KeyPair).

On purpose we do not implement Drop and we still implement Copy. This is so as to not negatively effect users of the library (see link below from when this was attempted previously). This means that stack variables are not zerozied and there may be additional copies made by the compiler.

Adds a section to the README explaining the aims of the zeroize feature, included here:

Zeroizing secrets is supported using the `zeroize` feature. Enabling this
feature changes the MSRV to 1.44 (see https://docs.rs/zeroize/1.2.0/zeroize/).

The aim of the `zeroize` feature is to allow users of this library to reduce the
number of copies of secrets in memory (by wrapping our types, implementing
`Zeroize` and implementing `Drop` (i.e. derive `Zeroize`). Our aim is not to
guarantee that there are no copies left un-zeroed in memory. Internally we
absolutely do leave secrets on the stack.

> I think even zeroing some out the copies is better than none, because every
> copy makes a Heartbleed-like attack a little easier.

@elichai this quote is from you in this PR's thread, I gave not attribution of the source of the quote. Pleases say so if you would like attribution (or if you would prefer not to be quoted).

Please note, PR introduces a new dependency on zeroize v1.2 .

[tcharding]

I was going to open an issue and discussion and what not but since this is pretty basic I thought I'd just learn how to contribute to rust-secp245k1 and open a PR. Please feel free to be as critical as you like, I'm here to learn.

[thomaseizinger]

Given how trivial the generated implementation is, we might want to implement this ourselves so we don't add syn to this dependency tree? (Assuming none of the other dependencies already pull it in but I don't think so)

[tcharding]

Has the added bonus that we can use zeroize for the feature name :)

[tcharding]

Is 'no new dependencies' a hard rule @apoelstra ? I don't see how to do this without adding the dependency on zeroize.

[thomaseizinger]

Has the added bonus that we can use zeroize for the feature name :)

FWIW: You could have also achieved this by enabling the custom derive support through the feature that is exposed by zeroize instead of depending on zeroize_derive directly: https://docs.rs/zeroize/1.2.0/zeroize/#custom-derive-support

zeroize = { version = "1.2", default-features = false, optional = true, features = ["zeroize_derive"] }

This will re-export the Zeroize derive from the zeroize crate as zeroize::Zeroize. Serde uses the same pattern so you don't have to depend on serde and serde_derive at the same time.

[tcharding]

Cool, cheers man.

[apoelstra]

Agree with @thomaseizinger, we might as well generate this ourselves (especially as we already have macros which do a pile of stuff for byte arrays...might as well impl Zeroize on all of them).

Also need to add a note to the README that if you enable this feature the MSRV is 1.44, since normally it's 1.29. And I guess CI needs another case (I'd be fine if we just tacked testing this onto the nightly test case, since we don't actually use the code we only need to build-test it.)

[real-or-random]

We rejected zeroize in the past, see the discussion in #102 .

We can of course reconsider this.

[apoelstra]

@real-or-random well, this PR is far more limited in scope than the previous one. We aren't implementing Drop or unimplementing Copy or anything, we're just giving users who want to use zeroize the ability to use it with our types.

[real-or-random]

Ok, I see. This makes sense then!

[tcharding]

Perhaps we should not be implementing Zerioze for SecretKey at all since doing so implies that the library correctly handles zeroing secrets. I personally am not comfortable being responsible for making that statement since I do not know enough about how the compiler works to guarantee that the call chain let (sk, _) = generate_keypair() -> SecretKey::new() -> random_32_bytes() will not leave any secret data in memory if the user calls sk.zeroize().

This makes me think that all the zeroing I've done outside of this library is just smoke and mirrors :)

[elichai]

Perhaps we should not be implementing Zerioze for SecretKey at all since doing so implies that the library correctly handles zeroing secrets. I personally am not comfortable being responsible for making that statement since I do not know enough about how the compiler works to guarantee that the call chain let (sk, _) = generate_keypair() -> SecretKey::new() -> random_32_bytes() will not leave any secret data in memory if the user calls sk.zeroize().

This makes me think that all the zeroing I've done outside of this library is just smoke and mirrors :)

Yeah, if you want to do best efforts you need to do one of 2 things:

1. Box the secret on the heap, and only use the box/reference and zero at the end.
2. Make sure that every stack instance is zeroed. this includes "moved" memory (it's actually debatable if writing into moved memory is UB or not in rust)

And even after these 2 you have 2 problems:

1. At the "math" level, you need to make sure that scalars/points/limbs everything is zeroed correctly and there are no copies/moves left unzeroed.
2. The compiler can still introduce copies that are invisible to your code, which you obviously can't zero out. so you should probably audit the assembly.

EDIT: IMHO even though that last point seems like this is meaningless I don't agree. I think even zeroing some out the copies is better than none, because every copy makes a Heartbleed-like attack a little easier.

[tcharding]

EDIT: IMHO even though that last point seems like this is meaningless I don't agree. I think even zeroing some out the copies is better than none, because every copy makes a Heartbleed-like attack a little easier.

So, if I'm understanding correctly; zero what we can, gather as much information as we can get about things we might be missing and document that somewhere so its explicit what we've done and where there might be holes still. And doing so adds value to users of the library even if its not perfect. I"m on it!

[elichai]

Concept ACK

[dr-orlovsky]

Alternative impl without new dependency: #311

[tcharding]

Feel free to close this if #311 goes in.

[tcharding]

Seems like @dr-orlovsky has this feature covered (#312 and #313).

[elichai]

I just found a big problem with ever implementing Zeroize on SecretKey. it allows you to zero out a secret using safe rust, making this possible:

let secp = Secp256k1::new();
let mut secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
secret_key.zeroize();
let sig = secp.sign(&message, &secret_key);

This will cause an abort and violate libsecp256k1's API rules.

[kayabaNerve]

Necro-comment, yet a custom zeroize impl could call zeroize (writing zeroes without being optimized out if not further used). and then write all 1s. This will create a valid secret key, and if the key is used after zeroizing, the write 1s won't be optimized out.