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

[11.x] Fix Bcrypt/Argon/Argon2I Hashers not checking database field for nullish value before checking hash compatibility #52297

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

localpath
Copy link
Contributor

@localpath localpath commented Jul 28, 2024

Backstory:
If we don't check for a nullish or empty value first before checking if a given $hashedValue is compatible with our apps algorithm, we'll always throw a runtime exception which means the end user won't receive the normally expected 401/422 or similar failed to authenticate, but rather a 500 server error which makes applications look broken.

Started seeing 500 errors for users attempting to login to the app after hash recheck landed in the framework. If a users password field is null or empty in the database they should receive a 401/422 or similar authentication exception/response. This has caused quite a bit of issues as you can imagine the context of a 500 level error vs a 401/422 which is expected if someone is trying to login but hasn't yet set a password.

Seems like this was an oversight in this feature being rolled out since we can see some inconsistencies and incorrect logical checks being missing or out of order in the hashing implementations.

Updates BcryptHasher ArgonHasher and Argon2IdHasher Hash implementations to correctly check if the $hashedValue field being checked for Hash Algorithm compatibility is not null or of length 0. This gives us the behavior before hash verification was added to the framework that will trigger the framework to return early with a proper response of false as opposed to a generic RuntimeException being thrown for a value that should never have been checked for hash algorithm compatibility.

To reproduce:

  1. Install Laravel 11.x and use Fortify or your flavor of starter authentication
  2. Create a user
  3. Set the users password field to null in the database
  4. Attempt to login with any password that meets the auth validation you have in place
  5. See a 500 level error response because of the missing statements being added here

image

To show original behavior:

  1. Repeat above 1-5
  2. Add HASH_VERIFY=false in your .env
  3. Attempt to login with the database password field still null
  4. See the framework issue the original, correct authentication failed response

image

Thanks for all you do and hopefully this makes it in the framework!

Updates Bcrypt Hasher implementation to correctly check if the `$hashedValue` field being checked for Hash Algorithm compatibility is not `null` or of length `0`. This gives us the behavior before hash verification was added to the framework that will trigger the framework to return early with a proper response of `false` as opposed to a generic `RuntimeException` being thrown for a value that should never have been checked for hash algorithm compatibility
@@ -18,13 +18,13 @@ class Argon2IdHasher extends ArgonHasher
*/
public function check(#[\SensitiveParameter] $value, $hashedValue, array $options = [])
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like a logic error to check for the hash compatibility before checking if the hashed value is even non-null or empty.

the original if statement would never be reached. contrast to the other hashing implementations that were missing this entirely

if (is_null($hashedValue) || strlen($hashedValue) === 0) {
            return false;
        }

@@ -67,6 +67,10 @@ public function make(#[\SensitiveParameter] $value, array $options = [])
*/
public function check(#[\SensitiveParameter] $value, $hashedValue, array $options = [])
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we don't check for a nullish or empty value first, we'll always throw a runtime exception which means the end user won't receive the normally expected 401 or similar failed to authenticate, but rather a 500 server error which makes applications look broken

@localpath localpath changed the title Fix Bcrypt Hasher not checking database field for nullish value Fix Bcrypt/Argon/Argon2I Hashers not checking database field for nullish value before checking hash compatibility Jul 28, 2024
@localpath localpath changed the title Fix Bcrypt/Argon/Argon2I Hashers not checking database field for nullish value before checking hash compatibility [11.x] Fix Bcrypt/Argon/Argon2I Hashers not checking database field for nullish value before checking hash compatibility Jul 28, 2024
@driesvints
Copy link
Member

cc @valorin

@taylorotwell taylorotwell merged commit 18b9bad into laravel:11.x Jul 29, 2024
28 of 29 checks passed
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.

None yet

3 participants