Let's say you set password_count_log2 to 1-6 or above 14, i will use 4 as an example:

  1. When creating a password _password_generate_salt gets called with $count_log2 of 4.
  2. The function bumps this up to DRUPAL_MIN_HASH_COUNT (or down to DRUPAL_MAX_HASH_COUNT ) before writing an encoded version into password string.
  3. Now, let's try to log in... user_authenticate calls user_needs_new_hash
  4. which checks _password_get_count_log2($account->pass) (in our example 7 because of the bump) and variable_get('password_count_log2', DRUPAL_HASH_COUNT) (in our example 4). It won't match so user_needs_new_hash returns a true. On every login. No exceptions.
  5. 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
CommentFileSizeAuthor
#10 991270-10-password-log2-boundaries.patch8.83 KBcarlos8f
#8 991270-8-password-log2-boundaries.patch5.16 KBcarlos8f
#7 991270-7-password-log2-boundaries.patch1.93 KBcarlos8f
#1 password_enforce_boundaries.patch1.55 KBchx

Comments

chx’s picture

chx commented
Status: Active » Needs review
StatusFileSize
new password_enforce_boundaries.patch1.55 KB

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ includes/password.inc	2010-12-07 11:50:22 +0000
@@ -99,17 +99,21 @@ function _password_base64_encode($input,
+function _password_enforce_boundaries($count_log2) {
+  // Minimum log2 iterations is DRUPAL_MIN_HASH_COUNT.
+  $count_log2 = max($count_log2, DRUPAL_MIN_HASH_COUNT);
+  // Maximum log2 iterations is DRUPAL_MIN_HASH_COUNT.
+  return min($count_log2, DRUPAL_MAX_HASH_COUNT);

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

chx’s picture

chx commented

The intention is that indeed and the max / min is quite correct.

carlos8f’s picture

carlos8f commented

subscribing

chx’s picture

chx commented

on 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?

carlos8f’s picture

carlos8f commented

yeah, I like < and > better. I'll see if I can patch that up.

carlos8f’s picture

carlos8f commented
StatusFileSize
new 991270-7-password-log2-boundaries.patch1.93 KB

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

carlos8f’s picture

carlos8f commented
StatusFileSize
new 991270-8-password-log2-boundaries.patch5.16 KB

Added some tests. Note that this patch adds a new file, modules/simpletest/tests/password.test.

chx’s picture

chx commented
Status: Needs review » Needs work

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

carlos8f’s picture

carlos8f commented
Status: Needs work » Needs review
StatusFileSize
new 991270-10-password-log2-boundaries.patch8.83 KB

Added utility function, user_load()/save(), and better tests.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Very nicely done!

carlos8f’s picture

carlos8f commented

Note to committers: don't forget to cvs add modules/simpletest/tests/password.test !

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

It'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.

carlos8f’s picture

carlos8f commented

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

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC I guess.

chx’s picture

chx commented

Poor patch was ready and now stalled for a week over minute misunderstanding :(

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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