[11.x] Prevents Bcrypt hashing from spilling over and creating insecure hashes.

[waxim]

Recently Okta had a security incident created by them using bcrypt hashes that were over 72 chars in length, bcrypt implementations generally allow strings longer than 72 charachters, but simply omit the characters over that length. This can lead to predictable hashes if you can "stuff" the string with 72 known characters.

Take this simple example from Laravel Framework 11.41.3.

<?php

namespace App\Console\Commands;

use Illuminate\Console\Command;
use Illuminate\Support\Facades\Hash;
class TestBcryptSpillOver extends Command
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'bcrypt:test';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Test bcrypt spill over';

    /**
     * Execute the console command.
     */
    public function handle()
    {
        // Make our prefixes 72 characters long
        $my_hash_prefix = "some.very.long.prefix-"; // 22 characters
        $my_hash_key = "lnHQkcD7fZdMzJt640xbeOhzDGCI3aki7mePMxQgEQBkuUuwwm"; // 50 characters
        $super_secret_password = "thisIsMySuperSecretPassword";
        $random_password = "thisIsARandomPassword";

        $hash_one = $my_hash_prefix . $my_hash_key . $super_secret_password;
        $hash_two = $my_hash_prefix . $my_hash_key . $random_password;

        $computed_hash_one = Hash::make($hash_one);
        $computed_hash_two = Hash::make($hash_two);

        $this->info("Hash one: " . $computed_hash_one);
        $this->info("Hash two: " . $computed_hash_two);

        $this->info("Checking hashes for match");

        $check_one = Hash::check($hash_one, $computed_hash_one);
        $check_two = Hash::check($hash_two, $computed_hash_one);

        if($check_one && $check_two) {
            $this->info("Hashes match, bcrypt is vulnerable to spill over");
        } else {
            $this->info("Hashes do not match, bcrypt is not vulnerable to spill over");
        }
    }
}

Running this, should return us Hashes do not match, bcrypt is not vulnerable to spill over because $hash_two should not be the same as $computed_hash_one but it doesn't, it returns Hashes match, bcrypt is vulnerable to spill over because of the spill over.

This change, very simply rejects strings longer than 72 chars, when passed to Hash::make() when using Bcrypt.

Running the same code above after my change, throws an exception instead of silently failing.

Resources

1. https://trust.okta.com/security-advisories/okta-ad-ldap-delegated-authentication-username/
2. https://n0rdy.foo/posts/20250121/okta-bcrypt-lessons-for-better-apis/

[AhmedHaroon]

While this is a security fix it may break existing applications running Laravel 11. Therefore, I suggest that you target Laravel 12 for the new default behavior of throwing an exception and you may create another pull request to backport it to Laravel 11 with opt-in flag to enable exception throwing.

[waxim]

Could do, happy to have a config value in hashing.php, but this only affects newly created hashes and not stored ones, they'll all still validate (albeit insecurely) so, I think we're probably okay, no?

[taylorotwell]

I think since doesn't actually matter much for the typical user password storage scenario where you aren't stuffing passwords with other values, I would rather table this for Laravel 12 or even make it configurable.

[waxim]

Okay, but people use Hash::make for way more than passwords 🤷

[valorin]

The Okta issue wasn't due to raw passwords being 72+ bytes. They were prepending usernames to passwords when generating cache tokens - which is a completely custom and unusual use-case. It doesn't really apply here.

Hash::make() generates a fresh hash with a unique salt, so it's specifically designed for storing passwords. What other use cases are you thinking of?

[waxim]

I've seen people in userland use Hash::make for all kinds of "encrypted" values, including with appended prefixes (which could in theory spill over 72) but also, I just thought since bcrypt in PHP is broken, why not do this just in case, basically no overhead and it "polyfills" the broken implementation.

But clearly I'm alone. 🤷

[valorin]

I don't think the issue is unique to PHP though? My understanding is that it's the underlying Blowfish algo, and not unique to PHP.

Please don't misunderstand me, I'm not opposed to the change, but I just don't see a big need for it. I actually asked the question recently over on https://securinglaravel.com/security-tip-should-you-limit-password-lengths/, but there wasn't really a clear consensus from the various discussions it spawned.

That said, if you think it's important, I'd suggest following Taylor's recommendation and PRing it into 12 so it can be discussed in that context.

I'd be interested to see what sort of discussion it creates, and will happily share the link to get more feedback on it. 🙂

[waxim]

I'll give it a go tomorrow.

Yes it's not unique to PHP, but lots of libraries (in other languages) do add the additional check on top of the system implementation, because frankly why not?

Laravel is a "Batteries Included" framework, and if we can do things that help developers make safer tools, I think we should. A check as simple and low rent as this just seems like such a no brainer.

[waxim]

An edited version that uses a config value to enable / disable the check, targeted at Laravel 12.

#54509

/cc @taylorotwell and @valorin