-
Notifications
You must be signed in to change notification settings - Fork 326
Implement ZeroizeOnDrop for SHA 1..=2 and Blake2#516
Conversation
Enables using zeroize with a distinct name, enables cleanly working around msrv requirements.
newpavlov
commented
Nov 12, 2023
Do not implement the Zeroize trait for cryptographic types. Users may execute it and get an invalid state. Drop + ZeroizeOnDrop will be sufficient.
Also it's probably worth to wait for the next breaking release of digest which will bump MSRV to 1.65 and add zeroize support during migration to it.
kayabaNerve
commented
Nov 12, 2023
This does not return an invalid state. It's effectively a reset with a guarantee the prior state is gone. Please read the implementation.
Also, I am unable to get CI happy 0_o
You literally set zeros into hasher states, which may cause security issues if such state is to be accidentally used by users. For proper reset you should use the initialization constants. You SHOULD NOT implement the Zeroize trait for types which may contain and rely on any kind of inner invariant. It's misuse of the trait.
kayabaNerve
commented
Nov 12, 2023
... and then immediately after setting zeroes, set the initialization constants.
kayabaNerve
commented
Nov 12, 2023
If you'd rather it directly set the initialization constants, that can be done. It'd just require re-implementing the zeroize's crate internal mechanisms to avoid being optimized out by the compiler. That would duplicate code (several times), notably increase maintenance effort, and be at greater risk of failure IMO.
Yes, you are right, but it's not "zeroization" anymore, isn't it? The point of the Zeroize trait is to perform "volatile" erasure of secrets and it's usually used to erase "primitive" values. Erasure of "complex" types is done using Drop impls which use Zeroize under the hood.
I will repeat myself: you should not implement the Zeroize trait for the hasher types. See the other algorithm crates which support zeroize, e.g. the block cipher crates.
kayabaNerve
commented
Nov 12, 2023
It's not zeroization, yet the intent is to erase the prior state. This does perform exactly that erasure. I'm unsure of anyone who performs zeroization and then reads the memory, always expecting zeroes (no matter the type) like it's a calloc call.
As for whether or not this should solely implement ZeroizeOnDrop... Sure. I'm completely fine with that and can edit this PR to do it that way, per your preference 👍 I only did it this was initially as it leaves the user with greater choice over when they want to zeroize.
Generally Zeroize is useful for the low-level types that store state like integers and slices, and higher-level constructions benefit from ZeroizeOnDrop as there's no need to explicitly call zeroize (prevents that from being forgotten) and doing it on drop instead of manually prevents a use-after-zeroize condition.
With ZeroizeOnDrop, you can just enable the zeroize feature and you get zeroization for free with no additional changes.
newpavlov
commented
Nov 12, 2023
The minimal versions CI failures are not relevant to this PR and should be fixed by RustCrypto/actions#34
kayabaNerve
commented
Nov 12, 2023
Happy to hear. I just pushed a commit which solely adds ZeroizeOnDrop for SHA 1..= 2 and Blake2 👍
kayabaNerve
commented
Nov 12, 2023
Pushed what I believe to be resolutions 👍 Thanks for the tips :)
kayabaNerve
commented
Dec 5, 2023
Is there anything I can do to push this forward?
newpavlov
commented
Jan 11, 2024
Closing in favor of #545.
Uh oh!
There was an error while loading. Please reload this page.
Relevant to #87.
Resets the hasher state after calling zeroize so it's still usable (1 and 2) indistinguishable from a newly constructed hasher.
This needs a impl Zeroize where Wrapped: Zeroize on CoreWrapper and associated in digest to be pleasantly effective. It's technically reachable now via wrappers'
decomposefunctions. I would've done that with this PR yet the digest crate is in a distinct repo.