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

          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