Moodle
  1. Moodle
  2. MDL-35332

Improve security of hashed passwords

    Details

    • Testing Instructions:
      Hide

      While testing you might want to run a query on the database to look at the format of the password field:

      > SELECT id, username, password FROM mdl_user;

      id username password
      3 testuser b0c1805f9b6435abbf6c2d3fe81ecb36

      if the password is of the form:

      b0c1805f9b6435abbf6c2d3fe81ecb36

      then it's the old-style md5 hash. If it looks more like:

      $2y$10$DzO4FuCYRFnx1wyiYiOI8udJA8yrALCYFnMyFOfX.ZbNLo6.VtV/y

      then it is in the new (bcrypt) format

      Test functionality with PHP version that supports bcrypt (typically PHP 5.3.7 or above)

      Hashes updated on first login following upgrade

      1. Install a site without the patch applied
      2. Create some users with known passwords
      3. Upgrade site to include patch
      4. Login as some of the users (they should still be able to login)
      5. Log out and log back in again (they should still be able to login)
      6. Try to login as a user with the wrong password (should not be able to login)

      DB checks:
      After step 2 - old style hash
      After step 3 - same as step 2
      After step 4 - new style hash
      After step 5 - same as step 4

      e.g. the hash changes once when the user first successfully logs in

      New users created by admin

      1. Install a new site with the patch installed
      2. Create some users with known passwords
      3. Login as some of the users (they should be able to login)

      DB checks: password should always be the new format

      Self registration

      1. Create a site with patch applied
      2. As admin enable self-registration via Site Admin > Plugins > Authentication > Manage Authentication > Self registration
      3. Logout
      4. Create a user account (note: mail must be working on your site to test this)
      5. Test you can login with the password you provided

      DB checks: password should always be the new format

      Password change

      1. Create a site with patch applied
      2. Login as any user
      3. Click My Profile Settings > Change password and enter a new password
      4. Logout
      5. Check that you can login again with the new password

      DB checks: password should always be the new format

      Note: You will get a PHP Notice when changing passwords due to MDL-37515.

      Bulk upload with passwords specified

      1. Create a text file containing the following:
        firstname,lastname,username,email,password
        Test,User,testuser,test@example.com,abc123
      2. As an admin visit Site Admin > Users > Accounts > Upload Users
      3. Choose the file you created in step 1 and click upload users
      4. Under 'New user password' choose 'Field required in file'
      5. Click upload users
      6. Test user should be created
      7. Logout
      8. Login with testuser and password abc123
      9. You should be able to login
      10. Logout and log back in again

      DB checks:
      After step 5 - new hash starting '$2y$04$'
      After step 8 - new style hash starting '$2y$10$'
      After step 10 - same as step 8

      Bulk upload with passwords not specified

      1. Create a text file containing the following:
        firstname,lastname,username,email
        Test,User2,testuser2,test2@example.com
      2. As an admin visit Site Admin > Users > Accounts > Upload Users
      3. Choose the file you created in step 1 and click upload users
      4. Under 'New user password' choose 'Create password if needed'
      5. Click upload users
      6. Test user should be created
      7. Run the cron
      8. Check email, you should have received an email with a temporary password (note: mail must be working on your site to test this)
      9. Logout
      10. Login with testuser and password from the email
      11. You should be able to login. You will be prompted to change the password
      12. Change the password
      13. Logout and log back in again with the new password
      14. You should be able to log in

      DB checks:
      After step 5 - 'to be generated'
      After step 7 - new style hash starting '$2y$04$'
      After step 9 - new style hash starting '$2y$10$'
      After step 11 - same as step 9

      Test forgotten password functionality

      1. Create a new site with the patch applied
      2. Visit the login page (not logged in)
      3. Click "Forgotten your username or password?"
      4. Enter an existing username or password that you have access to the email of
      5. Click the confirmation link in your email
      6. Check your email again to get the new password
      7. Check you can login with the new password

      DB checks:
      After step 1 - new style hash
      After step 5 - new style hash, but different to before
      After step 7 - same as step 5

      Account access via non-caching auth plugin

      1. Create a new site with the patch applied
      2. Add the attached test auth plugin to auth/test/
      3. Visit the notification page to install
      4. Go to Site admin > Plugins > Authentication > Manage Authentication Plugins
      5. Enable the plugin
      6. Create a new user, assigning them the 'test auth' authentication type
      7. Logout
      8. Login as the test user (any password will work)
      9. The password should not be stored in the user table
      10. Logout and log back in as admin
      11. Change the users auth method to manual and set a password
      12. Logout and login as that user checking the password works
      13. Log back in as admin and switch back to the test auth plugin
      14. Log back in as the user with the same password
      15. Logout and back in as the user with a different password

      DB checks:
      After step 6 - 'not cached'
      After step 8 - same as step 6
      After step 11 - new style hash
      After step 14 - same as step 11
      After step 15 - 'not cached'

      Test functionality with PHP version that doesn't support bcrypt (typically 5.3.6 or below)

      Repeat the tests above making sure everything works - should continue
      to function as before but using only the old algorithm (md5)

      Note: Some distributions have backported bcrypt support to releases prior to 5.3.7.

      Show
      While testing you might want to run a query on the database to look at the format of the password field: > SELECT id, username, password FROM mdl_user; id username password 3 testuser b0c1805f9b6435abbf6c2d3fe81ecb36 if the password is of the form: b0c1805f9b6435abbf6c2d3fe81ecb36 then it's the old-style md5 hash. If it looks more like: $2y$10$DzO4FuCYRFnx1wyiYiOI8udJA8yrALCYFnMyFOfX.ZbNLo6.VtV/y then it is in the new (bcrypt) format Test functionality with PHP version that supports bcrypt (typically PHP 5.3.7 or above) Hashes updated on first login following upgrade Install a site without the patch applied Create some users with known passwords Upgrade site to include patch Login as some of the users (they should still be able to login) Log out and log back in again (they should still be able to login) Try to login as a user with the wrong password (should not be able to login) DB checks: After step 2 - old style hash After step 3 - same as step 2 After step 4 - new style hash After step 5 - same as step 4 e.g. the hash changes once when the user first successfully logs in New users created by admin Install a new site with the patch installed Create some users with known passwords Login as some of the users (they should be able to login) DB checks: password should always be the new format Self registration Create a site with patch applied As admin enable self-registration via Site Admin > Plugins > Authentication > Manage Authentication > Self registration Logout Create a user account (note: mail must be working on your site to test this) Test you can login with the password you provided DB checks: password should always be the new format Password change Create a site with patch applied Login as any user Click My Profile Settings > Change password and enter a new password Logout Check that you can login again with the new password DB checks: password should always be the new format Note: You will get a PHP Notice when changing passwords due to MDL-37515 . Bulk upload with passwords specified Create a text file containing the following: firstname,lastname,username,email,password Test,User,testuser,test@example.com,abc123 As an admin visit Site Admin > Users > Accounts > Upload Users Choose the file you created in step 1 and click upload users Under 'New user password' choose 'Field required in file' Click upload users Test user should be created Logout Login with testuser and password abc123 You should be able to login Logout and log back in again DB checks: After step 5 - new hash starting '$2y$04$' After step 8 - new style hash starting '$2y$10$' After step 10 - same as step 8 Bulk upload with passwords not specified Create a text file containing the following: firstname,lastname,username,email Test,User2,testuser2,test2@example.com As an admin visit Site Admin > Users > Accounts > Upload Users Choose the file you created in step 1 and click upload users Under 'New user password' choose 'Create password if needed' Click upload users Test user should be created Run the cron Check email, you should have received an email with a temporary password (note: mail must be working on your site to test this) Logout Login with testuser and password from the email You should be able to login. You will be prompted to change the password Change the password Logout and log back in again with the new password You should be able to log in DB checks: After step 5 - 'to be generated' After step 7 - new style hash starting '$2y$04$' After step 9 - new style hash starting '$2y$10$' After step 11 - same as step 9 Test forgotten password functionality Create a new site with the patch applied Visit the login page (not logged in) Click "Forgotten your username or password?" Enter an existing username or password that you have access to the email of Click the confirmation link in your email Check your email again to get the new password Check you can login with the new password DB checks: After step 1 - new style hash After step 5 - new style hash, but different to before After step 7 - same as step 5 Account access via non-caching auth plugin Create a new site with the patch applied Add the attached test auth plugin to auth/test/ Visit the notification page to install Go to Site admin > Plugins > Authentication > Manage Authentication Plugins Enable the plugin Create a new user, assigning them the 'test auth' authentication type Logout Login as the test user (any password will work) The password should not be stored in the user table Logout and log back in as admin Change the users auth method to manual and set a password Logout and login as that user checking the password works Log back in as admin and switch back to the test auth plugin Log back in as the user with the same password Logout and back in as the user with a different password DB checks: After step 6 - 'not cached' After step 8 - same as step 6 After step 11 - new style hash After step 14 - same as step 11 After step 15 - 'not cached' Test functionality with PHP version that doesn't support bcrypt (typically 5.3.6 or below) Repeat the tests above making sure everything works - should continue to function as before but using only the old algorithm (md5) Note: Some distributions have backported bcrypt support to releases prior to 5.3.7.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      master_MDL-35332
    • Rank:
      44004

      Description

      Currently moodle uses md5 for password hashing and a site-wide salt when hashing passwords. It's generally considered best practice to use a modern hashing algorithm like Bcrypt for password hashing, and to have per-user salts - e.g:

      https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet#Rule_1:_Use_a_Modern_Hash_Algorithm
      https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet#Rule_2:_Use_a_Long_Cryptographically_Random_Per-User_Salt

      Bcrypt is now available natively in PHP5.3 via the crypt() function. I thought I'd create this bug to gauge interest and perhaps kick off some work to implement this.

      Obviously this would need to be implemented in a backward compatible way. I would suggest something like:

      1. Add 'password_hash' and 'salt' columns to user table, initially set to null
      2. Add code to authenticate using bcrypt or similar (ideally using an existing library)
      3. Update code for creating users to use new method
      4. When a user logs in check if 'password_hash' is null
      4a. If null authenticate using md5 and password column then calculate new hash with salt and store in db
      4b. If not null authenticate using bcrypt and new data

      This would gradually migrate existing sites to the new algorithm (as users login) while allowing new sites to use bcrypt exclusively. For sites that want to migrate fully, the administrator could reset all 'old' passwords.

      There are bound to be some edge cases that will need to be considered, such as 'changeme' passwords, other authentication plugins, etc. I would appreciate any thoughts from anyone more familiar with moodle authentication.

      Simon

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Thanks for the proposal.

          Show
          Petr Škoda added a comment - Thanks for the proposal.
          Hide
          Dan Poltawski added a comment -

          +1 for the idea in general. Here is an article I read a while ago about this issue: http://codahale.com/how-to-safely-store-a-password/

          Here is a link to the Mahara change which did the same: https://bugs.launchpad.net/mahara/+bug/843568
          Note that when doing bulk csv operations they use sha1 to avoid the problems spending an excessive amount of time generating the hash.

          Show
          Dan Poltawski added a comment - +1 for the idea in general. Here is an article I read a while ago about this issue: http://codahale.com/how-to-safely-store-a-password/ Here is a link to the Mahara change which did the same: https://bugs.launchpad.net/mahara/+bug/843568 Note that when doing bulk csv operations they use sha1 to avoid the problems spending an excessive amount of time generating the hash.
          Hide
          Dan Poltawski added a comment -

          This is interesting, particularly the php-implementation for backwards compatibility: https://gist.github.com/3707231

          https://wiki.php.net/rfc/password_hash

          Show
          Dan Poltawski added a comment - This is interesting, particularly the php-implementation for backwards compatibility: https://gist.github.com/3707231 https://wiki.php.net/rfc/password_hash
          Hide
          Simon Coggins added a comment -

          That looks like a much cleaner implementation than others I've seen - I like how the algorithm, options and hash are all stored together and password_needs_rehash() is quite neat too.

          Thanks for the link to the Mahara solution. Since we would need some code to migrate 'old' md5 hashes to the new algorithm anyway, one option would be to continue to use md5 for bulk imports then switch them to the new algorithm the first time the user logs in. Or are the benefits of sha1 over md5 sufficient to make it worth managing the extra code?

          Show
          Simon Coggins added a comment - That looks like a much cleaner implementation than others I've seen - I like how the algorithm, options and hash are all stored together and password_needs_rehash() is quite neat too. Thanks for the link to the Mahara solution. Since we would need some code to migrate 'old' md5 hashes to the new algorithm anyway, one option would be to continue to use md5 for bulk imports then switch them to the new algorithm the first time the user logs in. Or are the benefits of sha1 over md5 sufficient to make it worth managing the extra code?
          Hide
          Simon Coggins added a comment -

          I've had a go at implementing this - it could do with a good review and testing especially by someone with a good knowledge of how authentication plugins and the core moodle functions interact. I've tried to find and test as many potential issues as I can but there's quite a few so it's possible I might have missed something.

          I decided not to implement SHA1 hashes too as it would have added to the complexity and since SHA1s can be calculated nearly as quickly as MD5s it doesn't improve security that much.

          There is one unresolved issue - the PHP library requires PHP 5.3.7 due to a security issue with bcrypt in earlier versions (see http://stackoverflow.com/a/12461336/73522) whereas Moodle supports back to 5.3.2.

          My solution is to check the PHP version number and fallback to using md5 for PHP versions before 5.3.7. This works as long as a site isn't migrated from PHP 5.3.7+ back to PHP 5.3.2 - 5.3.6. Is that a use case that needs to be supported?

          Show
          Simon Coggins added a comment - I've had a go at implementing this - it could do with a good review and testing especially by someone with a good knowledge of how authentication plugins and the core moodle functions interact. I've tried to find and test as many potential issues as I can but there's quite a few so it's possible I might have missed something. I decided not to implement SHA1 hashes too as it would have added to the complexity and since SHA1s can be calculated nearly as quickly as MD5s it doesn't improve security that much. There is one unresolved issue - the PHP library requires PHP 5.3.7 due to a security issue with bcrypt in earlier versions (see http://stackoverflow.com/a/12461336/73522 ) whereas Moodle supports back to 5.3.2. My solution is to check the PHP version number and fallback to using md5 for PHP versions before 5.3.7. This works as long as a site isn't migrated from PHP 5.3.7+ back to PHP 5.3.2 - 5.3.6. Is that a use case that needs to be supported?
          Hide
          Dan Marsden added a comment -

          adding Skodak here - any chance you could take a look at Simon's proposal here? - I'd guess we have missed getting this into 2.4

          Show
          Dan Marsden added a comment - adding Skodak here - any chance you could take a look at Simon's proposal here? - I'd guess we have missed getting this into 2.4
          Hide
          Dan Poltawski added a comment - - edited

          Forum thread about this issue: https://moodle.org/mod/forum/discuss.php?d=213871

          [removing security flag so more people can see it]

          Show
          Dan Poltawski added a comment - - edited Forum thread about this issue: https://moodle.org/mod/forum/discuss.php?d=213871 [removing security flag so more people can see it]
          Hide
          Petr Škoda added a comment -

          my +1 for new salt column in the user table and better hashing mechanisms, some time ago I was looking at http://en.wikipedia.org/wiki/PBKDF2

          I suppose this should be delayed until 2.5 because of the risk of regressions and changes in API/DB that would be required for per user salts.

          Show
          Petr Škoda added a comment - my +1 for new salt column in the user table and better hashing mechanisms, some time ago I was looking at http://en.wikipedia.org/wiki/PBKDF2 I suppose this should be delayed until 2.5 because of the risk of regressions and changes in API/DB that would be required for per user salts.
          Hide
          Simon Coggins added a comment -

          A separate salt column isn't required when using the password_compat library as the algorithm, salt and password are all stored together as a single string.

          Show
          Simon Coggins added a comment - A separate salt column isn't required when using the password_compat library as the algorithm, salt and password are all stored together as a single string.
          Hide
          Petr Škoda added a comment -

          Aah, I finally read the patch carefully and I think it is very good and with a few changes it might be integrated soon. Let's discuss the details next week. Thanks!

          Show
          Petr Škoda added a comment - Aah, I finally read the patch carefully and I think it is very good and with a few changes it might be integrated soon. Let's discuss the details next week. Thanks!
          Hide
          Simon Coggins added a comment -

          Following discussions at the hackfest:

          • We'd like to get this into master soon after 2.4 is out so that it has some time to get thoroughly tested before 2.5.
          • Moodle is increasing the version requirements for 2.5 so the version issue will be less of a problem (or not a problem at all if it requires 5.3.8+). Since downgrading PHP version is likely to be uncommon and there are two workarounds (upgrade PHP or reset user passwords) we concluded that it is not a major issue.
          • I said I'd do some tests on speed of md5 vs bcrypt for the bulk user upload part (see below)

          I did a test upload of 10000 users, first hashing with md5 then with bcrypt. Results were:

          md5 1:18 (78s)
          bcrypt 11:46 (706s)

          So with bcrypt generating users is about 10 times slower. The trade off here is bulk upload speed vs. password security (but only between the time the bulk import is done and the first time the user logs in as the hash is updated after first login anyway).

          I think I'd prefer to leave it using md5, but it's trivial to change if required.

          Can anyone see anything else that needs looking at? Otherwise I'll push for peer review.

          Show
          Simon Coggins added a comment - Following discussions at the hackfest: We'd like to get this into master soon after 2.4 is out so that it has some time to get thoroughly tested before 2.5. Moodle is increasing the version requirements for 2.5 so the version issue will be less of a problem (or not a problem at all if it requires 5.3.8+). Since downgrading PHP version is likely to be uncommon and there are two workarounds (upgrade PHP or reset user passwords) we concluded that it is not a major issue. I said I'd do some tests on speed of md5 vs bcrypt for the bulk user upload part (see below) I did a test upload of 10000 users, first hashing with md5 then with bcrypt. Results were: md5 1:18 (78s) bcrypt 11:46 (706s) So with bcrypt generating users is about 10 times slower. The trade off here is bulk upload speed vs. password security (but only between the time the bulk import is done and the first time the user logs in as the hash is updated after first login anyway). I think I'd prefer to leave it using md5, but it's trivial to change if required. Can anyone see anything else that needs looking at? Otherwise I'll push for peer review.
          Hide
          Dan Poltawski added a comment -

          I agree bulk user upload should continue to use md5.

          From the hackfest we discussed the backwards compatibility of a site is using 5.3.8 then the password hashes will not be compatible with sites with lower php versions, but:

          • If an actual production site has upgrade to 5.3.8 then its very unlikely it'll later go backwards, and that is really the responsibility of the site admins. Briefly, downgrading php versions on a real site seems very edge case, and will be limitation.
          • Password hashes stored in moodle backup files will be affected by this, so a backup file taken in a Moodle php 5.4 site will not be compatible with an php version without bcypt support, but this too is very edge case as:
            • Only a limited set of users have the capability to backup userdata including hashes
            • You'd need the site salt anyway
            • Worse case is a password reset on imported user
          Show
          Dan Poltawski added a comment - I agree bulk user upload should continue to use md5. From the hackfest we discussed the backwards compatibility of a site is using 5.3.8 then the password hashes will not be compatible with sites with lower php versions, but: If an actual production site has upgrade to 5.3.8 then its very unlikely it'll later go backwards, and that is really the responsibility of the site admins. Briefly, downgrading php versions on a real site seems very edge case, and will be limitation. Password hashes stored in moodle backup files will be affected by this, so a backup file taken in a Moodle php 5.4 site will not be compatible with an php version without bcypt support, but this too is very edge case as: Only a limited set of users have the capability to backup userdata including hashes You'd need the site salt anyway Worse case is a password reset on imported user
          Hide
          Petr Škoda added a comment -

          Admins may also let Moodle auto-generate the passwords for users in cron because then they do not have to tell them the new passwords - they get them via email instead.

          Show
          Petr Škoda added a comment - Admins may also let Moodle auto-generate the passwords for users in cron because then they do not have to tell them the new passwords - they get them via email instead.
          Hide
          Hubert Chathi added a comment -

          As of PHP 5.3.0, PHP includes its own implementation of the crypt hash algorithms, in case the system doesn't provide an implementation, so compatibility shouldn't be an issue.
          How do the SHA-* algorithms compare with regards to speed? If they aren't too slow, IMHO it would be much better to use those than MD5.

          Show
          Hubert Chathi added a comment - As of PHP 5.3.0, PHP includes its own implementation of the crypt hash algorithms, in case the system doesn't provide an implementation, so compatibility shouldn't be an issue. How do the SHA-* algorithms compare with regards to speed? If they aren't too slow, IMHO it would be much better to use those than MD5.
          Hide
          Dan Poltawski added a comment -

          Hubert: Did you see Simons comment? "There is one unresolved issue - the PHP library requires PHP 5.3.7 due to a security issue with bcrypt in earlier versions (see http://stackoverflow.com/a/12461336/73522) whereas Moodle supports back to 5.3.2."

          Really do you think that its worth adding sha support when we are going to need to support backwards compatible md5 support anyway? Use of the 'quick hash' should be limited to auto-generated passwords when they will be stored plain somewhere anyway to distribute passwords..

          Show
          Dan Poltawski added a comment - Hubert: Did you see Simons comment? "There is one unresolved issue - the PHP library requires PHP 5.3.7 due to a security issue with bcrypt in earlier versions (see http://stackoverflow.com/a/12461336/73522 ) whereas Moodle supports back to 5.3.2." Really do you think that its worth adding sha support when we are going to need to support backwards compatible md5 support anyway? Use of the 'quick hash' should be limited to auto-generated passwords when they will be stored plain somewhere anyway to distribute passwords..
          Hide
          Hubert Chathi added a comment -

          Is there an advantage of bcrypt over SHA-512? If we use SHA-512 instead of bcrypt, then 5.3.2 would be fine, and we can tweak the number of rounds to run slower or faster. Even if you do want to use bcrypt, I think that SHA-* would be a much saner fallback for PHP <5.3.7 than MD5. As I've mentioned elsewhere, US-CERT recommends against using MD5 for any purposes since it should be considered cryptographically broken.
          For passwords that users will have to change the first time they log in, it doesn't really matter what hash is used. Simon mentioned using it for bulk uploaded users – do bulk uploaded users need to change their password the first time they log in?

          Show
          Hubert Chathi added a comment - Is there an advantage of bcrypt over SHA-512? If we use SHA-512 instead of bcrypt, then 5.3.2 would be fine, and we can tweak the number of rounds to run slower or faster. Even if you do want to use bcrypt, I think that SHA-* would be a much saner fallback for PHP <5.3.7 than MD5. As I've mentioned elsewhere, US-CERT recommends against using MD5 for any purposes since it should be considered cryptographically broken. For passwords that users will have to change the first time they log in, it doesn't really matter what hash is used. Simon mentioned using it for bulk uploaded users – do bulk uploaded users need to change their password the first time they log in?
          Hide
          Dan Poltawski added a comment -

          Hubert: I don't know about this in detail, but the point with bcrypt is specifically to be slow to compute, I don't think sha-512 has that goal in mind. Did you see my links above?

          Well, I hope that bulk-uploaded passwords require reset after login, but again - if we are auto-generating these passwords they will stored somewhere in plain text, so IMO any attempt to secure that hash is futile. I know what you are saying about md5, and thats a great reason for us to move to bcrypt or alternative by default, but its not a rationale to change the bulk actions, since we're already using it, so need to support it. (And, whilst its cryptographically broken, i'm not convinced this is a major risk in our context of bulk-user creation)

          Show
          Dan Poltawski added a comment - Hubert: I don't know about this in detail, but the point with bcrypt is specifically to be slow to compute , I don't think sha-512 has that goal in mind. Did you see my links above? Well, I hope that bulk-uploaded passwords require reset after login, but again - if we are auto-generating these passwords they will stored somewhere in plain text, so IMO any attempt to secure that hash is futile. I know what you are saying about md5, and thats a great reason for us to move to bcrypt or alternative by default, but its not a rationale to change the bulk actions, since we're already using it, so need to support it. (And, whilst its cryptographically broken, i'm not convinced this is a major risk in our context of bulk-user creation)
          Hide
          Hubert Chathi added a comment -

          The PHP implementations of the SHA-* algorithms in crypt allow for changing the number of rounds, which is meant to allow you to make the hash slower to compute. Let's do some timings:

          <?php
          $iterations = 100;
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$1$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'MD5: ' . (microtime(true) - $t) . "\n";
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$2y$04$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'bcrypt cost 4: ' . (microtime(true) - $t) . "\n";
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$2y$10$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'bcrypt cost 10: ' . (microtime(true) - $t) . "\n";
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$5$rounds=1000$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'SHA-256 1000 rounds: ' . (microtime(true) - $t) . "\n";
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$5$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'SHA-256 5000 rounds: ' . (microtime(true) - $t) . "\n";
          
          $t = microtime(true);
          $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
          $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb';
          for ($i = 0; $i < $iterations; $i++) {
              crypt($pw, '$5$rounds=50000$' . strrev($salt));
              $pw++; $salt++;
          }
          
          print 'SHA-256 50000 rounds: ' . (microtime(true) - $t) . "\n";
          

          On my system, that gives:

          MD5: 0.034749031066895
          bcrypt cost 4: 0.14812278747559
          bcrypt cost 10: 8.1772599220276
          SHA-256 1000 rounds: 0.14065194129944
          SHA-256 5000 rounds: 0.67985415458679
          SHA-256 50000 rounds: 6.7994530200958
          

          So SHA-256 with 50000 rounds is almost as slow as bcrypt with a cost parameter of 10 (which is what Simon's patch uses). It also looks like SHA-256 with 1000 rounds (the minimum that PHP will allow) is still substantially slower than MD5, so we may still need to use MD5 for bulk actions.

          Basically, as I see it, I don't care a great deal what is used for short-lived passwords, andif we didn't have to worry about the broken bcrypt, then I would be fine with bcrypt. But if we do need to worry about broken bcrypt, then I would rather fall back to SHA-* for PHP < 5.3.7 instead of MD5. And since SHA-* works with PHP >= 5.3.2, then unless there's a reason to use bcrypt over SHA-* when we are able to, then it probably makes sense to just use SHA-*.

          Show
          Hubert Chathi added a comment - The PHP implementations of the SHA-* algorithms in crypt allow for changing the number of rounds, which is meant to allow you to make the hash slower to compute. Let's do some timings: <?php $iterations = 100; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$1$' . strrev($salt)); $pw++; $salt++; } print 'MD5: ' . (microtime( true ) - $t) . "\n" ; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$2y$04$' . strrev($salt)); $pw++; $salt++; } print 'bcrypt cost 4: ' . (microtime( true ) - $t) . "\n" ; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$2y$10$' . strrev($salt)); $pw++; $salt++; } print 'bcrypt cost 10: ' . (microtime( true ) - $t) . "\n" ; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$5$rounds=1000$' . strrev($salt)); $pw++; $salt++; } print 'SHA-256 1000 rounds: ' . (microtime( true ) - $t) . "\n" ; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$5$' . strrev($salt)); $pw++; $salt++; } print 'SHA-256 5000 rounds: ' . (microtime( true ) - $t) . "\n" ; $t = microtime( true ); $salt = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; $pw = 'bbbbbbbbbbbbbbbbbbbbbbbbb'; for ($i = 0; $i < $iterations; $i++) { crypt($pw, '$5$rounds=50000$' . strrev($salt)); $pw++; $salt++; } print 'SHA-256 50000 rounds: ' . (microtime( true ) - $t) . "\n" ; On my system, that gives: MD5: 0.034749031066895 bcrypt cost 4: 0.14812278747559 bcrypt cost 10: 8.1772599220276 SHA-256 1000 rounds: 0.14065194129944 SHA-256 5000 rounds: 0.67985415458679 SHA-256 50000 rounds: 6.7994530200958 So SHA-256 with 50000 rounds is almost as slow as bcrypt with a cost parameter of 10 (which is what Simon's patch uses). It also looks like SHA-256 with 1000 rounds (the minimum that PHP will allow) is still substantially slower than MD5, so we may still need to use MD5 for bulk actions. Basically, as I see it, I don't care a great deal what is used for short-lived passwords, andif we didn't have to worry about the broken bcrypt, then I would be fine with bcrypt. But if we do need to worry about broken bcrypt, then I would rather fall back to SHA-* for PHP < 5.3.7 instead of MD5. And since SHA-* works with PHP >= 5.3.2, then unless there's a reason to use bcrypt over SHA-* when we are able to, then it probably makes sense to just use SHA-*.
          Hide
          Simon Coggins added a comment -

          One advantage of this library is that the implementation has been accepted into PHP core, so eventually this library can be removed as it will be handled natively by PHP.

          It also provides quite a nice method for updating the hashes which provides forward compatibility in case the current algorithm is shown to be vulnerable or if you want to increase the work factor.

          Does anyone know what version of PHP will be required by Moodle 2.5? If it's 5.3.8 or greater then it's a non-issue.

          Show
          Simon Coggins added a comment - One advantage of this library is that the implementation has been accepted into PHP core, so eventually this library can be removed as it will be handled natively by PHP. It also provides quite a nice method for updating the hashes which provides forward compatibility in case the current algorithm is shown to be vulnerable or if you want to increase the work factor. Does anyone know what version of PHP will be required by Moodle 2.5? If it's 5.3.8 or greater then it's a non-issue.
          Hide
          Hubert Chathi added a comment -

          Ah, I see. I had missed the bit about the new PHP password hashing API, and it looks like it only supports bcrypt. Based on that, it does make sense to use bcrypt. And as much as I would like to see it fall back to SHA-* instead of MD5, I understand the reluctance to have three hash algorithms.

          Simon, on this line: https://github.com/totara/moodle/compare/master...master_MDL-35332#L4R4419, should it read PASSWORD_DEFAULT instead of PASSWORD_BCRYPT?

          Show
          Hubert Chathi added a comment - Ah, I see. I had missed the bit about the new PHP password hashing API, and it looks like it only supports bcrypt. Based on that, it does make sense to use bcrypt. And as much as I would like to see it fall back to SHA-* instead of MD5, I understand the reluctance to have three hash algorithms. Simon, on this line: https://github.com/totara/moodle/compare/master...master_MDL-35332#L4R4419 , should it read PASSWORD_DEFAULT instead of PASSWORD_BCRYPT?
          Hide
          Simon Coggins added a comment -

          Hi Hubert,

          Yes you're right, thanks for spotting that. I've updated the branch.

          Show
          Simon Coggins added a comment - Hi Hubert, Yes you're right, thanks for spotting that. I've updated the branch.
          Hide
          Frédéric Massart added a comment -

          Great patch Simon! Thanks for your work on this, this is definitely a great improvement that we should integrate asap! Here are a few comments.

          admin/tool/uploaduser/index.php

          1. When updating an existing user, the password will not be forced to be changed if not weak, and if the option has not been set on the upload form. But in any case that does not really matter as in validate_internal_user_password_md5() we will end up updating the hash if required. The comments could be updated.

          lib/moodlelib.php

          1. I'd personally add a new parameter $legacy to the functions *_user_password() (or use password_is_legacy_hash()) instead of creating the _md5() ones. This would also prevent some code duplication such as in update_internal_user_password().
          2. I would use PASSWORD_BCRYPT instead of PASSWORD_DEFAULT. In the documentation they state that we should use VARCHAR(255) fields for password storage when using DEFAULT. Also I think it's safer to force the algorithm to be sure of the one used, and eventually modify it later on as part of a (security) release if need be.
          3. The documentation of update_internal_user_password() and the comments when calling it should state that this will also update the password when the algorithm has changed, or is still an md5.

          lib/tests/moodlelib_test.php

          1. test_hash_internal_user_password(): Do you think making sure that this returns an MD5 for PHP < 5.3.7 would be interesting?
          2. test_update_internal_user_password(): Perhaps you could add a test that makes sure that validating a MD5 will get the password to be updated to bcrypt() if PHP >= 5.3.7.

          General comments

          • The patch conflicted on master.
          • According to our Coding Style, the comments should start with a capital letter and end with a period.
          • When importing the library, I don't think we need to add the extra files from the repository, the library itself is enough. But you could add a readme_moodle.txt file which contains the details about the library, documentation links, version downloaded, license, etc... Also there might be a newer version now.
          • I guess it has been agreed that Moodle 2.5 will require PHP >= 5.3.7.

          Please let me know what you think about those comments. Thanks!

          Fred

          Show
          Frédéric Massart added a comment - Great patch Simon! Thanks for your work on this, this is definitely a great improvement that we should integrate asap! Here are a few comments. admin/tool/uploaduser/index.php When updating an existing user, the password will not be forced to be changed if not weak, and if the option has not been set on the upload form. But in any case that does not really matter as in validate_internal_user_password_md5() we will end up updating the hash if required. The comments could be updated. lib/moodlelib.php I'd personally add a new parameter $legacy to the functions *_user_password() (or use password_is_legacy_hash()) instead of creating the _md5() ones. This would also prevent some code duplication such as in update_internal_user_password(). I would use PASSWORD_BCRYPT instead of PASSWORD_DEFAULT. In the documentation they state that we should use VARCHAR(255) fields for password storage when using DEFAULT. Also I think it's safer to force the algorithm to be sure of the one used, and eventually modify it later on as part of a (security) release if need be. The documentation of update_internal_user_password() and the comments when calling it should state that this will also update the password when the algorithm has changed, or is still an md5. lib/tests/moodlelib_test.php test_hash_internal_user_password(): Do you think making sure that this returns an MD5 for PHP < 5.3.7 would be interesting? test_update_internal_user_password(): Perhaps you could add a test that makes sure that validating a MD5 will get the password to be updated to bcrypt() if PHP >= 5.3.7. General comments The patch conflicted on master. According to our Coding Style, the comments should start with a capital letter and end with a period. When importing the library, I don't think we need to add the extra files from the repository, the library itself is enough. But you could add a readme_moodle.txt file which contains the details about the library, documentation links, version downloaded, license, etc... Also there might be a newer version now. I guess it has been agreed that Moodle 2.5 will require PHP >= 5.3.7. Please let me know what you think about those comments. Thanks! Fred
          Hide
          Simon Coggins added a comment -

          Thanks for the feedback! I've updated the branch with a new version which fixes those issues, rebased against master and updated to the latest version of the library. Is there a wiki or somewhere else where you track external library dependencies that I should update when this is merged?

          There are still a couple more changes needed depending on some questions I have (see below):

          admin/tool/uploaduser/index.php
          When updating an existing user, the password will not be forced to be changed if not weak, and if the option has not been set on the upload form. But in any case that does not really matter as in validate_internal_user_password_md5() we will end up updating the hash if required. The comments could be updated.

          Force change in this context refers to forcing the user to change their password next time they login rather than updating the hash to a new algorithm. I have not modified the behaviour of that code, and it seems that we correctly set the new hash on L586 regardless of if the password is weak or not. I have updated the comments to try to make it clear that there are two things going on.

          I would use PASSWORD_BCRYPT instead of PASSWORD_DEFAULT. In the documentation they state that we should use VARCHAR(255) fields for password storage when using DEFAULT. Also I think it's safer to force the algorithm to be sure of the one used, and eventually modify it later on as part of a (security) release if need be.

          The advantage of using PASSWORD_DEFAULT is that if a site upgrades PHP to a new release which updates the algorithm, their hashes will immediately start updating to the new algorithm as users log in, without them having to upgrade Moodle too. The old hashes will still work (because the algorithm information is stored with the hash). This was how the library was designed to work. Of course it will only work as described above for sites on PHP5.5+ since the value of PASSWORD_DEFAULT will not change unless the compatibility library code is updated or it is replaced by the native implementation.

          You are right that we should use VARCHAR(255) though, well spotted.

          My preference would be to keep PASSWORD_DEFAULT and increase the password field length to 255, unless you have a compelling reason why it is a bad idea. Why do you consider it safer to be sure of the algorithm used?

          I've made a few other extra changes too:

          1. I looked at updating references to passwordsaltmain in config-dist.php, the installer and the security report. In the end I didn't make any changes since the salt will still be used by the bulk upload code. One place where it was used which won't work now is in lib/phpunit/bootstrap.php where the salt is changed to prevent logins in the test environment:

          $CFG->passwordsaltmain = 'phpunit'; // makes login via normal UI impossible

          That will no longer work so I removed it. I'm not sure if it is needed or what the best way to prevent UI logins while tests are running. One option would be to add:

          // Prevent logins while unit testing.
          if (defined('PHPUNIT_TEST') and PHPUNIT_TEST)

          Unknown macro: { return false; }

          at the top of authenticate_user_login(), but that would make it impossible to write tests for that function.

          Do you know why logins via the UI needed to be prevented and if there is another way to handle it?

          2. The password_compat library documentation recommends testing the return value of password_hash before storing it (false indicates a hashing error). For that to happen something must be badly wrong so I've added some code which throws a coding_exception.

          3. While testing I noticed that if you try to do a site upgrade but are not logged in as an admin, you can't login to hit the notification page, because the system tries to update the admin user's hash but the password field in the database isn't long enough yet. I fixed it by checking the database version in update_internal_user_password(), and using the legacy algorithm if the database hasn't been updated yet.

          Show
          Simon Coggins added a comment - Thanks for the feedback! I've updated the branch with a new version which fixes those issues, rebased against master and updated to the latest version of the library. Is there a wiki or somewhere else where you track external library dependencies that I should update when this is merged? There are still a couple more changes needed depending on some questions I have (see below): admin/tool/uploaduser/index.php When updating an existing user, the password will not be forced to be changed if not weak, and if the option has not been set on the upload form. But in any case that does not really matter as in validate_internal_user_password_md5() we will end up updating the hash if required. The comments could be updated. Force change in this context refers to forcing the user to change their password next time they login rather than updating the hash to a new algorithm. I have not modified the behaviour of that code, and it seems that we correctly set the new hash on L586 regardless of if the password is weak or not. I have updated the comments to try to make it clear that there are two things going on. I would use PASSWORD_BCRYPT instead of PASSWORD_DEFAULT. In the documentation they state that we should use VARCHAR(255) fields for password storage when using DEFAULT. Also I think it's safer to force the algorithm to be sure of the one used, and eventually modify it later on as part of a (security) release if need be. The advantage of using PASSWORD_DEFAULT is that if a site upgrades PHP to a new release which updates the algorithm, their hashes will immediately start updating to the new algorithm as users log in, without them having to upgrade Moodle too. The old hashes will still work (because the algorithm information is stored with the hash). This was how the library was designed to work. Of course it will only work as described above for sites on PHP5.5+ since the value of PASSWORD_DEFAULT will not change unless the compatibility library code is updated or it is replaced by the native implementation. You are right that we should use VARCHAR(255) though, well spotted. My preference would be to keep PASSWORD_DEFAULT and increase the password field length to 255, unless you have a compelling reason why it is a bad idea. Why do you consider it safer to be sure of the algorithm used? I've made a few other extra changes too: 1. I looked at updating references to passwordsaltmain in config-dist.php, the installer and the security report. In the end I didn't make any changes since the salt will still be used by the bulk upload code. One place where it was used which won't work now is in lib/phpunit/bootstrap.php where the salt is changed to prevent logins in the test environment: $CFG->passwordsaltmain = 'phpunit'; // makes login via normal UI impossible That will no longer work so I removed it. I'm not sure if it is needed or what the best way to prevent UI logins while tests are running. One option would be to add: // Prevent logins while unit testing. if (defined('PHPUNIT_TEST') and PHPUNIT_TEST) Unknown macro: { return false; } at the top of authenticate_user_login(), but that would make it impossible to write tests for that function. Do you know why logins via the UI needed to be prevented and if there is another way to handle it? 2. The password_compat library documentation recommends testing the return value of password_hash before storing it (false indicates a hashing error). For that to happen something must be badly wrong so I've added some code which throws a coding_exception. 3. While testing I noticed that if you try to do a site upgrade but are not logged in as an admin, you can't login to hit the notification page, because the system tries to update the admin user's hash but the password field in the database isn't long enough yet. I fixed it by checking the database version in update_internal_user_password(), and using the legacy algorithm if the database hasn't been updated yet.
          Hide
          Frédéric Massart added a comment -

          Hi Simon,

          • That's all good about the upload of users, I think the previous comments got me confused but the current logic is perfect.
          • If PASSWORD_DEFAULT is better, safe and reliable, then let's go for it!
          • (1) Hum... I think you should not remove the salt from there. To me it looks like this has been added so we make sure that we would never be able to authenticate a real user because the salt differs from the production environment. But, when BCRYPT will be used, then there could be match and we don't want that (good spotting). I see a few possible solutions (I'm not convinced by any though):
            • We ignore that and assume PHP Unit will never talk with the production environment;
            • We use the salt even when we use BCRYPT (I know BCRYPT itself uses a salt, so redundancy? Useless?);
            • We use a different 'cost' or setting for password_compat() in PHP Unit (Not a very nice solution...);
            • We return false in authenticate_user_login as you suggested (Probably the best solution).
          • (2) Good thinking. Do you think this should be a coding_exception? A moodle_exception sound more appropriate to me.
          • (3) Good thinking too! I quite dislike the hardcoded version number in that function, but you might want to have a look at setuplib.php@redirect_if_major_upgrade_required() which would force the upgrade script to be run. As we are touching the user table, that might be acceptable. Not sure what would be the integrators point of view on that.

          Thanks for your work on this Simon. Please comment and/or edit your patch about the comments above and fell free to push for integration once it's done. Oh and maybe you'll want to squash a few of your commmits.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Simon, That's all good about the upload of users, I think the previous comments got me confused but the current logic is perfect. If PASSWORD_DEFAULT is better, safe and reliable, then let's go for it! (1) Hum... I think you should not remove the salt from there. To me it looks like this has been added so we make sure that we would never be able to authenticate a real user because the salt differs from the production environment. But, when BCRYPT will be used, then there could be match and we don't want that (good spotting). I see a few possible solutions (I'm not convinced by any though): We ignore that and assume PHP Unit will never talk with the production environment; We use the salt even when we use BCRYPT (I know BCRYPT itself uses a salt, so redundancy? Useless?); We use a different 'cost' or setting for password_compat() in PHP Unit (Not a very nice solution...); We return false in authenticate_user_login as you suggested (Probably the best solution). (2) Good thinking. Do you think this should be a coding_exception? A moodle_exception sound more appropriate to me. (3) Good thinking too! I quite dislike the hardcoded version number in that function, but you might want to have a look at setuplib.php@redirect_if_major_upgrade_required() which would force the upgrade script to be run. As we are touching the user table, that might be acceptable. Not sure what would be the integrators point of view on that. Thanks for your work on this Simon. Please comment and/or edit your patch about the comments above and fell free to push for integration once it's done. Oh and maybe you'll want to squash a few of your commmits. Cheers, Fred
          Hide
          Dan Poltawski added a comment -

          Regarding phpunit (jumping in on this without understanding it much, apologies if i've misunderstood). I think that Petr has tried really really hard to make sure we don't use phpunit data in production sites.. IMO, perhaps too hard. I don't think it is necessary to extend this to adding something in authenticate_user_login(). There are lots of other checks ensuring that phpunit isn't talking to the production database. If anyone thinks that this is necessary, please come up with a use case for where this would be a problem.

          Show
          Dan Poltawski added a comment - Regarding phpunit (jumping in on this without understanding it much, apologies if i've misunderstood). I think that Petr has tried really really hard to make sure we don't use phpunit data in production sites.. IMO, perhaps too hard. I don't think it is necessary to extend this to adding something in authenticate_user_login(). There are lots of other checks ensuring that phpunit isn't talking to the production database. If anyone thinks that this is necessary, please come up with a use case for where this would be a problem.
          Hide
          Simon Coggins added a comment -

          1. I agree with Dan (that it should not be necessary), and would like to avoid the change to authenticate_user_login if possible because of the impact on testing it. Petr do you have a view on this?

          2. Changed to moodle_exception.

          3. Yes I agree the hardcoded version in the password function was not very nice. I have used redirect_if_major_upgrade_required() instead. Of course I am now just hardcoding the version in a different place! With this change the upgrade will occur without requiring a login, so any user visiting might trigger the upgrade (not just admins). I think that's okay for a major upgrade though.

          Interestingly last night I noticed password_compat has just came out of beta and it has had the version constraint removed. The reason is that some distributions backported the fix into older PHP releases so instead they have come up with a test which checks the actual functionality instead of just the version number. Here is the discussion:

          https://github.com/ircmaxell/password_compat/issues/10

          And this confirms that their test works as expected:

          http://3v4l.org/0sgNq

          So I've created a password_compat_not_supported() to use instead of the version check and updated to the latest library again.

          Finally, I've squashed everything into a single commit, and rebased against the latest master, updated the version numbers, and retested.

          Show
          Simon Coggins added a comment - 1. I agree with Dan (that it should not be necessary), and would like to avoid the change to authenticate_user_login if possible because of the impact on testing it. Petr do you have a view on this? 2. Changed to moodle_exception. 3. Yes I agree the hardcoded version in the password function was not very nice. I have used redirect_if_major_upgrade_required() instead. Of course I am now just hardcoding the version in a different place! With this change the upgrade will occur without requiring a login, so any user visiting might trigger the upgrade (not just admins). I think that's okay for a major upgrade though. Interestingly last night I noticed password_compat has just came out of beta and it has had the version constraint removed. The reason is that some distributions backported the fix into older PHP releases so instead they have come up with a test which checks the actual functionality instead of just the version number. Here is the discussion: https://github.com/ircmaxell/password_compat/issues/10 And this confirms that their test works as expected: http://3v4l.org/0sgNq So I've created a password_compat_not_supported() to use instead of the version check and updated to the latest library again. Finally, I've squashed everything into a single commit, and rebased against the latest master, updated the version numbers, and retested.
          Hide
          Frédéric Massart added a comment -

          Looks good to me Simon, I'm pushing your patch for integration. Cheers!

          Show
          Frédéric Massart added a comment - Looks good to me Simon, I'm pushing your patch for integration. Cheers!
          Hide
          Simon Coggins added a comment -

          Sorry I'm going to have to pull this out of integration review. I've noticed that using redirect_if_major_upgrade_required() has broken fresh installs via the UI because the admin user can't set their password.

          I'll push a fix and resubmit.

          Show
          Simon Coggins added a comment - Sorry I'm going to have to pull this out of integration review. I've noticed that using redirect_if_major_upgrade_required() has broken fresh installs via the UI because the admin user can't set their password. I'll push a fix and resubmit.
          Hide
          Simon Coggins added a comment -

          Okay, I've pushed another version so this is ready for integration review again. I don't seem to have permissions to push to review so Fred could you do it?

          For now I've left the fixes as separate commits so people can see what I've done, I can squash them prior to merging if preferred.

          Show
          Simon Coggins added a comment - Okay, I've pushed another version so this is ready for integration review again. I don't seem to have permissions to push to review so Fred could you do it? For now I've left the fixes as separate commits so people can see what I've done, I can squash them prior to merging if preferred.
          Hide
          Frédéric Massart added a comment -

          Hi Simon, pushing for integration as per your request. I had a quick glance at the patch so I felt like mentioning this:

          • I'm not sure about the use of redirect_if_major_upgrade_required() in upgrade_password, but I guess you have your reasons.
          • I'd suggest changing == with === in the function that checks if password_compat is supported. Also, returning $test !== 'supported' might be easier to understand.

          Thanks !

          Show
          Frédéric Massart added a comment - Hi Simon, pushing for integration as per your request. I had a quick glance at the patch so I felt like mentioning this: I'm not sure about the use of redirect_if_major_upgrade_required() in upgrade_password, but I guess you have your reasons. I'd suggest changing == with === in the function that checks if password_compat is supported. Also, returning $test !== 'supported' might be easier to understand. Thanks !
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Simon Coggins added a comment -

          You are right about redirect_if_major_upgrade_required(), I didn't realise that it was already called on login/index.php so my call was not necessary. I've removed it.

          I've also updated the equality operator, rebased against latest master and squashed the commits so all good to go again.

          Thanks Frédéric!

          Show
          Simon Coggins added a comment - You are right about redirect_if_major_upgrade_required(), I didn't realise that it was already called on login/index.php so my call was not necessary. I've removed it. I've also updated the equality operator, rebased against latest master and squashed the commits so all good to go again. Thanks Frédéric!
          Hide
          Dan Poltawski added a comment - - edited

          Hi Simon,

          This is is looking great, thanks. I have spotted a few blockers for this, but I think that is looking fine for integration once we've got over those.

          password_compat_not_supported()

          1. To be honest, i'm unsure if we should do this expensive check rather than just use php version (and backported fixes suffer without it). I think we need some votes on this.
          2. I didn't think that making a 'one off cache' like that is supposed to be supported (will check with Sam). i.e. it needs defining properly in db/caches.php
          3. Trivial: $bcrypt_support should by $bcryptsupport (no underscores in variables) http://docs.moodle.org/dev/Coding_style#Variables

          validate_internal_user_password()

          4. Maybe now is the time to define 'not cached' as a constant somewhere, I think its being used in a few differnet places in code and its exact phrasing turns out to be important!

          hash_internal_user_password()

          5. I think that it is worth put a comment about using $legacyhash being useful for operations which need to be quick (i.e. bulk users) so people addding these features in future will know to use it. People who don't know about bcrypt wont understand this being necesary (same for setnew_password_and_mail)

          update_internal_user_password()

          6. Underscores in variables $password_changed $algorithm_changed

          lib/password_compat/tests/*

          7. These tests are created by us, right? If so, they should have the proper Moodle copyright header, MOODLE_INTERNAL checks etc.
          8. Wrapping the whole test class in an IF statement is not really a nice way of doing it (and I guess it probably depends on the moodle environment being setup at the point of the test being included). Elsewhere we use markTestSkipped() to do this kind of thing. It looks like it might be painful to do on every test, we should look if there is a better way to do this with phpunit. Petr, any thoughts?

          $CFG->passwordsaltmain

          9. I've always disliked this peice of configuration as its very critical and easy to loose. Just wondering if you've thought about the lifetime of this config variable. Now when we move everyone to bcypt and have user salts then we might be able to get rid of it eventually (or at least, its not as critical if people loose it). BUT, its still being used for the legacy (/fast) hashes then we can never get rid of it. Maybe it'd be good to move away from it alltogether. Just a thought anyway.

          In summary, I think that points 1,2,7,8 need clarifying or fixing before we can integrate this and the other points would be good to fix too.
          thanks,

          Dan

          Show
          Dan Poltawski added a comment - - edited Hi Simon, This is is looking great, thanks. I have spotted a few blockers for this, but I think that is looking fine for integration once we've got over those. password_compat_not_supported() 1. To be honest, i'm unsure if we should do this expensive check rather than just use php version (and backported fixes suffer without it). I think we need some votes on this. 2. I didn't think that making a 'one off cache' like that is supposed to be supported (will check with Sam). i.e. it needs defining properly in db/caches.php 3. Trivial: $bcrypt_support should by $bcryptsupport (no underscores in variables) http://docs.moodle.org/dev/Coding_style#Variables validate_internal_user_password() 4. Maybe now is the time to define 'not cached' as a constant somewhere, I think its being used in a few differnet places in code and its exact phrasing turns out to be important! hash_internal_user_password() 5. I think that it is worth put a comment about using $legacyhash being useful for operations which need to be quick (i.e. bulk users) so people addding these features in future will know to use it. People who don't know about bcrypt wont understand this being necesary (same for setnew_password_and_mail) update_internal_user_password() 6. Underscores in variables $password_changed $algorithm_changed lib/password_compat/tests/* 7. These tests are created by us, right? If so, they should have the proper Moodle copyright header, MOODLE_INTERNAL checks etc. 8. Wrapping the whole test class in an IF statement is not really a nice way of doing it (and I guess it probably depends on the moodle environment being setup at the point of the test being included). Elsewhere we use markTestSkipped() to do this kind of thing. It looks like it might be painful to do on every test, we should look if there is a better way to do this with phpunit. Petr, any thoughts? $CFG->passwordsaltmain 9. I've always disliked this peice of configuration as its very critical and easy to loose. Just wondering if you've thought about the lifetime of this config variable. Now when we move everyone to bcypt and have user salts then we might be able to get rid of it eventually (or at least, its not as critical if people loose it). BUT, its still being used for the legacy (/fast) hashes then we can never get rid of it. Maybe it'd be good to move away from it alltogether. Just a thought anyway. In summary, I think that points 1,2,7,8 need clarifying or fixing before we can integrate this and the other points would be good to fix too. thanks, Dan
          Hide
          Dan Poltawski added a comment -

          Sam, could you comment on the MUC cache?

          Show
          Dan Poltawski added a comment - Sam, could you comment on the MUC cache?
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Sam Hemelryk added a comment -

          Hiya,

          I've just been looking at the cache use here (and only the cache use).
          Having run tests I think its certainly worthwhile. The following are the average execution times for running crypt, or retrieving from a cache (in microsecs).

          Crypt 0.0030241012573242
          Cache 0.00033092498779297

          While the operation is pretty fast anyway its clearly faster with the cache involved.

          The thing that caught my eye was that the cache is not using a definition.
          Having weighed that up I think that is the correct decision here. While there is an argument for allowing users to move it to a store of there choice it will add clutter to the admin interfaces, and as its called only when logging in best left as it is.

          Gets a +1 from me on the cache front.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hiya, I've just been looking at the cache use here (and only the cache use). Having run tests I think its certainly worthwhile. The following are the average execution times for running crypt, or retrieving from a cache (in microsecs). Crypt 0.0030241012573242 Cache 0.00033092498779297 While the operation is pretty fast anyway its clearly faster with the cache involved. The thing that caught my eye was that the cache is not using a definition. Having weighed that up I think that is the correct decision here. While there is an argument for allowing users to move it to a store of there choice it will add clutter to the admin interfaces, and as its called only when logging in best left as it is. Gets a +1 from me on the cache front. Many thanks Sam
          Hide
          Simon Coggins added a comment -

          Thanks Dan,

          I'm out of the office this week but I'll respond to the questions and push some updates when I'm back next week.

          Show
          Simon Coggins added a comment - Thanks Dan, I'm out of the office this week but I'll respond to the questions and push some updates when I'm back next week.
          Hide
          Simon Coggins added a comment -

          1. It's not that expensive, it's just that it could be called a lot for bulk imports and since it's not going to change I thought it made sense to cache it. The advantage is relatively minor - it means that the new method will work with 5.3.2 for any distribution that has backported the bcrypt fix back to that version. On balance I think it's probably worth it, but I don't feel that strongly about it so happy to switch back to the version check if people prefer.

          2. Based on Sam's comments I'm assuming it's okay.

          3. I'll fix that.

          4. Should I do that as part of this patch or would it make sense to file another bug?

          5. Will do, unless we change how we do fast hashing (see below) in which case it is no longer relevant.

          6. Will fix.

          7. No actually these test are part of the password_compat library. I thought I'd include them in case any unexpected environment issue caused them to fail, it might help someone discover why.

          8. Yeah I couldn't come up with a better way - I couldn't return early because the files are included. I didn't know about markTestSkipped() though. I'll switch to that unless anyone can come up with anything better. Unfortunately it makes merging future versions of the library a bit more difficult if there are changes to the tests.

          9. Yes I had hoped to get rid of it but I thought we needed it around for the fast hashing. Thinking about it some more, another option for the fast hashing would be to use bcrypt, but with a very low cost factor.

          With a cost factor of 1 I can get ~50,000 hashes/second on my machine which is still 16 times slower than md5 but good enough for bulk imports. The current code would automatically update the low cost hashes on the next login. They would also get per-user salts.

          We would still need to keep the salt config variables around (and check old md5 hashes using them) for backward compatibility, but it wouldn't be necessary to set it for new installs so it could come out of config-dist.php (or at least be must less visible).

          Unless anyone has any objections I think that change would make sense.

          Show
          Simon Coggins added a comment - 1. It's not that expensive, it's just that it could be called a lot for bulk imports and since it's not going to change I thought it made sense to cache it. The advantage is relatively minor - it means that the new method will work with 5.3.2 for any distribution that has backported the bcrypt fix back to that version. On balance I think it's probably worth it, but I don't feel that strongly about it so happy to switch back to the version check if people prefer. 2. Based on Sam's comments I'm assuming it's okay. 3. I'll fix that. 4. Should I do that as part of this patch or would it make sense to file another bug? 5. Will do, unless we change how we do fast hashing (see below) in which case it is no longer relevant. 6. Will fix. 7. No actually these test are part of the password_compat library. I thought I'd include them in case any unexpected environment issue caused them to fail, it might help someone discover why. 8. Yeah I couldn't come up with a better way - I couldn't return early because the files are included. I didn't know about markTestSkipped() though. I'll switch to that unless anyone can come up with anything better. Unfortunately it makes merging future versions of the library a bit more difficult if there are changes to the tests. 9. Yes I had hoped to get rid of it but I thought we needed it around for the fast hashing. Thinking about it some more, another option for the fast hashing would be to use bcrypt, but with a very low cost factor. With a cost factor of 1 I can get ~50,000 hashes/second on my machine which is still 16 times slower than md5 but good enough for bulk imports. The current code would automatically update the low cost hashes on the next login. They would also get per-user salts. We would still need to keep the salt config variables around (and check old md5 hashes using them) for backward compatibility, but it wouldn't be necessary to set it for new installs so it could come out of config-dist.php (or at least be must less visible). Unless anyone has any objections I think that change would make sense.
          Hide
          Dan Poltawski added a comment -

          4. Should I do that as part of this patch or would it make sense to file another bug?

          If you can do this as part of this it'd be great, although its not exactly a blocker.

          Thinking about it some more, another option for the fast hashing would be to use bcrypt, but with a very low cost factor.

          I like this idea. +1

          Show
          Dan Poltawski added a comment - 4. Should I do that as part of this patch or would it make sense to file another bug? If you can do this as part of this it'd be great, although its not exactly a blocker. Thinking about it some more, another option for the fast hashing would be to use bcrypt, but with a very low cost factor. I like this idea. +1
          Hide
          Simon Coggins added a comment -

          Okay all done. I have done 4. as part of the patch and moved to low-cost bcrypt as fallback. The lowest cost factor I was allowed to use was 4 but it's still fast enough.

          Show
          Simon Coggins added a comment - Okay all done. I have done 4. as part of the patch and moved to low-cost bcrypt as fallback. The lowest cost factor I was allowed to use was 4 but it's still fast enough.
          Hide
          Simon Coggins added a comment -

          This is ready for integration review. Could someone please set the status as I don't have permissions?

          Show
          Simon Coggins added a comment - This is ready for integration review. Could someone please set the status as I don't have permissions?
          Hide
          Dan Poltawski added a comment -

          Done. Thanks

          Show
          Dan Poltawski added a comment - Done. Thanks
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Cheers!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Cheers!
          Hide
          Simon Coggins added a comment -

          Rebased against latest master.

          Show
          Simon Coggins added a comment - Rebased against latest master.
          Hide
          Damyon Wiese added a comment -

          Testing user upload with 1000 users took 12.44 seconds.

          Show
          Damyon Wiese added a comment - Testing user upload with 1000 users took 12.44 seconds.
          Hide
          Damyon Wiese added a comment -

          Testing time of new / old hash functions:
          Time to hash 100 with bcrypt (slow):7.2197759151459
          Time to hash 1000 with bcrypt (fast):1.4305491447449
          Time to hash 1000 with md5 :0.00057816505432129

          This looks fast enough for the import situation.

          Show
          Damyon Wiese added a comment - Testing time of new / old hash functions: Time to hash 100 with bcrypt (slow):7.2197759151459 Time to hash 1000 with bcrypt (fast):1.4305491447449 Time to hash 1000 with md5 :0.00057816505432129 This looks fast enough for the import situation.
          Hide
          Damyon Wiese added a comment -

          Thanks to all for the collaboration/work on this issue.

          I have pushed this to integration master.

          I think it is a big improvement. I did lots of testing and followed the discussion and I am happy with this solution. It would be good to mention the php versions required for this feature in the release notes for 2.5 as well as the upgrade behaviour (added docs_required label).

          There may need to be some special upgrade notes for people upgrading a clustered Moodle (the php versions should be kept in sync across the nodes or they may get some login failures - would require an outage rather than upgrading the nodes one by one).

          Show
          Damyon Wiese added a comment - Thanks to all for the collaboration/work on this issue. I have pushed this to integration master. I think it is a big improvement. I did lots of testing and followed the discussion and I am happy with this solution. It would be good to mention the php versions required for this feature in the release notes for 2.5 as well as the upgrade behaviour (added docs_required label). There may need to be some special upgrade notes for people upgrading a clustered Moodle (the php versions should be kept in sync across the nodes or they may get some login failures - would require an outage rather than upgrading the nodes one by one).
          Hide
          Dan Poltawski added a comment - - edited

          Hi,

          It looks like Starting test 'accesslib_testcase::test_permission_evaluation'. is taking a small lifetime to complete on my machine after this change.

          vendor/bin/phpunit --filter=test_permission_evaluation lib/tests/accesslib_test.php
          PHPUnit 3.7.10 by Sebastian Bergmann.

          Configuration read from /Users/danp/git/integration/phpunit.xml

          /.

          Time: 04:56, Memory: 63.50Mb

          OK (1 test, 13174 assertions)

          Show
          Dan Poltawski added a comment - - edited Hi, It looks like Starting test 'accesslib_testcase::test_permission_evaluation'. is taking a small lifetime to complete on my machine after this change. vendor/bin/phpunit --filter=test_permission_evaluation lib/tests/accesslib_test.php PHPUnit 3.7.10 by Sebastian Bergmann. Configuration read from /Users/danp/git/integration/phpunit.xml /. Time: 04:56, Memory: 63.50Mb OK (1 test, 13174 assertions)
          Hide
          Simon Coggins added a comment -

          Ah - I suspect this is the problem:

          $testusers= array();
          for ($i=0; $i<CONTEXT_CACHE_MAX_SIZE + 100; $i++) {
              $user = $generator->create_user();
              $testusers[$i] = $user->id;
          }
          

          since CONTEXT_CACHE_MAX_SIZE is 2500 and create_user() will be generating a hash. I'll look at modifying the data generator code to use a fast hash too.

          Show
          Simon Coggins added a comment - Ah - I suspect this is the problem: $testusers= array(); for ($i=0; $i<CONTEXT_CACHE_MAX_SIZE + 100; $i++) { $user = $generator->create_user(); $testusers[$i] = $user->id; } since CONTEXT_CACHE_MAX_SIZE is 2500 and create_user() will be generating a hash. I'll look at modifying the data generator code to use a fast hash too.
          Hide
          Dan Poltawski added a comment -

          Thanks Simon, I changed the generator to use the fast hash and it went down to 35 seconds. But i've also just been wondering if we should generate the hash at all (in most scenarios).

          We were just discussing on the dev chat and perhaps we cna stop hashing by default.

          Show
          Dan Poltawski added a comment - Thanks Simon, I changed the generator to use the fast hash and it went down to 35 seconds. But i've also just been wondering if we should generate the hash at all (in most scenarios). We were just discussing on the dev chat and perhaps we cna stop hashing by default.
          Hide
          Damyon Wiese added a comment -

          Thanks Simon - Dan is testing a patch for this issue. We'll probably just update the generator to use the fast hash.

          Show
          Damyon Wiese added a comment - Thanks Simon - Dan is testing a patch for this issue. We'll probably just update the generator to use the fast hash.
          Hide
          Simon Coggins added a comment - - edited

          It's a one line change to the data generator.

          Switching to the fast hash seems to do the trick:

          Current master (without patch): Time: 39 seconds, Memory: 101.75Mb
          With patch (fast hash): Time: 47 seconds, Memory: 101.75Mb
          With patch (default hash): Time: 03:16, Memory: 101.75Mb

          I've pushed that as a separate commit to the branch.

          Show
          Simon Coggins added a comment - - edited It's a one line change to the data generator. Switching to the fast hash seems to do the trick: Current master (without patch): Time: 39 seconds, Memory: 101.75Mb With patch (fast hash): Time: 47 seconds, Memory: 101.75Mb With patch (default hash): Time: 03:16, Memory: 101.75Mb I've pushed that as a separate commit to the branch.
          Hide
          Dan Poltawski added a comment -

          Thanks Simon, i've pulled that in (same as my patch). I'm going to create a new issue about removing hashing alltogether when not necessary.

          Show
          Dan Poltawski added a comment - Thanks Simon, i've pulled that in (same as my patch). I'm going to create a new issue about removing hashing alltogether when not necessary.
          Hide
          Dan Poltawski added a comment -

          Created MDL-37963 for that.

          Show
          Dan Poltawski added a comment - Created MDL-37963 for that.
          Hide
          Simon Coggins added a comment -

          Regarding documentation: Good point about the cluster upgrades. I can put together some draft release notes as a starting point if you like.

          Is there a 2.5 version docs wiki yet? Some pages, notably http://docs.moodle.org/24/en/Password_salting will need updating.

          Also do you have a method of tracking external dependencies for managing updates? If so lib/password_compat/ should go on that list.

          Show
          Simon Coggins added a comment - Regarding documentation: Good point about the cluster upgrades. I can put together some draft release notes as a starting point if you like. Is there a 2.5 version docs wiki yet? Some pages, notably http://docs.moodle.org/24/en/Password_salting will need updating. Also do you have a method of tracking external dependencies for managing updates? If so lib/password_compat/ should go on that list.
          Hide
          Rossiani Wijaya added a comment -

          Hi Simon,

          This is working great with PHP Version 5.4.

          However when I tested with PHP Version 5.3.2, the password doesn't convert at all. It kept the old hash password.

          Could you confirm this issue?

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Simon, This is working great with PHP Version 5.4. However when I tested with PHP Version 5.3.2, the password doesn't convert at all. It kept the old hash password. Could you confirm this issue? Thanks
          Hide
          Simon Coggins added a comment -

          Hi Rossiani,

          Yes that is correct - the new hashing library isn't supported prior to 5.3.7 so it should fall back to using the "old" system (i.e. continue to use md5 hashes).

          This bit of the testing instructions:

          Test functionality with PHP version 5.3.6 or below
          Repeat the tests above making sure everything works - should continue
          to function as before but using only the old algorithm (md5)

          just means that no existing functionality should have been broken in older PHP versions.

          Show
          Simon Coggins added a comment - Hi Rossiani, Yes that is correct - the new hashing library isn't supported prior to 5.3.7 so it should fall back to using the "old" system (i.e. continue to use md5 hashes). This bit of the testing instructions: Test functionality with PHP version 5.3.6 or below Repeat the tests above making sure everything works - should continue to function as before but using only the old algorithm (md5) just means that no existing functionality should have been broken in older PHP versions.
          Hide
          Rossiani Wijaya added a comment -

          Hi Simon,

          Thanks for the feedback.

          I will continue testing this tomorrow morning. I need to use Adrian machine to test this with older version of PHP.

          Show
          Rossiani Wijaya added a comment - Hi Simon, Thanks for the feedback. I will continue testing this tomorrow morning. I need to use Adrian machine to test this with older version of PHP.
          Hide
          Rossiani Wijaya added a comment -

          Hi Simon,

          This works great with PHP version 5.3.2.

          However with the lack of email configuration with the system, I can't test all the confirmation that's done through email. Instead, I checked the password field and it's generating the correct hash for the user.

          Also just noting on minor test result for Account access via non-caching auth plugin:

          • step 6: new user can't be save with empty password, therefore the user table password field contains the hash value of the password instead of 'not cached'.

          Other than that, this is working great.

          Show
          Rossiani Wijaya added a comment - Hi Simon, This works great with PHP version 5.3.2. However with the lack of email configuration with the system, I can't test all the confirmation that's done through email. Instead, I checked the password field and it's generating the correct hash for the user. Also just noting on minor test result for Account access via non-caching auth plugin : step 6: new user can't be save with empty password, therefore the user table password field contains the hash value of the password instead of 'not cached'. Other than that, this is working great.
          Hide
          Aparup Banerjee added a comment -

          Hi,
          Rossi asked me if this would be ok to pass. imo, given the patch size versus the extensive test with the few issues being detected, i think this should be ok to pass (its master only too).

          Show
          Aparup Banerjee added a comment - Hi, Rossi asked me if this would be ok to pass. imo, given the patch size versus the extensive test with the few issues being detected, i think this should be ok to pass (its master only too).
          Hide
          Rossiani Wijaya added a comment -

          Thanks Apu for your feedback.

          I'm passing this test.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Thanks Apu for your feedback. I'm passing this test. Test passed.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!
          Hide
          Frédéric Massart added a comment -

          Before raising an issue, I'd like to report some bugs I'm having.

          On MacOS 10.7.3, PHP 5.3.8. A moodle_exception is raised while trying to hash the old MD5 password after login:

          Debug info:
          Error code: Failed to generate password hash.
          $a contents:
          Stack trace:
          
              line 4457 of /lib/moodlelib.php: moodle_exception thrown
              line 4492 of /lib/moodlelib.php: call to hash_internal_user_password()
              line 4418 of /lib/moodlelib.php: call to update_internal_user_password()
              line 62 of /auth/manual/auth.php: call to validate_internal_user_password()
              line 4199 of /lib/moodlelib.php: call to auth_plugin_manual->user_login()
              line 133 of /login/index.php: call to authenticate_user_login()
          
          • The $password passed is test
          • The $hash right before the line '$ret = crypt(...)' is: $2y$10$yKPv9N0zLo7fEzVZKV4npL
          • The $ret is then: $2rcByx51ejoM
          • And the exception is raised.

          That also made me realise that if this function returns null, which can happen, we would (try to) save a null password in the database.

          Am I doing something wrong or should an issue be raised?

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Before raising an issue, I'd like to report some bugs I'm having. On MacOS 10.7.3, PHP 5.3.8. A moodle_exception is raised while trying to hash the old MD5 password after login: Debug info: Error code: Failed to generate password hash. $a contents: Stack trace: line 4457 of /lib/moodlelib.php: moodle_exception thrown line 4492 of /lib/moodlelib.php: call to hash_internal_user_password() line 4418 of /lib/moodlelib.php: call to update_internal_user_password() line 62 of /auth/manual/auth.php: call to validate_internal_user_password() line 4199 of /lib/moodlelib.php: call to auth_plugin_manual->user_login() line 133 of /login/index.php: call to authenticate_user_login() The $password passed is test The $hash right before the line '$ret = crypt(...)' is: $2y$10$yKPv9N0zLo7fEzVZKV4npL The $ret is then: $2rcByx51ejoM And the exception is raised. That also made me realise that if this function returns null, which can happen, we would (try to) save a null password in the database. Am I doing something wrong or should an issue be raised? Cheers, Fred
          Hide
          Damyon Wiese added a comment -

          Are there any debug messages in your apache log Fred ?

          Show
          Damyon Wiese added a comment - Are there any debug messages in your apache log Fred ?
          Hide
          Frédéric Massart added a comment -

          Hum, I checked the error log of PHP and nothing was there but the exception thrown.

          Show
          Frédéric Massart added a comment - Hum, I checked the error log of PHP and nothing was there but the exception thrown.
          Hide
          Damyon Wiese added a comment -

          Please add a new issue with as much detail as possible and we'll try make sure we cater for this environment (if crypt is not working it should at least keep the md5 hashes).

          Show
          Damyon Wiese added a comment - Please add a new issue with as much detail as possible and we'll try make sure we cater for this environment (if crypt is not working it should at least keep the md5 hashes).
          Hide
          Frédéric Massart added a comment -

          Raised MDL-38102.

          Show
          Frédéric Massart added a comment - Raised MDL-38102 .
          Hide
          Dan Poltawski added a comment -

          People interested in this topic might be interested in this presentation:

          http://passwords12.at.ifi.uio.no/Simon_Marechal_The_Future_Of_Hashing/

          (mostly over my head though )

          Show
          Dan Poltawski added a comment - People interested in this topic might be interested in this presentation: http://passwords12.at.ifi.uio.no/Simon_Marechal_The_Future_Of_Hashing/ (mostly over my head though )
          Hide
          Dan Poltawski added a comment -

          Hi Simon,

          I've just put some (pretty poorly worded) comments about this on http://docs.moodle.org/25/en/Upgrading_to_Moodle_2.5 and http://docs.moodle.org/dev/Moodle_2.5_release_notes, if you could improve it it'd be great. Just thinking we need to make sure this isn't forgotten about!

          Show
          Dan Poltawski added a comment - Hi Simon, I've just put some (pretty poorly worded) comments about this on http://docs.moodle.org/25/en/Upgrading_to_Moodle_2.5 and http://docs.moodle.org/dev/Moodle_2.5_release_notes , if you could improve it it'd be great. Just thinking we need to make sure this isn't forgotten about!
          Hide
          Simon Coggins added a comment -

          I've updated them with a bit more detail covering some other edge cases (such as downgrading PHP or restoring a course to an older site with $CFG->includeuserpasswordsinbackup setting enabled.

          Show
          Simon Coggins added a comment - I've updated them with a bit more detail covering some other edge cases (such as downgrading PHP or restoring a course to an older site with $CFG->includeuserpasswordsinbackup setting enabled.
          Hide
          Dan Poltawski added a comment -

          Thanks!

          Show
          Dan Poltawski added a comment - Thanks!
          Hide
          Helen Foster added a comment -

          Dan and Simon, thanks for your docs contributions. I just changed the first sentence of http://docs.moodle.org/25/en/Password_salting but don't know anything about it, so would appreciate help in updating the page.

          Show
          Helen Foster added a comment - Dan and Simon, thanks for your docs contributions. I just changed the first sentence of http://docs.moodle.org/25/en/Password_salting but don't know anything about it, so would appreciate help in updating the page.
          Hide
          Simon Coggins added a comment -

          Ah well spotted Helen, I missed that page. I have updated it now - explaining that it from 2.5 is no longer required except for upgrades or old PHP versions. I've also linked to the 2.4 docs for anyone who still needs to use it. Dan would you be able to take a look to double check you are happy?

          Also this page needs to be edited to remove the reference to md5:

          http://docs.moodle.org/25/en/Security

          but I don't have edit permissions. Could someone change that?

          Show
          Simon Coggins added a comment - Ah well spotted Helen, I missed that page. I have updated it now - explaining that it from 2.5 is no longer required except for upgrades or old PHP versions. I've also linked to the 2.4 docs for anyone who still needs to use it. Dan would you be able to take a look to double check you are happy? Also this page needs to be edited to remove the reference to md5: http://docs.moodle.org/25/en/Security but I don't have edit permissions. Could someone change that?
          Hide
          Helen Foster added a comment -

          http://docs.moodle.org/25/en/Password_salting looks great - many thanks Simon. I have removed the reference to md5 in http://docs.moodle.org/25/en/Security and a couple of other places.

          I'm removing the docs_required label now, though anyone please feel free to further improve the docs as necessary (leaving a comment on the talk page if a page is protected).

          Show
          Helen Foster added a comment - http://docs.moodle.org/25/en/Password_salting looks great - many thanks Simon. I have removed the reference to md5 in http://docs.moodle.org/25/en/Security and a couple of other places. I'm removing the docs_required label now, though anyone please feel free to further improve the docs as necessary (leaving a comment on the talk page if a page is protected).
          Hide
          Dan Poltawski added a comment -

          I think that page is great, really clear, thanks Simon!

          Show
          Dan Poltawski added a comment - I think that page is great, really clear, thanks Simon!
          Hide
          Ryan Foster added a comment -

          From the testing instructions, it seemed to me initially that the new password hashing would only affect installations with PHP 5.3.7 or higher (due to a bug in PHP's implementation of bcrypt prior to 5.3.7). However, after seeing the new password hashes in the database of an installation running on Red Hat Enterprise Linux 6.4 with PHP 5.3.3, and reading through all of the comments here, it seems that the code actually tests to see if PHP's bcrypt is properly patched to 5.3.7 functionality while not necessarily running 5.3.7. If my understanding is correct, could the testing instructions, description, or documentation be updated to indicate that this new functionality might affect versions of PHP with the bcrypt patch backported?

          That aside, I really appreciate the effort made to strengthen the password hashes that Moodle uses. Thanks!

          Show
          Ryan Foster added a comment - From the testing instructions, it seemed to me initially that the new password hashing would only affect installations with PHP 5.3.7 or higher (due to a bug in PHP's implementation of bcrypt prior to 5.3.7). However, after seeing the new password hashes in the database of an installation running on Red Hat Enterprise Linux 6.4 with PHP 5.3.3, and reading through all of the comments here, it seems that the code actually tests to see if PHP's bcrypt is properly patched to 5.3.7 functionality while not necessarily running 5.3.7. If my understanding is correct, could the testing instructions, description, or documentation be updated to indicate that this new functionality might affect versions of PHP with the bcrypt patch backported? That aside, I really appreciate the effort made to strengthen the password hashes that Moodle uses. Thanks!
          Hide
          Simon Coggins added a comment - - edited

          Good point Ryan, thanks for the note.

          I've updated the testing instructions and documentation for 2.5 and 2.6 here:

          http://docs.moodle.org/26/en/Password_salting

          Show
          Simon Coggins added a comment - - edited Good point Ryan, thanks for the note. I've updated the testing instructions and documentation for 2.5 and 2.6 here: http://docs.moodle.org/26/en/Password_salting

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              19 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: