Download & Extend

phpass breaks user_authenticate() and user_load()

Project:Secure Password Hashes
Version:5.x-2.x-dev
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:patch (to be ported)

Issue Summary

With phpass installed, calls to user_authenticate() fail. This is because user_authenticate() calls user_load() with an array of user information that includes a plaintext password, and user_load() has a hardcoded md5() call to resolve the password. pwolanin suggested a core patch to be shipped with phpass, which is the only solution I can think of as well.

Comments

#1

Status:active» needs review

Not sure if this is the right idea or not. It looks like once phpass is installed it's there for good, so I didn't add a module_exists check. I also didn't write it in such a way as to be acceptable in the core queue - getting it into core doesn't seem like an achievable goal. But maybe I'm a pessimist.

AttachmentSize
1370988.user_load.D6.patch 1.07 KB

#2

How did this not come up before?

#3

It's not a facility used by core, only by external authentication modules using the password. I was aware it could be an issue, but didn't seem critical.

Patch looks reasonable to me - I think if you apply this patch you are not going to disable phpass. The only value I could see to a module_exists check would be that the patch could be applied prior to phpass being installed.

#4

Priority:normal» critical

Ok, well discovered now that user_authenticate() *is* used by core when you have email verification disabled for user registration.

I think in addition to supplying this patch with the module, we need to make the module work around that problem absent the patch.

#5

Here's a patch for the module itself that works around the authentication failure.

The core registration code flow is very wonky, but I think the logic here is both correct and minimal.

AttachmentSize
1370988-5.patch 1.4 KB

#6

oops missed-used an undefined $form_values that should be $form_state['values']

AttachmentSize
1370988-6.patch 1.41 KB

#7

we could perhaps add a define() to the core patch? That way we can 100% skip the extra submit function if the core patch is applied.

AttachmentSize
1370988-7-user_load.D6.patch 1.24 KB

#8

with code comment

AttachmentSize
1370988-8-user_load.D6.patch 1.3 KB

#9

corresponding module patch

AttachmentSize
1370988-9.patch 1.45 KB

#10

full patch

AttachmentSize
1370988-10.patch 3.5 KB

#11

Version:6.x-2.x-dev» 5.x-2.x-dev
Status:needs review» patch (to be ported)

committed to 6.x

nobody click here