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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
FileSize
5.02 KB

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.

Dave Reid’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
5.02 KB

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

Damien Tournoud’s picture

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.

Dave Reid’s picture

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

pwolanin’s picture

simple string inspection is much faster than preg match.

pwolanin’s picture

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

pwolanin’s picture

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

cwgordon7’s picture

cwgordon7’s picture

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.

webchick’s picture

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?

Damien Tournoud’s picture

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

Damien Tournoud’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
754 bytes

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

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

[updated the summary]

Damien Tournoud’s picture

Issue summary: View changes

Update the summary.

webchick’s picture

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.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.14 KB

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.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.16 KB

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

skottler’s picture

Subscribe

sun’s picture

Issue tags: -Needs tests

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

skottler’s picture

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.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

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

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

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

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.16 KB

fixed typo.

webchick’s picture

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.

Anonymous’s picture

Issue summary: View changes

include phpass as backport