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

Add explicit zeroize features #539

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Add explicit zeroize features #539

merged 1 commit into from
Nov 25, 2024

Conversation

ros-cr
Copy link
Contributor

@ros-cr ros-cr commented Nov 25, 2024

During a recent code audit on another project, I noticed that the argon2, balloon-hash and bcrypt-pbkdf crates implemented in this code repository do not explicitly declare their zeroize feature variants for memory sanitization behavior.
Fix this by adding explicit Cargo.toml [feature] entries for this zeroize flag.

By my current understanding, the relevant optional security hardening functionality which is gated by this feature flag can already be used by other crates today if they opt-in to its use in their [dependencies] reference, without requiring this patch because the relevant functionality gets picked up as an "implicit feature".

However, by not explicitly announcing this optional security feature, some consumers will either miss that it exists, or do not regard it as a reliable stable functionality (which this PR assumes it is). See also Rust RFC3491 which will remove this implicit feature behavior in the future.

Prior example of this change: see sha1-checked.

Thanks to @hko-s for helping to debug this.

I do not consider this a direct security issue (as covered by SECURITY.md), because the hardening effect of memory sanitization is optional anyway and not guaranteed in existing documentation.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We relied on implicit crate features because we previously had MSRV older than 1.60, but since we have bumped MSRV to 1.81 it's indeed worth to use explicit features everywhere.

@newpavlov newpavlov merged commit fa7df79 into RustCrypto:master Nov 25, 2024
31 checks passed
@ros-cr
Copy link
Contributor Author

ros-cr commented Nov 25, 2024

That makes sense!
Thank you for the super-quick feedback and merge 👍

@ros-cr
Copy link
Contributor Author

ros-cr commented Nov 25, 2024

@newpavlov : with regards to "use explicit features everywhere", I expect that for example the handling of rayon and password-hash dependencies/features could probably be improved via the dep: syntax within this repository. That's out of scope for this PR goal, though, and just a quick hint if you're not already aware of it.

@ros-cr
Copy link
Contributor Author

ros-cr commented Nov 25, 2024

@newpavlov also note that the MSRV column in the README.md overview table appears to be outdated.

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.

2 participants