Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
kayabaNerve wants to merge 10 commits into
RustCrypto:master from
kayabaNerve:zeroize
Closed

Implement ZeroizeOnDrop for SHA 1..=2 and Blake2 #516
kayabaNerve wants to merge 10 commits into
RustCrypto:master from
kayabaNerve:zeroize

Conversation

@kayabaNerve

@kayabaNerve kayabaNerve commented Nov 12, 2023
edited
Loading

Copy link
Copy Markdown

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.

AaronFeickert reacted with hooray emoji

Copy link
Copy Markdown
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 commented Nov 12, 2023
edited
Loading

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
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 commented Nov 12, 2023
edited
Loading

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
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 commented Nov 12, 2023
edited
Loading

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
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 commented Nov 12, 2023
edited
Loading

Copy link
Copy Markdown
Member

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.

kayabaNerve reacted with thumbs up emoji

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Comment thread sha3/Cargo.toml Outdated
Comment thread blake2/Cargo.toml Outdated

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

Is there anything I can do to push this forward?

Copy link
Copy Markdown
Member

Closing in favor of #545.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@newpavlov newpavlov newpavlov left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /