Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38102

New password hashing method fails

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              fred Frédéric Massart added a comment -

              Raising priority to Critical.

              Show
              fred Frédéric Massart added a comment - Raising priority to Critical.
              Hide
              poltawski 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
              poltawski 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
              simoncoggins 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
              simoncoggins 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
              simoncoggins 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
              simoncoggins 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
              salvetore Michael de Raadt added a comment -

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

              Show
              salvetore Michael de Raadt added a comment - Raising to a blocker and setting a security level (although this might be removed later).
              Hide
              fred 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
              fred 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
              poltawski 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
              poltawski 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
              fred 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
              fred 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - In which case, why was it caching a bad result..
              Hide
              simoncoggins 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
              simoncoggins 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
              fred Frédéric Massart added a comment -

              (It's Fred, but OK )

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

              Sorry, Fred!

              Show
              simoncoggins Simon Coggins added a comment - Sorry, Fred!
              Hide
              simoncoggins 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
              simoncoggins 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
              poltawski Dan Poltawski added a comment -

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

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

              (To test for regressions)

              Show
              poltawski Dan Poltawski added a comment - (To test for regressions)
              Hide
              simoncoggins 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
              simoncoggins 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
              salvetore 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
              salvetore 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
              simoncoggins 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
              simoncoggins 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
              poltawski Dan Poltawski added a comment -

              Sending for integration

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

              Thanks Simon - this has been integrated to master.

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

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

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

              Yes, thanks Simon.

              Show
              salvetore Michael de Raadt added a comment - Yes, thanks Simon.
              Hide
              salvetore 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
              salvetore 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
              salvetore Michael de Raadt added a comment -

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

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

              I agree it's not a security issue.

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

              I also agree (not a security issue).

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

              Thanks, all.

              This won't be reported in the security notices.

              Show
              salvetore Michael de Raadt added a comment - Thanks, all. This won't be reported in the security notices.
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    14/May/13