Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Drupal 7 introduced a better and more secure way of hashing passwords. This method has been backported to Drupal 6 in various modules, including the Secure Password Hashes and Password modules. When upgrading from Drupal 6 to Drupal 7, core assumes that passwords are always stored in the old format, and people are unable to login anymore.
Proposed resolution
Core should just skip passwords that are not stored in the expected format: a hexadecimal md5 hash.
Remaining tasks
None. Note that this patch applies only to Drupal 7, not Drupal 8.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#26 | 1205138-check-before-rehash-25.patch | 3.16 KB | pwolanin |
#20 | 1205138-check-before-rehash-20.patch | 3.16 KB | pwolanin |
#18 | 1205138-check-before-rehash-18.patch | 3.14 KB | pwolanin |
#15 | 1205138-check-before-rehash-15.patch | 754 bytes | pwolanin |
#8 | 1205138-check-before-rehash-8.patch | 743 bytes | pwolanin |
Comments
Comment #1
Dave ReidPatch for D7 attached, it may apply cleanly to D8, let's see.
Comment #3
Dave ReidRight, this will only apply to D7 as user_update_7000() has been removed in D8.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis sounds like a really good idea. I suggest we restrict the conversion to hashes matching [0-9a-f]{15}. There could be anything in there.
Comment #5
Dave ReidYeah that was one of my ideas to restrict to match only MD5 hashes.
Comment #6
pwolanin CreditAttribution: pwolanin commentedsimple string inspection is much faster than preg match.
Comment #7
pwolanin CreditAttribution: pwolanin commentedre #6 - Dave Reid's last patch cross-posted with mine and uses the same approach (plus he has tests).
Note that using just the string values instead of preg_match() was > 5.x faster in a simple test locally (testing 4 different strings 100000 times each).
Comment #8
pwolanin CreditAttribution: pwolanin commentedNot sure why Dave Reid's tests cause explosions. Here's a version without the test.
Comment #9
cwgordon7 CreditAttribution: cwgordon7 commented#5: 1205138-user-update-7000-non-md5.patch queued for re-testing.
Comment #10
cwgordon7 CreditAttribution: cwgordon7 commentedThe patch in #8 looks good, it's very simple and it works. It worries me that the test in #5 fails, but it seems like that might be unrelated to the patch and just a problem in the test or the testing framework, since it works.
Comment #11
webchickThe code comment doesn't seem to match what the code is doing there... we're not checking for md5(), we're checking for some weird $ mumbo jumbo.
This mumbo jumbo is *also* not explained, neither in this issue nor in this patch. What are we doing this for? Why are we checking that stuff? Is just checking for $ enough?
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso in #4 I asked that we check explicitly for something that looks like a md5. There can be *anything* in there.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is a purely useless optimization. The
preg_match()
here is already *way slower* then the db_query(), and all those are orders of magnitude slower then the hashing/stretching we do after that. There is absolutely no point in optimizing here.Comment #15
pwolanin CreditAttribution: pwolanin commentedOk, well if we don't mind using preg_match(), I think this is the most specific test we can make.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commented[updated the summary]
Comment #16.0
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdate the summary.
Comment #17
webchickOk great, that looks fine.
Talked with chx and he agreed we should add tests for this. Just a quickie 2-liner that adds a PHPass hash row to the D6 users table and an assertion that it's retained should be enough, I think.
Comment #18
pwolanin CreditAttribution: pwolanin commentedHmm, well a minor conumdrum is that we cannot actually add a D7 hash, since the D6 {users} table field is not long enough. Here's patch plus added assertion with a truncated hash.
Verified that assertion failed w/out patch and passes with patch from #15.
Comment #20
pwolanin CreditAttribution: pwolanin commentedoops - left a git config in place to strip the patch prefix. Kind of silly the test bot totally rejects -p0 patches now.
Comment #21
skottler CreditAttribution: skottler commentedSubscribe
Comment #22
sunTook some time to understand this patch, but now it makes 100% sense. Ready to fly, IMO.
Comment #23
skottler CreditAttribution: skottler commentedMinor typo, should be "Check that non-md5 hash was untouched."
13 days to next Drupal core point release.
Comment #24
pwolanin CreditAttribution: pwolanin commented@skottler, not sure I agree - I think the existing comment is grammatically correct.
Comment #25
pwolanin CreditAttribution: pwolanin commentedoh, oops - it's the missing "h"
Comment #26
pwolanin CreditAttribution: pwolanin commentedfixed typo.
Comment #27
webchickCommitted and pushed to 7.x. Thanks!
Comment #28.0
(not verified) CreditAttribution: commentedinclude phpass as backport