Zeroing out secrets

[elichai]

Hi,
I Implemented the Drop trait to SecretKey and SharedSecret with write_volatile to zero out the structs when they get dropped.
this should be essentially the same as memset_s meaning the compiler shouldn't optimize this out (https://doc.rust-lang.org/std/ptr/fn.write_volatile.html)

For this I had to remove the Copy trait because Copy types can't implement Drop.

[elichai]

BTW, I see that some times you use debug_assert but don't return a Result, why is that?
(i.e. https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/ecdh.rs#L42, https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/key.rs#L232)

[real-or-random]

I Implemented the Drop trait to SecretKey and SharedSecret with write_volatile to zero out the structs when they get dropped.
this should be essentially the same as memset_s meaning the compiler shouldn't optimize this out (https://doc.rust-lang.org/std/ptr/fn.write_volatile.html)

This sounds like a good idea to me (even though it's not perfect).

For this I had to remove the Copy trait because Copy types can't implement Drop.

This sounds like a bad idea at first glance, also because it has the potential to break downstream code. But maybe it's not even a bad thing if we want to promote safe code: If you want be careful with your secrets (e.g., and want them to be zeroed out in memory after usage), then you'll need to be careful when assigning them to other variables. I'm not sure if I like that change or not to be honest.

BTW, I see that some times you use debug_assert but don't return a Result, why is that?
(i.e. https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/ecdh.rs#L42, https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/key.rs#L232)

Well, at those code locations we assume that our code ensures that the call FFI function such that will always work. In other words: If the FFI call fails, this is either a bug in our code or in secp256k1, but it has nothing to do with code calling rust-secp256k1 functions. So there is no reason to signal an error to the library user, these calls are not supposed to fail.

[elichai]

@real-or-random Yeah,
my concern is that in reality if you pass a struct by value it gets moved in memory.
So I'm still debating myself how useful this is, although if it's used correctly (i.e. Boxed or passed only by reference) then this is an improvement over nothing.

example: https://play.rust-lang.org/?gist=404181cf18a18bcc2fee5bdd04acd6b7

[jonasnick]

conceptACK. I don't share the concerns that this breaks downstream code because this appears to be the correct way to handle secrets.

There's also a zeroize crate which seems to do the same thing (write_volatile but more general). As for memory moves I wonder if pinning could help in the future.

[real-or-random]

I know we don't like dependencies too much but zeroize sounds like the right approach and it's small enough to pin a version / review changes from time to time.

re pinning memory: I don't see how this is related to secure zeroing. Do you have a specific scenario in mind?

[elichai]

@real-or-random This wouldn't be possible if the memory was pinned: (Assuming I understand the Pin API correctly)
https://play.rust-lang.org/?gist=404181cf18a18bcc2fee5bdd04acd6b7

i.e. the same but with a Box: (meaning the memory of the secret it self won't be moved)
https://play.rust-lang.org/?e60a80fda1a5d20bc24f471208643fd9

[apoelstra]

Semantically generate_keypair returns a SecretKey object which exists on its stack frame, and copies the bytes into the caller's stack frame without calling any Drop impl.

I believe the compiler is smart enough in this case to just construct the original SecretKey in place, but we're depending on this behaviour to happen consistently for this PR to make sense.

BTW, we can't use Pin because of this.

[apoelstra]

I'm very ambivalent about this PR. Getting rid of Copy won't prevent copies from being made, it will just hurt usability and possibly performance. Meanwhile Drop is not guaranteed to run.

[elichai]

I think Drop is guaranteed to run if you don't do anything unsafe (maunally copy the memory, abort the program etc.)

I think that the only problem with Drop is that it doesn't run on moved objects.

[apoelstra]

Drop is definitely not guaranteed to run. In addition to the things you describe, you can call mem::forget, put it in a Rc loop, or pass it to a thread that doesn't terminate.

[elichai]

You're right.

[real-or-random]

@apoelstra Any idea how an Pinnable version of generate_keypair (and similar functions) could look like without totally breaking usability? I think that will be weird.

[apoelstra]

Every time I try to do this I conclude it's simply impossible.

[elichai]

After playing a lot with Pin here: https://github.com/elichai/Lorenz/blob/master/src/secret.rs#L10
I'll say that in my experience it works good only on heap allocations. on stack allocation it's not easy to make a Pin for that without moving the object(and essentially copy it).

(so in my library for example the secrets are only on the heap)

[apoelstra]

I think I'm going to concept NAK this PR. It is a very incomplete solution and sends the following misleading messages:

• by getting rid of Copy secrets will not be copied in memory
• by implementing Drop, the drop function will be run on at least one copy of a secret, erasing it

It also hurts usability, which may imply increased security to the more Puritan-minded of our users..

[elichai]

Yeah I now(after seeing that without heap it's not a real option to do this correctly) think it might be better for the user of the library to pin and implement this himself.
Especially since we expose raw pointers anyway so he can easily wipe the underlying memory if he want.

[real-or-random]

I think I'm going to concept NAK this PR. It is a very incomplete solution and sends the following misleading messages:

* by getting rid of `Copy` secrets will not be copied in memory

* by implementing `Drop`, the `drop` function will be run on at least one copy of a secret, erasing it

It also hurts usability, which may imply increased security to the more Puritan-minded of our users..

Agreed, this is too incomplete. Feel free to discuss this further, but I'm closing for now.