Backport of #3277003: Harden user_pass_rehash() against attack

Issue fork drupal-3409043

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mcdruid created an issue. See original summary.

mcdruid opened merge request !5825

mcdruid’s picture

mcdruid commented
Status: Active » Needs review

Added a similar test to the one we're working on in the parent issue.

It could perhaps be a unit test, but not sure how reliable that would be as user_pass_rehash() calls drupal_get_hash_salt() which may use the $databases global etc..

mcdruid’s picture

mcdruid commented
Issue tags: +Needs change record

The test-only job failed as expected:

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

Reset password 284 passes, 1 fail, 0 exceptions, and 88 debug messages
Test run duration: 22 sec
Detailed test results
---------------------
---- UserPasswordResetTestCase ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      user.test          960 UserPasswordResetTestCase->testUniq
    No user_pass_rehash() hash collision for different users with no stored
    password.

Same as the parent issue, this will need a CR - any "in flight" reset links will become invalid when the code changes, but they are short-lived by design.

Anything else?

mcdruid’s picture

mcdruid commented
Issue tags: -Needs change record

Added a draft CR based on the one @poker10 made for the parent issue:

https://www.drupal.org/node/3409960

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Based on the discussion in the parent, and the fact the tests are green here setting it to RTBC. Also checked the "tests only" run in GitLab which gave the expected result.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Title: [D7 backport] Harden user_pass_rehash() against attack » [D7] Harden user_pass_rehash() against attack
mcdruid’s picture

mcdruid commented

The parent has been committed to D10/11 - I think we're okay to commit this to D7 now, but I don't want to commit my own patch/MR.

One tweak I might suggest is to use the word "minimal" instead of "lightweight":

    // Lightweight user objects are sufficient.
    $user = drupal_anonymous_user();

We don't really need to use objects at all as the properties are passed as individual params, but I think I'm happy with the rest of the test.

@poker10 are you happy to commit this (perhaps changing the comment as above)?

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

One tweak I might suggest is to use the word "minimal" instead of "lightweight"

Minimal does sound more correct to me. Or maybe even better to just say "Anonymous user objects are sufficient." or drop the comment as it doesn't add that much value either.

mcdruid’s picture

mcdruid commented

I think my motivation for adding the comment was to avoid anyone (including myself later on) thinking that we needed to add more code / dependencies to the test in order to provide more complete user objects.. per the vintage comment in the API docs which confirms that drupal_anonymous_user() doesn't return a full user object, or invoke hooks etc.. etc..

https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...

I'd be happy with a tweak to the wording, or for the comment to be removed on commit if we think it's not worth keeping.

  • poker10 committed 03b2efb8 on 7.x 
    Issue #3409043 by mcdruid: [D7] Harden user_pass_rehash() against attack
poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Fixed

I think the "minimal" word should be OK, so I changed that on commit. I consider that comment helpful (exactly for reasons mentioned in #10), so not removing it. Other than that I think this looks good and code is in line with the D10 commit (also the CRs are practically the same).

Committed this, thanks all!

Status: Fixed » Closed (fixed)

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