Password storage / Use password_hash from php5.5 for password storage

[ehuggett]

phplist currently uses unsalted single round hashes for admin and user passwords alike (with the option to disable hashing entirely for user passwords so it can be sent to them in an email instead of a change password link if i recall correctly).

I propose the following:

1. Replacing all password handling with password_hash() and password_verify() functions, This will increase the minimum version requirement to php5.5. 1.1 If this minimum version requirement is not acceptable then including the userland implementation would lower the requirement to "PHP >= 5.3.7 OR a version that has the $2y fix backported into it (such as RedHat provides)".
2. Use the constant PASSWORD_DEFAULT in the phplist configuration. This means phplist will automatically use improved algorithms (as the PHP constant changes) without configuration or code changes, currently Blowfish with a work factor of 10.
3. deprecate and immediately remove plaintext password storage option for user passwords.
4. deprecate and remove at a later date (2 releases?) backwards comparability with passwords stored in the current scheme.

I have started work in this direction for admin passwords here. So far this handles upgrading old/current password hashes to the proposed scheme (and upgrading passwords in the proposed scheme to the new value of PASSWORD_HASH if/when it changes in a future php release), but the "forgot password" options still write the old/current password hashes into the database and i have not touched user passwords.

I would suggest we remove the backwards compatibility for old hashes ASAP, if the user has not logged in when both are supported they can use the forgot password option to reset their password (overwriting the unsupported hash).

[michield]

I think it's great to improve on this. Are you going to PR it when you've finished? I do think there may be a need to keep a <5.5 option, for some folks who cannot upgrade their PHP.

User/subscriber password functionality is hardly used. They effectively login with a token.

What happens if the algo changes in a PHP Upgrade? That seems to mean all passwords become invalidated. As long as we communicate that clearly, that should be fine, but it may cause some confusion if not handled correctly.

[ehuggett]

The constant PASSWORDDEFAULT changing in newer versions of PHP should not cause any problems as the algorithm to use for the password comparison during login is stored with the password in the database (The proposed changes will result the password [hash/salt/rounds] being stored in [Crypt](https://en.wikipedia.org/wiki/Crypt(C)) format. ).

Downgrading PHP versions might be more interesting/'broken' for users who have logged in while the installation was running under a newer version with a different value for PASSWORD_DEFAULT as their stored password hash would have been upgraded to it when they logged in. Assumption: PASSWORD_DEFAULT won't be changed to use an algorithm introduced in the same PHP release therefore most 'minor' version downgrades will not be an issue.

note: The database schema might need updating, from http://php.net/manual/en/password.constants.php "if you use PASSWORD_DEFAULT you should store the resulting hash in a way that can store more than 60 characters (255 is the recomended width)"

[bramley]

Regarding point 3, what is the reason for removing the code for upgrading passwords stored as plain-text? That will affect anyone wanting to upgrade from phplist 2. To be clear, storing passwords in plain text is not an option, it is just the way it was done in phplist 2. When someone upgrades to phplist 3 the passwords will be stored in the current scheme.

Similarly on point 4, removing the code for upgrading passwords stored in the current scheme will affect anyone wanting to upgrade from phplist > 2 releases old.

[ehuggett]

To be honest, i had not properly considered the implications for installations not being kept up to date or what happens when site admins skip versions (possibly because i never really took advantage of that when phpList was part of my job).

I'm looking to sacrifice what i perceived to be unnecessary backwards compatibility to make the login process as simple as possible, as this leaves less room for error and makes it easier to audit in the future.

It should be noted that the deleted block of code is making 3.x users potentially susceptible to something like the pass-the-hash attack IF the value of HASH_ALGO is changed to a hash function with a longer output (for example, md5 to sha256) AND an admin user has not changed their password since this was done THEN the old password hash is now considered to be a plaintext password by validateLogin() and can be used as such in the login form.

Its not going to bring the sky down, but it shouldn't ever be possible for an attacker to login as a user by obtaining their password hash (ie SQL injection, a lost backup, etc)

So with that in mind,

on point 3 I still think its time to remove support for plaintext passwords in the database from validateLogin.

It could be moved into upgrade.php, it looks like it would be possible to make sure it only ever executes once by targetting the release version this change would be included in (which still works if the admin skips that version?).

on point 4 2 releases does now seem quite short now, can anyone give me an idea of how many installations would be affected if we waited longer? (ie how many sites upgrade within 3 days vs 3 months vs 1 year)

doing the "right thing" (storing passwords as best we can) starts to get complicated while maintaining long term backwards comparability (even more-so if we cannot assume all the password hashes in the database are not HASH_ALGO).

Basically, we would have to put the current password hashes through password_hash() during the upgrade and somehow mark those users as needing special handling the next time they login. something like

for x in $userids:
   $db_password = mysql(get password for user x)
   $compat_hash = password_hash($db_password, PASSWORD_DEFAULT)
   mysql(update user x set password to $compat_hash and special_handling to the best guess at hash ie sha256 )
done

To check the password they have supplied when they do login we would have do to something like

$username = id of the user
$password = POST['password']

$user = mysql(get record by username)

$password_hash = $user[password]
$special_handing =$user[special_handling]

if $special_handing is md5/sha256/etc
   $password = hash($special_handling, $password)

if password_verify($password,$password_hash)
   if $special_handing OR password_needs_rehash($password_hash, PASSWORD_DEFAULT)
      mysql(update password to password_hash($password,PASSWORD_DEFAULT) and special_handling to NULL)
   return LOGIN SUCCESS 
else
   return LOGIN FAILED

By doing it this way we significantly increase the difficulty of brute forcing the legacy password hashes if they really must be stored long term. To put some numbers on it, according to these Hashcat Benchmarks 8x Nvidia GTX 1080's can do around 23 012 100 000 SHA256 hashes per second (password attempts) but only 105 700 bycrpt hashes a second even with a work factor of 5 (i have gone with PHP's default of 10, higher is better / requires more work / takes longer).

alternatively, would either of these approaches address your concerns?

• Could we just ask the site administrator if they wish to send those users a password reset email during the upgrade and/or wipe the old hashes? If we leave the comparability in for a few releases, it will only have to prompt admins who still have users with old hashes when they upgrade to the release that removes it (but it will prompt every admin who skips all the versions that supported both schemes)

• The version in the initial release supporting both could become a plugin when its removed allowing admins who didn't upgrade fast enough/still have users with old hashes to avoid having to ask users to update their password if they feel its necessary / justifiable in their environment?

[bramley]

Edward, thanks for the explanation, the motivation is a bit clearer to me now. If I understand correctly the concerns are a problem with how plain text passwords are handled, and the relative weakness of md5 and sha256 hashes. I think that Michiel and Sam will need to comment on what approach they want you to take.

One comment about the handling of plain text passwords. I think that the test of the password length that allows plain text passwords is incorrect. That should be allowed only when upgrading from phplist 2.x when the code has been upgraded but the database still refers to 2.x. So it should be executed only once before the database is upgraded, and never afterwards. Testing the password length instead of the database version has the right effect but as you said potentially allows a hash to be used instead of the password.

[ehuggett]

There are no conditions that i am aware of which can be used differentiate between plaintext passwords and hashes of passwords with 100% accuracy. For example, the md5 checksum of the plaintext password password is 5f4dcc3b5aa765d61d8327deb882cf99, which itself would also be a valid plaintext password.

I would dismiss this as almost completely irrelevant when trying to maintain backwards compatibility with or upgrade plaintext passwords, but there might be users with password generators / managers that create passwords that look like checksums, and there are certainly some people who use utilities like md5(etc) and a "master password" to avoid having to remember or keep on record a unique password for every website, it looks something like hash("masterpassword" + $domain_name) (not something i would recommend doing by the way).

More worryingly, and i was quite surprised when i realised this, it would also be rather unfortunate if someone did actually use 5f4dcc3b5aa765d61d8327deb882cf99 as their password (or the hash of any common dictionary word or common password) and it was stored in the database without being hashed first, as even the very best we can do in the current situation with the column containing a mix of plaintext and hashes will identify it as a md5 hash, which means their password has been changed to password (or a common dictionary word) since(/as soon as) the system was(/is) upgraded to a version that uses md5 hashing for passwords until that user has their password reset.

In both cases outlined above, the user would be telling the truth when they tell the system administrator they did not forget their password 😄

Its obviously safe to assume all the passwords are plaintext when upgrading a database version that would never have contained hashed passwords, but its not clear to me what the correct thing to do is after that point as i can't see a perfect behaviour (and i suspect there is none).

The same will be true for the subscriber passwords, if anyone has chosen to set one.

[ehuggett]

What versions of php are currently supported / known to work and of those which versions does this need to be compatible with? (all of them?)

[samtuke]

Officially PHP 5.4 plus. All newer versions are used for running basic tests via Travis.

https://resources.phplist.com/system/start https://github.com/phpList/phplist3/blob/master/.travis.yml

[bramley]

Don't forget this pull request to continue supporting php 5.3.3 - https://github.com/phpList/phplist3/pull/164

[samtuke]

@bramley Good point -- docs updated: https://resources.phplist.com/system/start

[ehuggett]

Could be a problem for this?

1.1 If this minimum version requirement is not acceptable then including the userland implementation would lower the requirement to "PHP >= 5.3.7 OR a version that has the $2y fix backported into it (such as RedHat provides)".

https://github.com/ircmaxell/password_compat#requirements

Requirements

This library requires PHP >= 5.3.7 OR a version that has the $2y fix backported into it (such as RedHat provides). Note that Debian's 5.3.3 version is NOT supported.

The runtime checks have been removed due to this version issue. To see if password_compat is available for your system, run the included version-test.php. If it outputs "Pass", you can safely use the library. If not, you cannot.

If you attempt to use password-compat on an unsupported version, attempts to create or verify hashes will return false. You have been warned!

The reason for this is that PHP prior to 5.3.7 contains a security issue with its BCRYPT implementation. Therefore, it's highly recommended that you upgrade to a newer version of PHP prior to using this layer.

While the Author of the pull commit would in theory not be affected (assuming EL6 has the $2y fix) attempting to state that php5.3.3 is supported in the documentation would require some explanation if this library was in use.

It looks like you will eventually want/need the library anyway? If password_compat was included in the next release its check function could be to start advising any users on a system that fails it before it becomes a problem (perhaps buried in the admin area somewhere for now), but I'm not sure if i would claim anything under 5.3.7 is officially "supported" just to be on the safe side once its actually in use for passwords

[samtuke]

Before the recent PR, PHP 5.4 was the minimum version for phpList (and previously announced), and we can easily make it so again.

[ehuggett]

• I have included password_compat
• added setPassword to phpListAdminAuthentication (this brake installations with a custom admin_auth plugin?)
• password reset now uses setPassword
• the config check now uses validateLogin() to check the password is no longer "phplist"

I took the opportunity to play with phplist-docker, so its only been tested under PHP 5.6.30 so far

[michield]

That's brilliant. Is is ready for a PR, or are you still tweaking it? By the way, yes, point 3, I can work on the removal of the plain text password option (and move it to upgrade instead). That was an undocumented feature, that should be retired.

[ehuggett]

I do have a habit of rebasing (rewriting history) my own branches with master to bring them up to date and i plan to squash all of the commits before the PR, either of which might cause you issues/hassle if you pulled my branch now to start working on the upgrade but I would say its just about ready for admin passwords?

I still have some minor changes to what i have written any many "todo check this" comments in local files to come back to. I have tried to find every location where these changes might be relevant i.e the old function, encryptPassword, is also being used for password verification (and it looks like hash() has been used directly a few times).

The largest remaining issue is I don't currently have anything setup to test under older (or newer) php versions, but it works in theory (at least until anyone tries it!). I want to try and do this with docker just for the sake of it which will add some delays (perhaps submitting a PR for phplist-docker if what i end up doing is clean enough to make it a configurable "dev" feature).

I also still need to update config/config_extended

1. hash_algo no longer controls password hashing
2. there are only 2 options for password hashing, PASSWORD_BCRYPT and PASSWORD_DEAFULT which are currently equivalent, but a config option would allow 2.1 manually upgrading to a newer scheme when introduced but before PASSWORD_DEFAULT is changed in PHP 2.2 force PASSWORD_BYCRYPT to remain in use after PASSWORD_DEFAULT changes (would be an issue for anyone directly integrating with phplist if their crypt() implementation does not support whatever the new scheme is yet)

for upgrades: I've looked into the md5 and sha256 schemes used in crypt() just to confirm its not possible to "convert" the existing hashes into a format that will be "directly" useful with password_hash() even with a null/empty salt, so the compatibility fallback to the current scheme would be needed for at least 1 release. Its a dead end because we can't get past step 4 of 22 on https://www.akkadia.org/drepper/SHA-crypt.txt (or step 3 for md5, due to the salt always starting "$1$" so it can never be null).

[ehuggett]

To extended_config, I'm adding

• PHPLIST_PASSWORD_ALGO, default: PASSWORD_DEFAULT (which currently selects Bcrypt, but may change in future php versions)
• PHPLIST_PASSWORD_BCRYPT_COST, default: undefined (password_hash will use the php default of 10 if an array of options is not specified). It did not seem sensible to have a 'generic' cost option, as future algorithms may be significantly more expensive than Bcrypt with the same value for 'cost' (this is an assumption)

And removing from config.php

• HASH_ALGO (and marking it as deprecated in config_extended.php)

We should consider calculating an appropriate value for the bcrypt cost parameter during installation/upgrade and add it to the config check. But I think it would be OK to release without this improvement

# http://php.net/manual/en/function.password-hash.php#example-980

<?php
/**
 * This code will benchmark your server to determine how high of a cost you can
 * afford. You want to set the highest cost that you can without slowing down
 * you server too much. 8-10 is a good baseline, and more is good if your servers
 * are fast enough. The code below aims for ≤ 50 milliseconds stretching time,
 * which is a good baseline for systems handling interactive logins.
 */
$timeTarget = 0.05; // 50 milliseconds 

$cost = 8;
do {
    $cost++;
    $start = microtime(true);
    password_hash("test", PASSWORD_BCRYPT, ["cost" => $cost]);
    $end = microtime(true);
} while (($end - $start) < $timeTarget);

echo "Appropriate Cost Found: " . $cost . "\n";
?>

• I would start with cost = 10 instead of 8 as above, so as not to use a cost lower than the default
• I think we can get away with adding more than 50ms of delay to authentication, perhaps 100ms? (or more, as a side effect this is the closest thing phpList currently has to rate limiting authentication attempts but its a very 'expensive' way of implementing it)

[michield]

I think 100ms is fine as a delay. Depends a bit how much it is delayed by other parts of the process.

If we default PHPLIST_PASSWORD_ALGO to PASSWORD_DEFAULT what happens if PASSWORD_DEFAULT changes? Doesn't that mean you get locked out?

Maybe we should store the ALGO used in the DB and fix it, regardless of underlying defaults and then allow updating it with a command (eg delete the DB value, which will re-initialise)

[ehuggett]

Doesn't that mean you get locked out?

The first login after a change of cost or algorithm will be verified using a hash produced with the old settings, if successful the plaintext password provided by the user to login will be hashed with the new settings and the user record updated.

https://github.com/ehuggett/phplist3/blob/passwordhashing/public_html/lists/admin/phpListAdminAuthentication.php#L39

(so no, nobody gets locked out)

If we default PHPLIST_PASSWORD_ALGO to PASSWORD_DEFAULT what happens if PASSWORD_DEFAULT changes?

Then by design the default value of PHPLIST_PASSWORD_ALGO changes. I introduced the config option to allow the administrator to override this behaviour, mainly to allow newly introduced ciphers to be used before the PHP projects recommends them via PASSWORD_DEFAULT.

[bramley]

Just to clarify

Maybe we should store the ALGO used in the DB

The result of password_hash() includes the algorithm used. It's the $2y$ part in this example http://php.net/manual/en/function.password-hash.php#example-977

[michield]

The first login after a change of cost or algorithm will be verified using a hash produced with the old settings, if successful the plaintext password provided by the user to login will be hashed with the new settings and the user record updated.

Nice

[michield]

The result of password_hash() includes the algorithm used.

Ah, thanks

[ehuggett]

FYI argon2i was added to php7.2 in https://github.com/php/php-src/pull/1997/files, but PASSWORD_DEFAULT remains unchanged.

It seems I can't use constants to store part of my configuration under the supported PHP versions, I want it to be possible to configure parameters for hashing algorithms that don't exist (yet) and I think this requires an array of arrays? ie

$phplist_password_parameters = array(

    PASSWORD_BCRYPT => array(
            'cost' => PASSWORD_BCRYPT_DEFAULT_COST,
        ),
// Uncomment the following block if you are running on php7.2+ and wish to configure Argon2i
/*
*    PASSWORD_ARGON2I => array(
*            'memory_cost' => PASSWORD_ARGON2_DEFAULT_MEMORY_COST,
*            'time_cost' => PASSWORD_ARGON2_DEFAULT_TIME_COST,
*            'threads' => PASSWORD_ARGON2_DEFAULT_THREADS,
*        ),
*/
);

( define() will accept an array in php => 7, but this can be done in php => 5.6 using const with some restrictions)

The alternatives i considered

• Use the database for config, I would have to store and evaluate constant names as I don't think there is a guarantee that their values won't change (they are not documented)
• Use a constant but write it in such a way that the config does not work on php <= 5.6, but it doesn't brake either (and always uses the default cost for Bcrypt).

Main changes

• PHPLIST_PASSWORD_ALGO has become PHPLIST_PASSWORD
• PHPLIST_PASSWORD_BCRYPT_COST no longer exists, replaced with the global above
• rebased on master

TODO / advice please

1. How should these errors be handled? 1.1 https://github.com/ehuggett/phplist3/blob/b331779b62e0ff43f58abc7cd13e3cb665f14fd9/public_html/lists/admin/login.php#L79 1.2 https://github.com/ehuggett/phplist3/blob/b331779b62e0ff43f58abc7cd13e3cb665f14fd9/public_html/lists/admin/phpListAdminAuthentication.php#L99
2. How long are the old password hashes going to be supported? https://github.com/ehuggett/phplist3/blob/passwordhashing/public_html/lists/admin/phpListAdminAuthentication.php#L53
3. I have only run this under the phplist-docker images, so it needs to be tested on older (and newer) php versions
4. general sanity check / readover / are potential errors being caught etc

PS I did write some simple logging for password hashing time (warnings if too fast or slow) but I've removed it as I'm not sure how well it will age.

[ryancwalsh]

Wow, this is an open issue from 2+ years ago?

How are passwords handled in the most recent version of PhpList (I've never used PhpList before); is everything secure and using modern approaches such as https://www.php.net/manual/en/faq.passwords.php?