Use Hash::needsRehash in HasAttributes::castAttributeAsHashedString instead of Hash::isHashed

[bastien-phi]

Implementation of hashedcast fix. The fix is based on Hash::needsRehash which will check if the hash was generated with the same configurations as the default hasher. (https://www.php.net/manual/en/function.password-needs-rehash.php)

It will causes a breaking change, if the developer currently creates the User with hashed cast and modifying the configuration, for example :

User::create([
    'password' => Hash::make($password, ['rounds' => 14])
]);

With this new implementation, the password will be hashed twice. The probability is, IMHO, quite low and the fix is quite easy as it only requires to drop the hashed cast. It should not be a problem as the developer already hashes the password by hand.

Unfortunately, this implementation also requires changes in the laravel/laravel project.

Indeed, the User declares hasehd cast and UserFactory declares

'password' => '$2y$12$Z/vhVO3e.UXKaG11EWgxc.EL7uej3Pi1M0Pq0orF5cbFGtyVh0V3C', // password

To speed up tests, in phpunit.xml, environment variable BCRYPT_ROUNDS is overrided to 4.

As model factories can be used in tests (rounds 4 by default) and seeders (rounds 12 by default), we cannot define a pre-hashed value because of a double hashing in tests.

Therefore, there only viable solution I can think of is to drop the pre-hashed value and define

'password' => Hash::make('password'),

This way, the hash will always be generated with the correct config and won't be double hashed.
Another solution would be to defined

'password' => 'password',

and let the cast do its job.

It won't slow down too much the tests as the BCRYPT_ROUNDS is at its minimum.
Unfortunately, this change needs to be made on every project using hashed cast on user's password (and the other models/attributes using the cast).

@valorin You proposed to block if the rounds if more than X above the config value. I don't think it will fix that UserFactory issue because of the difference between the config in the pre-hashed value (12) and the config (4) during the tests.

I hope we could have a better idea to solve initial issue, at this time I don't...

@taylorotwell @valorin I wait for your reviews before targeting laravel/framework

[valorin]

I was thinking that rather than using needsRehash to check if the hash config is different, it should extract the number rounds and compare them against the number of rounds set in the hasher itself - not the app config but the Laravel default (i.e. 12, even if the app has them set to 4). That's easy to do for bcrypt, although Argon has a few different options which make it messier. This could be done with a new method on the hasher itself, but we're venturing into the realms of complexity.

We'd need to duplicate the $rounds setting in BcryptHasher so when it's set on the app level (and the cost options in Argon, etc), we still have the original value, and then add a new method to check the hash config is equal or less than the original values + buffer. For BC support, this method could be added into AbstractHasher and just passes back to needsRehash().

It feels like a lot of extra complexity, but I'm worried that if we don't support a range of hash configs, it will break stuff - as you've pointed out with the User Factory. Consider if the app changes it's rounds from 10 -> 12, any existing hashes being passed around and set on user models will get hashed again - it feels like a very strange use case, but not completely outside possibility. But this is only an issue if we're targeting 10.x - if we make it a breaking change and target 11.x, it can be documented and we're all good.

So I think we either need to bump this to 11.x as a breaking change or add the complexity to handle it in a flexible way.

[bastien-phi]

@valorin I'm not sure to understand everything well

compare them against the number of rounds set in the hasher itself - not the app config but the Laravel default
Do you want to compare it with hard-coded 12? What about if the developer set rounds to 14 in their config ?

Something like

class BcryptHasher extends AbstractHasher implements HasherContract
{

    protected $defaultRounds = 12;

    protected $rounds = 12;

    public function __construct(array $options = [])
    {
        $this->rounds = $options['rounds'] ?? $this->rounds;
        $this->defaultRounds = max($this->defaultRounds, $this->rounds); 
        ...
     }

     // ...
}

?

But this is only an issue if we're targeting 10.x - if we make it a breaking change and target 11.x, it can be documented and we're all good.

I'm not comfortable with only targetting 11.x. This would mean let the security issue in 10.x forever.
Maybe we could fix with needsRehash in 11.x as it is breaking (and convert hard-coded hash in UserFactory into Hash::make() call) while in 10.x we would use a more complex/flexible fix.

I will try to take a look on this tomorrow, between 2 client projects...

[bastien-phi]

Here is my new attempt to solve the problem in a non-breaking way.

I let @taylorotwell fix the naming, I know you will find a much better one than mine.

About the implementation
I believe that fixing it in a breaking way using Hash::needsRehash() and modifying the laravel skeleton is the way to go to have the issue properly secured.
That's why I decided to hard-code magic values in bcrypt and argon hashers. Theses values allows the developers to increase hash complexity in ways it should not impact their application. The maximum computation time the values can allow is about 2.5 seconds.
It corresponds to rounds: 16 for bcrypt. For argon, it was a bit harder to determine. Duration increases with memory_cost and decreases with threads. I think that the ratio memory/threads should not be greater than 4 times PASSWORD_ARGON2_DEFAULT_MEMORY_COST and the threads should not be greater than 16 (if not limited, we could increase a lot memory and thread keeping the ratio below the threshold). It let the developer adjust argon's parameter while protecting them from a possible attack.

@valorin what's your point of view about this implementation ?

[valorin]

Nice, that's basically was I was thinking! Good work solving the Argon question too.

My only concern is that the values are hardcoded away from the class default - so they could potentially be missed and not updated, but that's probably years off.

[bastien-phi]

I don't think your concerns are not a real problem as this should be a temporary fix for Laravel 10 which will get security fixes only until February 2025.