Basic Zeroize support

[afck]

Keeping #14 open for now:

• For some types Zeroize can't be derived and would be difficult to implement, e.g. because they contain vectors. We should evaluate which types potentially contain secret information.
• If the zeroize feature is enabled, we should also use it in internal computations, and clear all temporary data that is potentially secret.

On the other hand, I'd also be happy to remove Zeroize from the groups again, if we are confident that in practice, only the field elements will be secrets.

[hdevalence]

As a downstream user, it would be convenient to have external types implement Zeroize, but I personally don't think that it's worthwhile to use zeroize for internal computations, because this adds cost but doesn't provide a benefit (as it doesn't actually prevent secret leakage).

[afck]

But if I use the zeroize feature I would expect my secrets to be cleared from memory after

let fp: Zeroizing<Fp> = Zeroizing::New(create_secret());
let fp_bytes: Zeroizing<[u8; 48]> = fp.to_bytes()

go out of scope. If we don't clear the temporary value used in to_bytes, that is not the case. Doesn't that defeat the purpose of using zeroize in the first place?
Also, the overhead would only affect the users that enable the feature.

[ebfull]

It'd be great to get @tony-iqlusion's opinion of this.

[tarcieri]

For some types Zeroize can't be derived and would be difficult to implement, e.g. because they contain vectors.

If you have specific places custom derive support is lacking, let me know. Being able to use custom derive on complex data structures is the whole point of zeroize_derive.

As for using it on internal computations, it’s up to you. I do, but I can certainly sympathize with the performance drawbacks. I’m not sure there’s a good way to have a fallback for Zeroizing other than making your own wrapper newtype with appropriate deref coercions which doesn’t zeroize.

[ebfull]

@tarcieri ah okay, that's your github username. Sorry! I looked on your personal website to find it but couldn't.

I think @hdevalence is probably right, but in any case, it seems like a good "first step" would be supporting downstream users that want to use zeroize, and then moving the discussion about internal computations to a separate issue. (Closing #14 would be appropriate for the "first step" of this approach.)

I don't think we can safely assume that group elements are not secrets. We especially cannot assume projective points are not secret (when the scalar is assumed to be a secret) since e.g. the z-coordinate leaks some detail about the scalar, see this old example.

[hdevalence]

So, two issues that I'm not sure are clearly distinct in the above discussion:

1. Whether the bls12_381 types should implement Zeroize;
2. Whether the bls12_381 types should use the zeroize-on-drop (or other) functionality to automatically always clear themselves as they pass out of scope.

I think there's no reason not to implement (1) for both internal and external types, because it means that downstream users can opt in to using zeroizing wrappers if they want. But (2) has a pretty significant cost – it forces zeroing of every stack temporary, for instance – and doesn't actually ensure that all traces of secret data are cleared. For instance, because Rust doesn't give control over stack allocations, there's no way to ensure that all derivatives of secret data are cleared, but if this doesn't matter then there was no reason to try to wipe stack temporaries in the first place.

The code that actually creates all these temporaries has to be trusted anyways (it's the thing actually doing the computation), so it will end up repeatedly wiping some subset of its data, but probably not enough to actually clear all traces of secrets. So one could go further, and try to wipe the entire stack and heap allocations after the secret-using computation is finished. To do this more perfectly, one could have a system that tracks every memory page, stack or heap. And it could then align that tracking to the hardware's isolation capabilities, preventing Spectre attacks. But this would just be "using a separate OS process".

To put it another way, I think that any attempt to do memory zeroing is really an implicit specification of a security boundary between a "trusted time+code" and an "untrusted time+code", that implicitly specified security boundaries are usually not real boundaries, and that explicitly specifying that boundary basically amounts to reinventing the process abstraction. So, as a downstream user, my preference would be to do (1) and not (2).

[afck]

If you have specific places custom derive support is lacking, let me know.

@tarcieri: In this case it's just that G2Affine contains a field of type Choice, so we'd need to add zeroize support to the subtle crate as well first. Not zeroize_derive's fault.

For now, I could implement Zeroize for G2Affine manually, and not clear the infinity bit. Or is it better to not implement it for now? Should we add zeroize support to subtle?

@hdevalence: I'm not proposing to do (2) at all; users can just wrap bls12_381's types in Zeroizing wrapper to get zeroize-on-drop. My question was rather:

3. Whether if the zeroize feature is enabled, we should zeroize temporary values inside bls12_381's functions themselves, e.g. the tmp in to_bytes.

I'm not sure how likely it is for values to leak anyway (when does that happen — if the stack gets reallocated and moved? Or if the process gets swapped out?), but I'd expect zeroizing to prevent in the vast majority of cases that deallocated memory still contains secrets. If we don't clear bls12_381's internal temporary variables, it will very often contain secrets.