Posted by Dave Reid on June 30, 2011 at 2:00pm
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | user system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
Comments
#1
Patch for D7 attached, it may apply cleanly to D8, let's see.
#2
The last submitted patch, 1205138-user-update-7000-non-md5.patch, failed testing.
#3
Right, this will only apply to D7 as user_update_7000() has been removed in D8.
#4
This 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.
#5
Yeah that was one of my ideas to restrict to match only MD5 hashes.
#6
simple string inspection is much faster than preg match.
#7
re #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).
#8
Not sure why Dave Reid's tests cause explosions. Here's a version without the test.
#9
#5: 1205138-user-update-7000-non-md5.patch queued for re-testing.
#10
The 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.
#11
The 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?
#12
Also in #4 I asked that we check explicitly for something that looks like a md5. There can be *anything* in there.
#13
This 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.#15
Ok, well if we don't mind using preg_match(), I think this is the most specific test we can make.
#16
[updated the summary]
#17
Ok 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.
#18
Hmm, 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.
#19
The last submitted patch, 1205138-check-before-rehash-18.patch, failed testing.
#20
oops - left a git config in place to strip the patch prefix. Kind of silly the test bot totally rejects -p0 patches now.
#21
Subscribe
#22
Took some time to understand this patch, but now it makes 100% sense. Ready to fly, IMO.
#23
+++ b/modules/simpletest/tests/upgrade/upgrade.user.test@@ -26,6 +26,9 @@ class UserUpgradePathPasswordTokenTestCase extends UpgradePathTestCase {
+ // Check that a non-md5 has was untouched.
Minor typo, should be "Check that non-md5 hash was untouched."
13 days to next Drupal core point release.
#24
@skottler, not sure I agree - I think the existing comment is grammatically correct.
#25
oh, oops - it's the missing "h"
#26
fixed typo.
#27
Committed and pushed to 7.x. Thanks!
#28
Automatically closed -- issue fixed for 2 weeks with no activity.