Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ZeroizeOnDrop for SHA 1..=2 and Blake2 #516

Closed
wants to merge 10 commits into from

Conversation

kayabaNerve
Copy link

@kayabaNerve kayabaNerve commented Nov 12, 2023

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' decompose functions. I would've done that with this PR yet the digest crate is in a distinct repo.

@newpavlov
Copy link
Member

Do not implement the Zeroize trait for cryptographic types. Users may execute it and get an invalid state. Drop + ZeroizeOnDrop will be sufficient.

@newpavlov
Copy link
Member

newpavlov commented Nov 12, 2023

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
Copy link
Author

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

@newpavlov
Copy link
Member

newpavlov commented Nov 12, 2023

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
Copy link
Author

... and then immediately after setting zeroes, set the initialization constants.

@kayabaNerve
Copy link
Author

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.

@newpavlov
Copy link
Member

newpavlov commented Nov 12, 2023

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
Copy link
Author

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.

@tarcieri
Copy link
Member

tarcieri commented Nov 12, 2023

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
Copy link
Member

The minimal versions CI failures are not relevant to this PR and should be fixed by RustCrypto/actions#34

@kayabaNerve
Copy link
Author

Happy to hear. I just pushed a commit which solely adds ZeroizeOnDrop for SHA 1..= 2 and Blake2 👍

@kayabaNerve kayabaNerve changed the title Implement Zeroize for SHA 1..=3 and Blake2 Implement ZeroizeOnDrop for SHA 1..=2 and Blake2 Nov 12, 2023
sha3/Cargo.toml Outdated Show resolved Hide resolved
blake2/Cargo.toml Outdated Show resolved Hide resolved
@kayabaNerve
Copy link
Author

Pushed what I believe to be resolutions 👍 Thanks for the tips :)

@kayabaNerve
Copy link
Author

Is there anything I can do to push this forward?

@newpavlov newpavlov mentioned this pull request Jan 11, 2024
@newpavlov
Copy link
Member

Closing in favor of #545.

@newpavlov newpavlov closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants