Moodle
  1. Moodle
  2. MDL-38102

New password hashing method fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Authentication
    • Labels:
    • Testing Instructions:
      Hide

      Not that easy, I guess you could:

      1. Add the line:

      return null;

      as the first line of the password_hash() function in lib/password_compat/lib/password.php

      2. Create a new user

      Existing behaviour

      The creation will fail with the error:

      Error writing to database
      
      More information about this error
      
      Debug info: Column 'password' cannot be null
      INSERT INTO mdl_user (username,auth,suspended,firstname,lastname,email,maildisplay,mailformat,maildigest,autosubscribe,trackforums,htmleditor,city,country,timezone,lang,imagealt,url,icq,skype,aim,yahoo,msn,idnumber,institution,department,phone1,phone2,address,timemodified,description,descriptionformat,mnethostid,confirmed,timecreated,password) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)
      [array (
      0 => 'tests',
      1 => 'manual',
      2 => '0',
      3 => 'testets',
      4 => 'setse',
      5 => 'teses@tes.com',
      6 => '2',
      7 => '1',
      8 => '0',
      9 => '1',
      10 => '0',
      11 => '1',
      12 => 'tset',
      13 => 'AL',
      14 => '99',
      15 => 'en',
      16 => '',
      17 => '',
      18 => '',
      19 => '',
      20 => '',
      21 => '',
      22 => '',
      23 => '',
      24 => '',
      25 => '',
      26 => '',
      27 => '',
      28 => '',
      29 => 1365581538,
      30 => '',
      31 => '1',
      32 => '1',
      33 => 1,
      34 => 1365581538,
      35 => NULL,
      )]
      Error code: dmlwriteexception
      Stack trace:
      line 429 of /lib/dml/moodle_database.php: dml_write_exception thrown
      line 1089 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
      line 1131 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
      line 173 of /user/editadvanced.php: call to mysqli_native_moodle_database->insert_record()
      

      Behaviour with patch

      An exception will be thrown but instead the error will be:

      error/Failed to generate password hash.
      
      More information about this error
      
      Debug info: 
      Error code: Failed to generate password hash.
      $a contents:
      Stack trace:
      line 4487 of /lib/moodlelib.php: moodle_exception thrown
      line 172 of /user/editadvanced.php: call to hash_internal_user_password()
      
      Show
      Not that easy, I guess you could: 1. Add the line: return null ; as the first line of the password_hash() function in lib/password_compat/lib/password.php 2. Create a new user Existing behaviour The creation will fail with the error: Error writing to database More information about this error Debug info: Column 'password' cannot be null INSERT INTO mdl_user (username,auth,suspended,firstname,lastname,email,maildisplay,mailformat,maildigest,autosubscribe,trackforums,htmleditor,city,country,timezone,lang,imagealt,url,icq,skype,aim,yahoo,msn,idnumber,institution,department,phone1,phone2,address,timemodified,description,descriptionformat,mnethostid,confirmed,timecreated,password) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) [array ( 0 => 'tests', 1 => 'manual', 2 => '0', 3 => 'testets', 4 => 'setse', 5 => 'teses@tes.com', 6 => '2', 7 => '1', 8 => '0', 9 => '1', 10 => '0', 11 => '1', 12 => 'tset', 13 => 'AL', 14 => '99', 15 => 'en', 16 => '', 17 => '', 18 => '', 19 => '', 20 => '', 21 => '', 22 => '', 23 => '', 24 => '', 25 => '', 26 => '', 27 => '', 28 => '', 29 => 1365581538, 30 => '', 31 => '1', 32 => '1', 33 => 1, 34 => 1365581538, 35 => NULL, )] Error code: dmlwriteexception Stack trace: line 429 of /lib/dml/moodle_database.php: dml_write_exception thrown line 1089 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 1131 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw() line 173 of /user/editadvanced.php: call to mysqli_native_moodle_database->insert_record() Behaviour with patch An exception will be thrown but instead the error will be: error/Failed to generate password hash. More information about this error Debug info: Error code: Failed to generate password hash. $a contents: Stack trace: line 4487 of /lib/moodlelib.php: moodle_exception thrown line 172 of /user/editadvanced.php: call to hash_internal_user_password()
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
      git@github.com:totara/moodle.git
    • Pull Master Branch:
      master_MDL-38102
    • Rank:
      47913

      Description

      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.

      Worse case scenario we should probably output a debug developer notice and fallback on the MD5() to prevent websites to be broken because of an empty hash.

      I don't have the logs here any more, but I'll post more details here if I have more information.

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Raising priority to Critical.

          Show
          Frédéric Massart added a comment - Raising priority to Critical.
          Hide
          Dan Poltawski added a comment -

          If there was MUST-FIX-FOR-2.5, i'd add it here. We MUST not release with this bug present.

          Show
          Dan Poltawski added a comment - If there was MUST-FIX-FOR-2.5, i'd add it here. We MUST not release with this bug present.
          Hide
          Simon Coggins added a comment -

          Sorry to hear about this issue.

          Frederic, could you let me know what is returned by this test in your environment:

          https://github.com/ircmaxell/password_compat/blob/master/version-test.php

          If that test is returning "Pass" then the problem is with that test, and our use of it in password_compat_not_supported().

          If the test is returning "Fail" then there must be a problem with the logic of my code since it should check compatibility before trying to use crypt().

          Of course that doesn't preclude also providing a more graceful fallback if password_hash returns false/null though.

          Unfortunately this is not great timing for me (my wife is having a baby any day!) but I'll try to find some time as soon as I can to look at it further.

          Show
          Simon Coggins added a comment - Sorry to hear about this issue. Frederic, could you let me know what is returned by this test in your environment: https://github.com/ircmaxell/password_compat/blob/master/version-test.php If that test is returning "Pass" then the problem is with that test, and our use of it in password_compat_not_supported(). If the test is returning "Fail" then there must be a problem with the logic of my code since it should check compatibility before trying to use crypt(). Of course that doesn't preclude also providing a more graceful fallback if password_hash returns false/null though. Unfortunately this is not great timing for me (my wife is having a baby any day!) but I'll try to find some time as soon as I can to look at it further.
          Hide
          Simon Coggins added a comment -

          See also this thread for the background of the compatibility test:

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

          Show
          Simon Coggins added a comment - See also this thread for the background of the compatibility test: https://github.com/ircmaxell/password_compat/issues/10
          Hide
          Michael de Raadt added a comment -

          Raising to a blocker and setting a security level (although this might be removed later).

          Show
          Michael de Raadt added a comment - Raising to a blocker and setting a security level (although this might be removed later).
          Hide
          Frédéric Massart added a comment -

          I think this was a caching issue. Without really being sure of what happened and what I kept on doing on the instance, I know that after flushing the cache I could login without problems. I checked the log files and didn't find anything interesting.

          Show
          Frédéric Massart added a comment - I think this was a caching issue. Without really being sure of what happened and what I kept on doing on the instance, I know that after flushing the cache I could login without problems. I checked the log files and didn't find anything interesting.
          Hide
          Dan Poltawski added a comment -

          Hmm, Fred, were you changing an instance between versions?

          Hmm should this cache be an application cache? How do we properly detect puriging it?

          Show
          Dan Poltawski added a comment - Hmm, Fred, were you changing an instance between versions? Hmm should this cache be an application cache? How do we properly detect puriging it?
          Hide
          Frédéric Massart added a comment -

          I don't think I've done anything extensively tricky, I will run the compatibility test as soon as I can.

          Show
          Frédéric Massart added a comment - I don't think I've done anything extensively tricky, I will run the compatibility test as soon as I can.
          Hide
          Dan Poltawski added a comment -

          In which case, why was it caching a bad result..

          Show
          Dan Poltawski added a comment - In which case, why was it caching a bad result..
          Hide
          Simon Coggins added a comment -

          Caching of the password_compat_not_supported() result could potentially lead to the symptoms seen but I'm finding it difficult to figure out how it could have happened based on what Damyon described.

          You would have needed to have taken the cache from a site that does support bcrypt, and used it on a site where it wasn't supported, which doesn't seem to be what happened here.

          On the other hand if that is the problem it's possible that it may be fixed by this one:

          https://tracker.moodle.org/browse/MDL-37500

          I've also looked more at how we handle invalid values from password_hash and I've come to the conclusion we should throw and exception if either null or false is returned. There are 7 ways null can be returned but they all seem to be due to coding issues (like passing an array for the password or an invalid cost factor). I think falling back to md5 could conceal bugs.

          There is a slightly better case for falling back to md5 if password_hash returns false but again it's indicating a problem, not merely a lack of support (which we should detect and catch) so I would prefer to keep the current behavior. I've added a patch that reflects my preferred change.

          Damyon, I'd still be interested in hearing what result you're getting for that test on your system.

          Show
          Simon Coggins added a comment - Caching of the password_compat_not_supported() result could potentially lead to the symptoms seen but I'm finding it difficult to figure out how it could have happened based on what Damyon described. You would have needed to have taken the cache from a site that does support bcrypt, and used it on a site where it wasn't supported, which doesn't seem to be what happened here. On the other hand if that is the problem it's possible that it may be fixed by this one: https://tracker.moodle.org/browse/MDL-37500 I've also looked more at how we handle invalid values from password_hash and I've come to the conclusion we should throw and exception if either null or false is returned. There are 7 ways null can be returned but they all seem to be due to coding issues (like passing an array for the password or an invalid cost factor). I think falling back to md5 could conceal bugs. There is a slightly better case for falling back to md5 if password_hash returns false but again it's indicating a problem, not merely a lack of support (which we should detect and catch) so I would prefer to keep the current behavior. I've added a patch that reflects my preferred change. Damyon, I'd still be interested in hearing what result you're getting for that test on your system.
          Hide
          Frédéric Massart added a comment -

          (It's Fred, but OK )

          Show
          Frédéric Massart added a comment - (It's Fred, but OK )
          Hide
          Simon Coggins added a comment -

          Sorry, Fred!

          Show
          Simon Coggins added a comment - Sorry, Fred!
          Hide
          Simon Coggins added a comment -

          I think we are agreed that this issue was most likely caused by a caching issue during development.

          It has brought up the fact that we weren't checking the possible null return value from password_hash() so I'm submitting that fix to peer review now.

          Show
          Simon Coggins added a comment - I think we are agreed that this issue was most likely caused by a caching issue during development. It has brought up the fact that we weren't checking the possible null return value from password_hash() so I'm submitting that fix to peer review now.
          Hide
          Dan Poltawski added a comment -

          Makes sense to me simon. We just need some testing instructions for it.

          Show
          Dan Poltawski added a comment - Makes sense to me simon. We just need some testing instructions for it.
          Hide
          Dan Poltawski added a comment -

          (To test for regressions)

          Show
          Dan Poltawski added a comment - (To test for regressions)
          Hide
          Simon Coggins added a comment -

          It's a bit hard since the null return value wouldn't normally be returned, but I've done my best!

          Show
          Simon Coggins added a comment - It's a bit hard since the null return value wouldn't normally be returned, but I've done my best!
          Hide
          Michael de Raadt added a comment -

          Hi, Simon.

          Is this now ready for another peer review? You removed Dan as peer reviewer and I'm not sure why.

          Show
          Michael de Raadt added a comment - Hi, Simon. Is this now ready for another peer review? You removed Dan as peer reviewer and I'm not sure why.
          Hide
          Simon Coggins added a comment -

          Hi Michael,

          Didn't realise I removed peer reviewer, maybe I did it accidentally when adding testing instructions.

          Anyway, yes it is ready for review.

          Show
          Simon Coggins added a comment - Hi Michael, Didn't realise I removed peer reviewer, maybe I did it accidentally when adding testing instructions. Anyway, yes it is ready for review.
          Hide
          Dan Poltawski added a comment -

          Sending for integration

          Show
          Dan Poltawski added a comment - Sending for integration
          Hide
          Damyon Wiese added a comment -

          Thanks Simon - this has been integrated to master.

          Show
          Damyon Wiese added a comment - Thanks Simon - this has been integrated to master.
          Hide
          Damyon Wiese added a comment -

          Thanks Simon - test passes too. I agree this is better than trying to save a null password.

          Show
          Damyon Wiese added a comment - Thanks Simon - test passes too. I agree this is better than trying to save a null password.
          Hide
          Michael de Raadt added a comment -

          Yes, thanks Simon.

          Show
          Michael de Raadt added a comment - Yes, thanks Simon.
          Hide
          Michael de Raadt added a comment -

          I wondering if this is a security issue that needs to be reported. It looks to me that, while this is related to security, it is not a security problem in itself. That being the case, I'm inclined to not report it.

          It would be good to get some advise on this before the release.

          Show
          Michael de Raadt added a comment - I wondering if this is a security issue that needs to be reported. It looks to me that, while this is related to security, it is not a security problem in itself. That being the case, I'm inclined to not report it. It would be good to get some advise on this before the release.
          Hide
          Michael de Raadt added a comment -

          Fred has suggested to me that he doesn't think this is a security issue.

          Show
          Michael de Raadt added a comment - Fred has suggested to me that he doesn't think this is a security issue.
          Hide
          Damyon Wiese added a comment -

          I agree it's not a security issue.

          Show
          Damyon Wiese added a comment - I agree it's not a security issue.
          Hide
          Simon Coggins added a comment -

          I also agree (not a security issue).

          Show
          Simon Coggins added a comment - I also agree (not a security issue).
          Hide
          Michael de Raadt added a comment -

          Thanks, all.

          This won't be reported in the security notices.

          Show
          Michael de Raadt added a comment - Thanks, all. This won't be reported in the security notices.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: