Insecure User Password Storage

[ghost]

SI uses MD5 hashing to store user passwords in database as per save.php#L46. Not only is MD5 for password storage bad practice as MD5 is considered extremely insecure, but SI does not even add a salt to it, which makes it even worse. See this post for a concise explanation on how to securely store a password.

SI should be using a secure hashing library instead. PHP 5.5 has introduced password hashing functions which should be used. In the case a user does not have that version they can use the compatibility library made by the author of the PHP function which can be found here.

Since such a change, which is definitely needed would break existing passwords, there would have to be a conversion done. If a login is successful using the MD5 stored password then that submitted password should be hashed then using password_hash() and then replace the MD5 hash.

Nice video : https://www.youtube.com/watch?v=8ZtInClXe1Q

[justinkelly]

thanks @kyRAD - great suggestion - happy to have this implemented in SI - just needs to be able to migrate old user passwords

[apmuthu]

http://stackoverflow.com/questions/12459896/password-compat-for-older-php-version

https://github.com/ircmaxell/PHP-PasswordLib
http://www.openwall.com/phpass/

----- Original Message -----
From: Kyra ツ
To: simpleinvoices/simpleinvoices
Sent: Friday, November 22, 2013 7:23 AM
Subject: [simpleinvoices] Insecure User Password Storage (#133)

SI uses MD5 hashing to store user passwords in database as per save.php#L46. Not only is MD5 for password storage bad practice as MD5 is considered extremely insecure, but SI does not even add a salt to it, which makes it even worse. See this post for a concise explanation on how to securely store a password.

SI should be using a secure hashing library instead. PHP 5.5 has introduced password hashing functions which should be used. In the case a user does not have that version they can use the compatibility library made by the author of the PHP function which can be found here.

Since such a change, which is definitely needed would break existing passwords, there would have to be a conversion done. If a login is successful using the MD5 stored password then that submitted password should be hashed then using password_hash() and then replace the MD5 hash.

—
Reply to this email directly or view it on GitHub.

[apmuthu]

Is this a possible fix independant of any library:

http://stackoverflow.com/questions/14085421/implement-password-compat

----- Original Message -----
From: Kyra ツ
To: simpleinvoices/simpleinvoices
Sent: Friday, November 22, 2013 7:23 AM
Subject: [simpleinvoices] Insecure User Password Storage (#133)

SI uses MD5 hashing to store user passwords in database as per save.php#L46. Not only is MD5 for password storage bad practice as MD5 is considered extremely insecure, but SI does not even add a salt to it, which makes it even worse. See this post for a concise explanation on how to securely store a password.

SI should be using a secure hashing library instead. PHP 5.5 has introduced password hashing functions which should be used. In the case a user does not have that version they can use the compatibility library made by the author of the PHP function which can be found here.

Since such a change, which is definitely needed would break existing passwords, there would have to be a conversion done. If a login is successful using the MD5 stored password then that submitted password should be hashed then using password_hash() and then replace the MD5 hash.

—
Reply to this email directly or view it on GitHub.

[apmuthu]

http://www.phptherightway.com/#password_hashing

----- Original Message -----
From: Kyra ツ
To: simpleinvoices/simpleinvoices
Sent: Friday, November 22, 2013 7:23 AM
Subject: [simpleinvoices] Insecure User Password Storage (#133)

SI uses MD5 hashing to store user passwords in database as per save.php#L46. Not only is MD5 for password storage bad practice as MD5 is considered extremely insecure, but SI does not even add a salt to it, which makes it even worse. See this post for a concise explanation on how to securely store a password.

SI should be using a secure hashing library instead. PHP 5.5 has introduced password hashing functions which should be used. In the case a user does not have that version they can use the compatibility library made by the author of the PHP function which can be found here.

Since such a change, which is definitely needed would break existing passwords, there would have to be a conversion done. If a login is successful using the MD5 stored password then that submitted password should be hashed then using password_hash() and then replace the MD5 hash.

—
Reply to this email directly or view it on GitHub.

[ghost]

@apmuthu use the library I linked to above https://github.com/ircmaxell/password_compat . It's made by the author (@ircmaxell) of the PHP function introduced in PHP 5.5, and is compatible with older versions. It's the recommended one to use as it has been vouched for it's security.

This library will allow you to use the PHP 5.5 functions and once you drop older support, you just need to remove the library file and your code will already be compatible. Under no circumstance should you ever create your own implementation, or use one that has not been peer reviewed.

Btw the post you are asking about above is not necessarily independent of any library, it is using either using PHP 5.5+ or has a library included. Those functions would not work otherwise. You should read about the PHP password hashing functions, I linked to them in the original post.

[justinkelly]

thanks - if we swap to new format - can we migrate old passwords?

On Fri, Nov 22, 2013 at 5:23 PM, Kyra ツ notifications@github.com wrote:

@apmuthu https://github.com/apmuthu use the library I linked to above
https://github.com/ircmaxell/password_compat . It's made by the author (
@ircmaxell https://github.com/ircmaxell) of the PHP function introduced
in PHP 5.5, and is compatible with older versions. It's the recommended one
to use as it has been vouched for it's security.

This library will allow you to use the PHP 5.5 functions and once you drop
older support, you just need to remove the library file and your code will
already be compatible. Under no circumstance should you ever create your
own implementation, or use one that has not been peer reviewed.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/133#issuecomment-29051371
.

[ghost]

@justinkelly the only way to migrate old passwords is at login. You need the original password to hash, and the only time you have access to that is when a user logs in successfully with the old MD5 hash. So this is what would be done:

1. User attempts to login, check to see if a MD5 hash is present in database for that user
2. If there is one then MD5 their password and compare to stored MD5 as usual (If the comparison works then we know they used a valid password)
3. Pass that correct password to the new password hashing function and replace the MD5 hash in database
4. Next time user logs in there will not be an MD5 hash present, so login using password_verify() instead

That's pretty much he just of how the password "migration" would go.

[justinkelly]

@kyRAD thanks- this is prob the best migration strategy - anyone interested in coding this?

[ghost]

@justinkelly I can do it. I already have an understanding on how to do it, and how to do it securely so I'll just go ahead and do it. I'll see if I can get it made within the next two weeks or so as time permits. I'll post the code when done so can be peer reviewed.

[justinkelly]

@kyRAD awesome - thanks so much!!!!!!!!!!!!!!!!!!!!!

[michelinium]

I just found this recommendation about migration on http://php.vrana.cz/ukladani-hesel-bezpecne.php (Czech version only)

Tried to translate:
The correct solution is a new algorithm applied to the original hash, saved passwords therefore will be a result of function "scrypt( cost, salt, md5( password ) )". Convert all existing users in a one-time operation and for next password authentication use the sequence of functions. If in the future you decide to use a different algorithm, simply add it to the sequence.

Scrypt should be part of Zend Framework since 2.1, more here:
http://framework.zend.com/manual/2.1/en/modules/zend.crypt.key.derivation.html#scrypt-adapter
https://github.com/zendframework/Component_ZendCrypt/blob/master/Key/Derivation/Scrypt.php
I see SI is using old 1.1 version, so it would need some more work to implement it...

[ircmaxell]

I HIGHLY recommend against using the built-in implementation of scrypt from ZF2. I don't recommend scrypt in the first place for general password storage, but in the event that you do use it, do NOT use the PHP implementation of it. It is several thousand times slower than the C implementation (due to implementing crypto primitives in PHP), and therefore would leave you at significant risk of under-protection.

Instead, you can adapt the algorithm to use bcrypt instead:

// Run this for every user, and replace their md5'd password with this result
function migrate($md5_password) {
    $hash = 'md5_' . password_hash($md5_password, PASSWORD_BCRYPT);
    updateUsersPasswordHash($hash);
}

// Validate with this logic:
function validateLogin($password, $hash, array $hashOptions) {
    if (substr($hash, 0, 4) == "md5_") {
        // legacy password
        if (password_verify(md5($password), substr($hash, 4))) {
            // successful match, re-hash and update
            updateUsersPasswordHash(
                password_hash($password, PASSWORD_BCRYPT, $hashOptions)
            );
            return true;
        }
        return false;
    } elseif (password_verify($password, $hash)) {
        if (password_needs_rehash($hash, PASSWORD_BCRYPT, $hashOptions)) {
            updateUsersPasswordHash(
                password_hash($password, PASSWORD_BCRYPT, $hashOptions)
            );
        }
        return true;
    }
}

That's the basic logic involved, and will be significantly more secure than going with scrypt (and by migrating, you're not stuck with a legacy algorithm for all time)...

[michelinium]

Ok, I understand that Scrypt PHP library would be too slow and it could be strange to wait more than second to respond during login. In quoted article by Jakub Vrána there are mentioned three so called slow hash functions: Bcrypt, PBKDF2 and Scrypt. I just translated example with the last one, but it would also work with others.

As I understand it the key idea of the article is:
It’s not good to leave stored passwords hashed only by MD5 without salt & slower hash function and wait for next user login to get clear password to rehash it by more secure way. It could take days, months or year. Until that someone can still try brute force attack or stole database and crack it with app like oclHashcat (http://hashcat.net/oclhashcat/).
So the solution is to hash already hashed string. I guess last sentence "you're not stuck with a legacy algorithm for all time" is not about md5() as it probably will not disappear from PHP anytime soon (or later).

Anyway I don’t see it as the biggest security issue for Simple Invoices. What is an average number of users? Somewhere from one to five? I’m not sure, but to me there is bigger issue that SI isn’t prepared for two installations on the same domain. If you successfully log in to first folder you can then switch address and work with second installation. No username or password check even happen.

[justinkelly]

@michelinium can we migrate from md5 to bcrypt

bcrypt seems to be the favoured method around at the moment