implement Zeroize for Scalar and MontgomeryPoint

[DebugSteven]

This PR implements the Zeroize trait for Scalar
and MontgomeryPoint. This enables x25519-dalek to use the
zeroize crate to securely zero the memory of secret
keys. The switch to use zeroize would be nice because it
has nicer documentation and platform support. The nightly
feature in zeroize also provides fast byte array zeroing
via volatile_set_memory.

[hdevalence]

Cool! Two comments:

I think if we add support for zeroize, we should probably switch to using it internally, instead of having both zeroize and clear_on_drop.

Right now I don't think we export any trait implementations etc. related to clear_on_drop, so it's not part of the public API. If we switch to zeroize and expose an implementation of the Zeroize trait, this means we're committed to exposing that implementation until at least the next major version of the crate, and our API version gets locked to the zeroize version. So that would be something to think about before making this change (and before doing more work on the first point).

@tarcieri what's the current state of the zeroize API?

[hdevalence]

For Scalars, we might want to implement the ZeroizeWithDefault marker trait (which is valid, because setting the bytes of a Scalar to zero gives the byte representation of 0, which is the Default value of a Scalar).

This means that the blanket Zeroize impls come into play for [Scalar], etc.

For MontgomeryPoints, I'm not sure we need to implement any kind of zeroing, because scalars are (sometimes) secret but points aren't.

[tarcieri]

@hdevalence with Pin shipped, I'd really like to push people to use Pin<Z> where Z: Zeroize. I will try to add Pin support to zeroize RSN.

However, several people have adopted it with the current API, so I plan on keeping that available in perpetuity. I may move to putting it behind an off-by-default cargo feature though (e.g. unpinned). Whenever I do that, Rust versions with Pin (1.33+) will become the mandatory minimum required version for the crate.

There is no ClearOnDrop analogue. I've been meaning to make a zeroize_derive crate which allows you to #[derive(ZeroizeOnDrop)], but have no timeline on that.

All that said, if you do switch to zeroize, it will allow you to support WASM targets without custom logic to disable zeroization (it leverages WASM's existing volatile write semantics, and should be able to leverage memory fences "for free" when they're standardized).

[DebugSteven]

Your point to switch to use zeroize internally makes sense to me. I can implement ZeroizeWithDefault in addition to the Zeroize trait for Scalar and exclusively MontgomeryPoint depending on how you'd like to move forward with this change. Based on Tony's comment, would holding off until Pin is used in the zeroize crate be best?

I believe we need the zeroing for MontgomeryPoint because we have memory zeroing drop logic for SharedSecrets in x25519-dalek, which is implemented as a type wrapper around MontgomeryPoint.
https://github.com/dalek-cryptography/x25519-dalek/blob/60031b6ce4b0505b168f60ab0b9880700d889952/src/x25519.rs#L139-L148

[hdevalence]

@DebugSteven re: zeroing MontgomeryPoints, you're absolutely right, sorry about that.

From looking at the zeroize API I think that if the ZeroizeWithDefault trait is implemented it's not necessary to implement Zeroize because of the blanket impl. (side note: maybe DefaultIsZeroes would be a more descriptive trait name for the marker trait in zeroize?)

Re: a ClearOnDrop analogue, IIRC the dalek code is for the most part calling clear manually, so I don't know if we really need a wrapper type.

Getting Pin involved means dealing with / creating an MSRV policy for the crate. Maybe it would be good to hold off on doing more work on this change until zeroize is more stable? I'm not sure if there's any rush, and it seems like it won't be possible to undo this change without breaking stability, so I think it would be good to take it slowly.

[tarcieri]

@hdevalence I'm thinking pinned and unpinned cargo features, with pinned enabled by default to drive people to the new API, but the ability to shut it off and enable unpinned for legacy support.

If that works, I don't see any reason to ever remove unpinned support. I think it will remain permanently useful and necessary in certain circumstances, and would retain it in a 1.0 release which can hopefully happen fairly soon now that Pin support is stable.

That said, I've migrated to the 2018 edition in zeroize...

[tarcieri]

After playing around with adding Pin support to zeroize I've decided against it for now, and that it should be handled by a higher-level crate.

That said, zeroize v0.6.0 is out, and renames the marker trait for zeroizing with Default to DefaultIsZeroes: https://docs.rs/zeroize/0.6.0/zeroize/trait.DefaultIsZeroes.html

It also adds custom derive support for Zeroize if you are so inclined. It could be used for this PR potentially, if you're okay with proc macros:

https://docs.rs/zeroize/0.6.0/zeroize/#custom-derive-support

[isislovecruft]

This would be really nice to have instead of using clear_on_drop, since our usage of clear_on_drop is currently preventing cross-compilation and no_std builds from succeeding, which means embedded environments are going to likely have trouble with using curve25519-dalek.

I'm happy to implement DefaultWithZeroes for the remaining types, if that's okay. I agree that this shouldn't be a breaking change.

[isislovecruft]

We might want to switch to the proc_macro #[derive] that @tarcieri was mentioning later on in the thread.

[isislovecruft]

For some reason, @DebugSteven and I were unable to get the ZeroizeOnDrop proc_macro to work.

Another problem I ran into was the issue of zeroizing arbitrary things like Vec<Scalar> and Vec<[i8; 64]> etc, which we are currently using the wrapper struct ClearOnDrop for. @tarcieri is an equivalent to that something which zeroize would consider supporting? Otherwise we'll end up with a bunch of our own custom wrapper types.

[DebugSteven]

ZeroizeOnDrop can't be derived because it derives Drop (as well as Zeroize) & we can't derive Drop on a type that also derives Copy. MontgomeryPoint & Scalar both derive Copy.

My guess is that we'd want to implement special drop logic on type wrappers around those types where they are needed, probably similar to how I drafted the drop logic change in x25519-dalek. Unless we wanted to get rid of the Copy on those types, if that's possible.

[tarcieri]

@isislovecruft the proc macro is pretty bad right now. I need to rewrite it with synstructure. Apparently it is riddled with "my first proc macro" mistakes. It seems like most of the Drop handlers needed in curve25519-dalek are pretty trivial to write by hand by now, so if you're having problems with the proc macro I'd suggest just writing them by hand for now. I'll also add where I'm fairly confident in the semantics of zeroize itself, the proc macro is a bit scary and I should probably throw some more scary warnings on it.

Another problem I ran into was the issue of zeroizing arbitrary things like Vec<Scalar> and Vec<[i8; 64]> etc, which we are currently using the wrapper struct ClearOnDrop for.

There is not equivalent functionality in zeroize for this. I've considered opening a PR against clear_on_drop for this to replace its existing Clear trait with Zeroize (or implement Clear in terms of Zeroize) so all of the existing crates that use clear_on_drop could get WASM compatibility for free. If that's something @caesarb is interested in it's something I'd be happy to do.

Otherwise what I've been particularly interested in is building a higher-level secret-keeping library on top of zeroize, which leverages Pin to prevent things like Vec from making copies of data behind your back via reallocation. I think that's probably the best solution for these sorts of things going forward.

[isislovecruft]

What was still needed from this PR was merged in #306.