Harden user_pass_rehash() against attack

This issue was discussed by the Drupal Security Team.

It was agreed that it can be handled in public as a security hardening; we can use this issue.

The problem is that there is significantly less entropy in the inputs to user_pass_rehash() if the user has an empty password field in the database.

Users may have empty passwords if - for example - a site uses a Single Sign On integration to offload authentication.

Improvements could be made to the implementation to reduce the likelihood that an attacker could brute force e.g. a password reset URL if the user has no password stored in Drupal.

It may also be advisable for sites / projects that do not use Drupal's passwords for authentication to avoid leaving the password field empty in the database, but that can be looked at in a (public) follow-up. It wouldn't help much to set the password field to the same value for all users.

mcdruid → credited catch → .

mcdruid → credited greggles → .

mcdruid → credited Heine → .

mcdruid → credited moshe weitzman → .

mcdruid → credited poker10 → .

mcdruid → credited pwolanin → .

Adding credit from the private DST discussion - which is where the idea for this MR came from.

📌 [D7 backport] Harden user_pass_rehash() against attack Active

Not sure if tests can be a follow up with security improvements but think this should get some test coverage.

More tests are always good, but I'm not sure what additional tests we'd add here.

There are already tests which cover functionality where user_pass_rehash() is used to construct URLs - e.g.:

• \Drupal\Tests\user\Functional\UserPasswordResetTest
• \Drupal\Tests\user\Functional\UserCancelTest

The idea of the improvement being suggested is to reduce the chances of a successful brute force attack, and that's not necessarily an easy thing to test.

Open to suggestions of what testing we could add, but I think we may have to be satisfied that existing tests prove this change is not introducing a regression.

What about a test that checks that two users with an empty password and the same last login timestamp get different hashes? That wouldn't prove that there's not some other vector for hash collision, but it would prove that we got rid of this one.

Right, good plan.

Added a basic KernelTest that illustrates a possible hash collision when there are empty passwords in the db.

I've manually verified that the test fails without the change to user_pass_hash().

Sorry, I've not kept up with how we're doing (the equivalent of) test-only patches these days, but I'll try to find out in the next couple of days.

@mcdruid in the gitlab pipeline UI, there's a 'test only' job which reverts any non-test changes, then runs only the tests changed in the MR, it doesn't work for everything but it works for most.

Ah, interesting thanks.

The test only job seems to have passed, but also shows the expected failure:

https://git.drupalcode.org/project/drupal/-/jobs/506479

Testing Drupal\Tests\user\Kernel\UserPassRehashTest
F                                                                   1 / 1 (100%)
Time: 00:00.568, Memory: 4.00 MB
There was 1 failure:
1) Drupal\Tests\user\Kernel\UserPassRehashTest::testUniqueHashNoPasswordValue
Failed asserting that 'VxdDGWYOnSlfWVeVDwheEkF38Avnca_EsLcz3fdfMm4' is not equal to 'VxdDGWYOnSlfWVeVDwheEkF38Avnca_EsLcz3fdfMm4'.
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/project/drupal/core/modules/user/tests/src/Kernel/UserPassRehashTest.php:50
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

Do we need anything else in the MR?

I'd think this will need a CR; apart from anything else any reset URLs that have been sent out but not yet used will become invalid (although they only last for 24hrs by default IIRC).

The MR is not rebased, but I thought we fixed that in 🐛 Test-only job shouldn't require constant rebases to detect which files were changed. Active , so there is no need to rebase when running test-only pipeline.

I have created a simple draft CR here: https://www.drupal.org/node/3409738 →

And also created two follow-ups to fix the behavior of the test-only job from above here:

🐛 Test-only job reverts unrelated changes in non-rebased MRs Active
🐛 Test-only job does not detect failures correctly Needs review

Reviewing MR 5823

Test coverage is there per #22

CR reads fine to me.

Think this is good.

One small comment on the test, otherwise this looks good to me.

Thanks; thought I'd tried that but I must have been doing something wrong..

Thanks! Back to RTBC.

Out of interest, but can someone explain (if allowed) how separating the values (which are unique) with a : makes it so that the hashes are unique? I guess that was explained in private DST, but we can't see that discussion 🙂

See the test.

When concatenating unique id and email like these two pairs:
- 123 / example@test.com
- 12 / 3example@test.com

You’d get the same string without the “:”.

Ah, I see, makes perfect sense. Kind of overlooked that in the test case I guess. Thanks @fjgarlin!

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

quietone → closed merge request !5823

Publish CR

Automatically closed - issue fixed for 2 weeks with no activity.