Skip to content

argon2: detect allocation failures in hash_password_into - #568

Merged
newpavlov merged 6 commits into
masterfrom
argon2/alloc_zeroed
Mar 5, 2025
Merged

argon2: detect allocation failures in hash_password_into#568
newpavlov merged 6 commits into
masterfrom
argon2/alloc_zeroed

Conversation

@newpavlov

@newpavlov newpavlov commented Mar 4, 2025

Copy link
Copy Markdown
Member

Closes #566

@newpavlov newpavlov mentioned this pull request Mar 4, 2025
Closed
@newpavlov
newpavlov requested a review from tarcieri March 4, 2025 11:59
newpavlov
newpavlov commented Mar 4, 2025
View reviewed changes
Comment thread argon2/src/error.rs Outdated
jonasmalacofilho
jonasmalacofilho reviewed Mar 4, 2025
View reviewed changes

@jonasmalacofilho jonasmalacofilho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that my allocation-only benchmark in #566 suggests this could be a tiny bit slower than vec![Block::default, ...].

So maybe it would be good to benchmark this with high-ish memory sizes, or at least with the 19 MiB and 46 MiB sizes recommended by OWASP?

Comment thread argon2/src/block.rs
Comment thread argon2/src/error.rs Outdated
@tarcieri

tarcieri commented Mar 4, 2025

Copy link
Copy Markdown
Member

Maybe we should try to land #547 first so we can gauge any impact on performance?

tarcieri
tarcieri reviewed Mar 4, 2025
View reviewed changes
Comment thread argon2/src/block.rs Outdated
tarcieri
tarcieri reviewed Mar 4, 2025
View reviewed changes
Comment thread argon2/src/error.rs Outdated
@newpavlov

newpavlov commented Mar 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Maybe we should try to land #547 first so we can gauge any impact on performance?

I think we should merge this PR, then #547 without the alignment workaround after it was migrated to Blocks, and then finally add the alignment workaround to Blocks in a separate PR. I am still not 100% convinced that we should add the workaround. It needs additional investigation in my opinion.

@newpavlov
newpavlov merged commit 6679cbd into master Mar 5, 2025
@newpavlov
newpavlov deleted the argon2/alloc_zeroed branch March 5, 2025 13:14
@jonasmalacofilho jonasmalacofilho mentioned this pull request Mar 8, 2025
Merged
@tarcieri tarcieri mentioned this pull request Aug 27, 2026
Merged
tarcieri added a commit that referenced this pull request Aug 27, 2026
@tarcieri
argon2 v0.6.0 (#931)
b1e0ad6 
## Added
- Detect allocation failures in `hash_password_into` (#568)
- `parallel` feature (#547)
- `PasswordVerifier<str>` impl (#779)
- `kdf::{Kdf, Pbkdf}` impls (#823)

## Changed
- Upgrade to Rust 2024 edition; MSRV 1.85 (#563)
- Switch from `std::error::Error` to `core::error::Error` (#767)
- Rename `simple` feature to `password-hash` (#776)
- Bump `cpufeatures` to v0.3 (#847)
- Bump `password-hash` to v0.6 (#848)
- Bump `blake2` to v0.11 (#929)

## Removed
- `std` feature (#768)

## Fixed
- Check `p_cost < Params::MIN_P_COST` before `m_cost < p_cost * 8` (#745)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tarcieri tarcieri tarcieri approved these changes
+1 more reviewer
@jonasmalacofilho jonasmalacofilho jonasmalacofilho left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Argon2::hash_password_into should use fallible memory allocations

3 participants

@newpavlov @tarcieri @jonasmalacofilho