Repository navigation
fix(server): accept $2x$ in hash check and equalize unusable-hash timing - #357
Merged
Merged
Conversation
…le hashes reload_users warned that users with a $2x$ bcrypt hash cannot log in, although verify_internal dispatches every $2 hash to bcrypt, which accepts version 2x. Add $2x$ to is_supported_hash_format, the warning text, and the UserDefinition::password_hash docs. Dispatch is unchanged. Users whose stored hash is unusable (unsupported prefix, malformed Argon2, malformed bcrypt) returned without any hash work, unlike unknown users. Call verify_dummy_hash on those three paths so they cost the same as other failures; the malformed Argon2 case still returns the same Err and the auth provider still rejects. A cfg(test) counter on verify_dummy_hash lets tests assert this deterministically instead of by wall-clock time. Tests: cargo test --lib server::auth::password, cargo clippy --lib --tests and --lib with -D warnings, cargo fmt --check. Closes #356
inureyes
added
status:review
An Argon2 PHC string such as `$argon2id` or one missing the salt or hash output parses, so it passed the load-time format check with no warning, and argon2 reports PasswordInvalid for it without hashing, so it also skipped the dummy hash added for unusable hashes. Reject such hashes in `is_supported_hash_format` and return the malformed-hash error from `verify_argon2`, which routes them through the dummy-hash path. Login outcome is unchanged: these users still cannot authenticate. Found by the security review of this PR. Tests cover the three incomplete forms in the format check and the dummy-hash counter test, and both fail without the fix. Also rename the bad-version bcrypt fixture variable to match what it holds. Refs #356
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #354 / #355. Users with a
$2x$bcrypt hash can log in but triggered the load-time "cannot log in" warning, and users with an unusable stored hash returned faster than users with a real hash, which the 100 ms floor inverifyonly hides on fast hosts. Login outcomes are unchanged for every input;verify_internal's$2dispatch is untouched.is_supported_hash_format, the load-time warning text and theUserDefinition::password_hashrustdoc now list$2x$.verify_dummy_hashnow runs, matching the invalid-username and unknown-user branches, for: unsupported-format hashes (*,!, empty,$6$), malformed Argon2 hashes (the originalErris still returned, soverify_passwordstill logs it and rejects), and malformed bcrypt hashes (bad version, truncated).$argon2idor$argon2id$v=19$m=19456,t=2,p=1) are now treated as malformed:is_supported_hash_formatrejects them, so they get the load-time warning, andverify_argon2returns the malformed-hash error, so they take the dummy-hash path. Before, argon2 reported them as a wrong password without hashing anything.#[cfg(test)]call counter onPasswordVerifierlets tests assert exactly one dummy hash per unusable-hash or unknown-user attempt and none for valid users, with no wall-clock assertions. It is absent from non-test builds.Changes during review
The incomplete-Argon2 case was found by the security review and folded in here since it is the same unusable-hash timing gap #356 targets. Both tests covering it fail with the check removed.
Verification
cargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warningsandcargo clippy --lib -- -D warningsare clean.cargo test --no-fail-fastpasses every target except the fourtests/integration_test.rstests, which fail identically on cleanmainlocally (localhost SSH sandbox) and are left to CI.Closes #356