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.

Files: 
CommentFileSizeAuthor
#26 1205138-check-before-rehash-25.patch3.16 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 35,970 pass(es).
[ View ]
#20 1205138-check-before-rehash-20.patch3.16 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 36,010 pass(es).
[ View ]
#18 1205138-check-before-rehash-18.patch3.14 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1205138-check-before-rehash-18.patch. See the log in the details link for more information.
[ View ]
#15 1205138-check-before-rehash-15.patch754 bytespwolanin
PASSED: [[SimpleTest]]: [MySQL] 35,635 pass(es).
[ View ]
#8 1205138-check-before-rehash-8.patch743 bytespwolanin
PASSED: [[SimpleTest]]: [MySQL] 34,969 pass(es).
[ View ]
#6 1205138-check-before-rehash-2.patch874 bytespwolanin
PASSED: [[SimpleTest]]: [MySQL] 35,018 pass(es).
[ View ]
#5 1205138-user-update-7000-non-md5.patch5.07 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 35,652 pass(es), 0 fail(s), and 2 exception(es).
[ View ]
#3 1205138-user-update-7000-non-md5.patch5.02 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 35,019 pass(es), 0 fail(s), and 2 exception(es).
[ View ]
#1 1205138-user-update-7000-non-md5.patch5.02 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1205138-user-update-7000-non-md5.patch.
[ View ]

Comments

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
StatusFileSize
new5.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1205138-user-update-7000-non-md5.patch.
[ View ]

Patch for D7 attached, it may apply cleanly to D8, let's see.

Status:Needs review» Needs work

The last submitted patch, 1205138-user-update-7000-non-md5.patch, failed testing.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new5.02 KB
FAILED: [[SimpleTest]]: [MySQL] 35,019 pass(es), 0 fail(s), and 2 exception(es).
[ View ]

Right, this will only apply to D7 as user_update_7000() has been removed in D8.

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.

StatusFileSize
new5.07 KB
FAILED: [[SimpleTest]]: [MySQL] 35,652 pass(es), 0 fail(s), and 2 exception(es).
[ View ]

Yeah that was one of my ideas to restrict to match only MD5 hashes.

StatusFileSize
new874 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,018 pass(es).
[ View ]

simple string inspection is much faster than preg match.

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).

StatusFileSize
new743 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,969 pass(es).
[ View ]

Not sure why Dave Reid's tests cause explosions. Here's a version without the test.

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work

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?

Also in #4 I asked that we check explicitly for something that looks like a md5. There can be *anything* in there.

simple string inspection is much faster than preg match.

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.

Status:Needs work» Needs review
StatusFileSize
new754 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,635 pass(es).
[ View ]

Ok, well if we don't mind using preg_match(), I think this is the most specific test we can make.

Status:Needs review» Reviewed & tested by the community

[updated the summary]

Issue summary:View changes

Update the summary.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1205138-check-before-rehash-18.patch. See the log in the details link for more information.
[ View ]

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1205138-check-before-rehash-18.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 36,010 pass(es).
[ View ]

oops - left a git config in place to strip the patch prefix. Kind of silly the test bot totally rejects -p0 patches now.

Subscribe

Issue tags:-Needs tests

Took some time to understand this patch, but now it makes 100% sense. Ready to fly, IMO.

Status:Reviewed & tested by the community» Needs work

+++ 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.

Status:Needs work» Reviewed & tested by the community

@skottler, not sure I agree - I think the existing comment is grammatically correct.

Status:Reviewed & tested by the community» Needs work

oh, oops - it's the missing "h"

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 35,970 pass(es).
[ View ]

fixed typo.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

include phpass as backport