[10.x] Increase bcrypt rounds to 12

[valorin]

PHP is increasing the default bcrypt cost to either 11 or 12 to keep up with increases in computing, so we should do the same within Laravel. The current default of 10 was set in PHP 11 years ago, which is no longer a suitable default.

12 appears to be the sweet spot between performance and security, as confirmed by a member of the Hashcat team. Symfony uses a cost of 13, however that may be too high for some servers.

Due to the way hashing works, there are no backwards compatibility issues - older passwords with lower rounds will still be handled properly, and code that automatically rehashes passwords will upgrade them over time. It's also worth pointing out that since rounds are defined in config/hashing.php, existing projects won't automatically get the new rounds cost and thus won't have any performance impacts. The RFC contains hash calculation timings if you'd like more information on the impacts.

Increasing rounds to 12 in config/hashing.php should be a recommended upgrade step for Laravel 11 (and possibly added to the guide for 10?).

Application Skeleton PR: laravel/laravel#6245

[GrahamCampbell]

This change is not appropriate to make on the 10.x branch of laravel/framework.

[valorin]

Why not?

[GrahamCampbell]

Because this is not a bug fix or new feature. It's an unexpected change that should go in a major release, and not a minor one.

[roycewilliams]

Very fair! Though one could certainly argue that, given how easily bcrypt cost 10 can be cracked at scale, this change could quite reasonably be classified as a bugfix for a security weakness (for which the fix is simply long overdue).

But that's just from my perspective as someone who simulates the attacker, not as a developer. 😁

[valorin]

I consider this a security upgrade.

I had considered putting it 9.x, but since it's hardcoded in the app skeleton and you aren't likely to create a new app from 9.x, I PR'ed it into the 10.x config file. I've updated the framework to match the default in the config file so there aren't two defaults.

It won't affect any existing apps either, so it shouldn't have any side effects.

[gocanto]

Adding on @GrahamCampbell - I would suggest having a method here that allows you to set this value as Laravel does it all over the place.

Say

getRounds(): foo
{
 return $this->rounds;
}

Here, you can extend it and leave the default behaviour while allowing people to hook into it. ✋🏼

[valorin]

Sorry, ignore my last comment - I was confused with something else.

You can easily set it outside the class. What's the use case for extending the class itself?

[driesvints]

@GrahamCampbell I don't think this is a breaking change though? The only thing that'll happen is that stronger passwords will be created. Both the check and needsRehash will still work as expected. needsRehash will start to return true as soon as it picks up an old password with the old rounds which is what you want really.

If I see it wrong definitely feel free to point that out.

[mfn]

Only installations using this class directly without Framework would be immediately affected or those who changed the default value in config/bashing.php and set it to null (🤔).

In the majority of the cases the property will be overridden anyway due to https://github.com/laravel/laravel/blob/88695a7cf4b1fb2a248d129f8b53400ec65ddf8f/config/hashing.php#L32 .

So, to change the default for new installation, we've to change it in both places and with the default, existing ones will not be adapted.

ps: not making a case for whether this should go into L10/L11, just pointing out that also the config would need to be adapted.

[GrahamCampbell]

Breaking for people directly making the class.

[driesvints]

@GrahamCampbell but please explain how this would break anything?

[GrahamCampbell]

new BcryptHasher uses a different number of rounds, before and after.

[driesvints]

Again, I don't see the breaking change 😅

[GrahamCampbell]

It;s the same issue as if we had a sleep method with a default of 1 second, then we changed it to 10 seconds.

[valorin]

The benchmarks indicate the time increase is around ~200ms, so it'll be barely noticeable - if at all. Password hashing is supposed to be slow anyway.

If you're using the defaults for a security feature, you expect those defaults to track the recommended levels without breaking anything. This isn't a frivolous change, it's a security upgrade that follows recommendations from experts.

That said, it can be bumped to 11 if it needs to be, but I don't see the issue with making the change in 10. 🤷

[taylorotwell]

I don't think this is really breaking? One of the benefits of bcrypt is the cost is baked into the hash so that all existing hashes continue to work.