Thanks for the proposal.

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

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

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

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?

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?

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

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

[removing security flag so more people can see it]

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.

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.

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!

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.

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

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.

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.

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

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?

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)

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

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.

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?

Hi Hubert,

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

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

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.

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

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.

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.

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

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.

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.

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 !

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

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!

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

Sam, could you comment on the MUC cache?

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

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

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

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.

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.

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

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.

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

Done. Thanks

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!

Rebased against latest master.

Testing user upload with 1000 users took 12.44 seconds.

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.

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

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)

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.

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.

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

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.

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.

Created MDL-37963 for that.

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.

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

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.

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.

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.

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

Thanks Apu for your feedback.

I'm passing this test.

Test passed.

Congratulations this fix has been added to Moodle!

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

Thanks!

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

Are there any debug messages in your apache log Fred ?

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

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

Raised MDL-38102.

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 )

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!

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.

Thanks!

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.

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?

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

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

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!

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