Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Let's say you set password_count_log2
to 1-6 or above 14, i will use 4 as an example:
- When creating a password
_password_generate_salt
gets called with$count_log2
of 4. - The function bumps this up to
DRUPAL_MIN_HASH_COUNT
(or down toDRUPAL_MAX_HASH_COUNT
) before writing an encoded version into password string. - Now, let's try to log in...
user_authenticate
callsuser_needs_new_hash
- which checks
_password_get_count_log2($account->pass)
(in our example 7 because of the bump) andvariable_get('password_count_log2', DRUPAL_HASH_COUNT)
(in our example 4). It won't match souser_needs_new_hash
returns a true. On every login. No exceptions. - But that performance penalty is not the end! Because user_authenticate runs a direct UPDATE against user table if pass needs update. user_save? what user_save? who needs hooks? :p
Comment | File | Size | Author |
---|---|---|---|
#10 | 991270-10-password-log2-boundaries.patch | 8.83 KB | carlos8f |
#8 | 991270-8-password-log2-boundaries.patch | 5.16 KB | carlos8f |
#7 | 991270-7-password-log2-boundaries.patch | 1.93 KB | carlos8f |
#1 | password_enforce_boundaries.patch | 1.55 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedA simple little patch that helps a little. A proper patch would patch the hell out of user_authenticate to use user_save but it's 4AM. Tomorrow.
Comment #2
sun1) Second comment should use MAX_.
2) Missing phpDoc. In that, it would be nice to explain why we are using max() to figure out the minimum value, and min() to figure out the maximum value (i.e., the opposite function in both cases). It looks like the intention is to ensure that $count_log2 is $count_log2, unless it's outside of the valid MIN/MAX constant range?
Powered by Dreditor.
Comment #3
chx CreditAttribution: chx commentedThe intention is that indeed and the max / min is quite correct.
Comment #4
carlos8f CreditAttribution: carlos8f commentedsubscribing
Comment #5
chx CreditAttribution: chx commentedon another note, we probably could change the min-max duo to an if ( < ) elseif ( > ) else structure which would be a hell more readable and while a tiny bit little slower who cares when there is so much crypto going on?
Comment #6
carlos8f CreditAttribution: carlos8f commentedyeah, I like < and > better. I'll see if I can patch that up.
Comment #7
carlos8f CreditAttribution: carlos8f commentedOperators replacing min()/max() calls. I would feel better if we had unit tests for password.inc; I will try to write at least something that covers this bug.
Comment #8
carlos8f CreditAttribution: carlos8f commentedAdded some tests. Note that this patch adds a new file,
modules/simpletest/tests/password.test
.Comment #9
chx CreditAttribution: chx commentedTo fully solve the issue, user_authorize needs to convert to user_load and user_save. Also, please add a utility function as I did because you are duplicating code.
Comment #10
carlos8f CreditAttribution: carlos8f commentedAdded utility function, user_load()/save(), and better tests.
Comment #11
chx CreditAttribution: chx commentedThis looks good. Consider this: if a password implementation happens to depend on fields then the pseudo user object it gets wont do any good. So this is quite a bug.
Comment #12
sunVery nicely done!
Comment #13
carlos8f CreditAttribution: carlos8f commentedNote to committers: don't forget to
cvs add modules/simpletest/tests/password.test
!Comment #14
Dries CreditAttribution: Dries commentedIt's not clear why some tests need to live in password.test and others in user.test when they manipulate the same variables. Let's clean up the tests a bit more.
Comment #15
carlos8f CreditAttribution: carlos8f commentedpassword.test is intended as a unit test of password.inc, similar to graph.test for graph.inc. The only reason it's not a DrupalUnitTestCase is that it needs to do variable_set('password_count_log2'). Whereas the hunk for user.test tests the user module's usage of the hashing API. I think those are two separate things.
Comment #16
chx CreditAttribution: chx commentedBack to RTBC I guess.
Comment #17
chx CreditAttribution: chx commentedPoor patch was ready and now stalled for a week over minute misunderstanding :(
Comment #18
Dries CreditAttribution: Dries commentedThanks for the explanation. I looked into it some more and it makes sense now. I was confused because the functions in password.inc start with user_*. Now that I re-discovered that these user_* functions are defined in password.inc, this patch makes sense. Committed to CVS HEAD.