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

Password rehashing doesn't work when using basic / onceBasic #50627

Closed
joostdebruijn opened this issue Mar 18, 2024 · 11 comments
Closed

Password rehashing doesn't work when using basic / onceBasic #50627

joostdebruijn opened this issue Mar 18, 2024 · 11 comments

Comments

@joostdebruijn
Copy link
Contributor

joostdebruijn commented Mar 18, 2024

Laravel Version

11.0.7

PHP Version

8.3.4

Database Driver & Version

MariaDB 10.11.7

Description

The default hashing method of my application is argon2id. However, I've still some users with a bcrypt password.

PR #48665 introduces password rehashing - that's enabled in my application. However, if a user authenticates via basic or onceBasic, the password is not rehashed and the error is thrown. In Laravel 10 they were able to login (keeping their bcrypt hashed password), however in Laravel 11 an error is being triggered: This password does not use the Argon2id algorithm., which is true - but I would expect that they can login and that their password will be rehashed. If a user use the 'normal' login via Auth::attempt() it works properly.

I'm willing to prepare a PR to solve this, but I've no time to dive into it on short term - so I just created this issue for now.

Steps To Reproduce

  1. Create a user with a bcrypt hashed password in a Laravel 11 application
  2. Set the HASH_DRIVER to argon2id
  3. Try to login with the user via basic or onceBasic, the login attempt will fail
@jnoordsij
Copy link
Contributor

jnoordsij commented Mar 18, 2024

The issue is most likely the verify flag being true by default in the hashing config file in Laravel 11. Given that this is now included in the framework, it's probably even harder to spot the difference.

As I feared when encountering this issue myself, the message is a bit hard to grasp and debug, and moreover results in, in my eyes, confusing end-user behavior; see also #49834.

I think at least some elaboration in the release notes on this could be added, although I personally would still think disabling algorithm verification by default is the way to go.

Edit: to solve your problem for now, update your hashing config file to disable verification and hashes should be updated.

@driesvints
Copy link
Member

driesvints commented Mar 19, 2024

@valorin could you maybe confirm this?

Thanks @jnoordsij!

@joostdebruijn
Copy link
Contributor Author

Thanks @jnoordsij, very helpful! Can conform the old behavior is back when I set hashing.bcrypt.verify and hashing.argon.verify to false. Because both config keys were not defined, the default values were active.

However, the hashes are not updated after a onceBasic authentication, which I more or less would expect with hashing.rehash_on_login on true. Rehasing works, as expected, when using Auth::attempt().

@joostdebruijn
Copy link
Contributor Author

Sorry, I need to correct myself. With the configs hashing.rehash_on_login, hashing.bcrypt.verify and hashing.argon.verify explicitly to true, also an authentication via Auth::attempt() will not rehash and falls into This password does not use the Argon2id algorithm. error. If I set hashing.bcrypt.verify and hashing.argon.verify to false, rehashing works via attempt(), not via onceBasic and basic. I think those are two different problems, although related.

I agree with the post from @jnoordsij in #49834, that this is confusing default behavior.

@valorin
Copy link
Contributor

valorin commented Mar 19, 2024

Yep, looks like two different issues at play here.

  1. The hashing.*.verify flag was blocking the different algorithms being verified - I believe this is the intended default behaviour now?
  2. I missed the once*() authentication methods when implementing rehashing... 😔

The first is only an issue if you've got hashes with different algos (as the OP does), but the second is a bug that I believe should be fixed. The question is though, would it be considered a breaking change, or just a bugfix?

@valorin
Copy link
Contributor

valorin commented Mar 19, 2024

Just to add, I can't see a huge benefit to defaulting hashing.*.verify to true outside of preventing abuse through hashed casts - where a user could intentionally inject a very expensive hash to DoS the app. However, the flipside is most apps won't have different algorithms at use in hashing either, so... 🤷

That said, maybe a documented env var would help? HASHING_VERIFY, which you can set false easily to toggle it off in one go without overriding config?

@jnoordsij
Copy link
Contributor

Just to add, I can't see a huge benefit to defaulting hashing.*.verify to true outside of preventing abuse through hashed casts - where a user could intentionally inject a very expensive hash to DoS the app. However, the flipside is most apps won't have different algorithms at use in hashing either, so... 🤷

(Just wondering, definitely not an expert or something, just trying to learn and understand.) Is there any real life scenario where the user has that much access that it can actually influence the hashing algorithm in any way at creation time? Because I feel like that is always entirely dictated by the application; and even if not, it would be erroneous behavior anyways if a user can somehow influence the hash algorithm but it being denied usage in any way afterwards.

@valorin
Copy link
Contributor

valorin commented Mar 19, 2024

Yep, it's possible if you use Eloquent's Hashed Cast on your models.

The hashed cast checks if the password value is hashed before rehashing - to prevent you double-hashing the value when passing model values around. Unfortunately, this means that anyone submitting a hash in a password field can bypass the app hashing configuration and save a hash with a custom, and potentially malicious configuration.

It's the reason for this PR: #48814

valorin added a commit to valorin/laravel-framework that referenced this issue Mar 23, 2024
To help solve part of the issue reported in laravel#50627, exposing an env var
to toggle hash verification avoids the need to manually implement the
hashing.php file.
@valorin
Copy link
Contributor

valorin commented Mar 23, 2024

I made a PR that adds a simple env var for HASH_VERIFY to solve that part of the issue: #50718

I'll aim to PR a fix for once*() this week. 🤞

taylorotwell pushed a commit that referenced this issue Mar 24, 2024
To help solve part of the issue reported in #50627, exposing an env var
to toggle hash verification avoids the need to manually implement the
hashing.php file.
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@joostdebruijn
Copy link
Contributor Author

This one has been solved by #50843. Thanks all!

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

No branches or pull requests

5 participants