Weak password storage

[meg23]

From manico.james@gmail.com on May 06, 2011 16:11:15

The ESAPI reference implementation contains a weak salting mechanism for password storage. (Currently uses a known value, the account name) It also does not implement or encourage salt isolation.

Original issue: http://code.google.com/p/owasp-esapi-java/issues/detail?id=224

[meg23]

From manico.james@gmail.com on May 28, 2012 20:19:44

Owner: chrisisbeef

[meg23]

From michalis...@gmail.com on June 26, 2012 13:46:06

I have a very simple suggestion (which I'm currently using). Double salting :

Add this line in the Authenicator.java

private static final String MASTERSALT = new String(ESAPI.securityConfiguration().getMasterSalt());

Replace hashPassword in implementation (e.g. FileBasedAuthentication) with

public String hashPassword(String password, String accountName) throws EncryptionException {
String salt = ESAPI.encryptor().hash(accountName.toLowerCase(), (String)MASTERSALT);
return ESAPI.encryptor().hash(password, salt);
}

The good thing with this method is that MasterSalt is available on ESAPI.properties, i.e. in a different place than the user database (in my case the user database is a mysql table).

[meg23]

From j...@manico.net on March 28, 2014 05:46:59

This entire password storage mechanism in ESAPI is bunk. I suggest the move to PBKDF2, a more formal Key Derivation Function as well as very strong per-user random salting.

[kwwall]

I am dropping the priority of this issue from Critical to High since if it has not been fixed since 2011 and the last comment on it is from 2014 (before this comment, of course), so it seems obvious that no one thinks it is all that important enough to really be Critical.

Truth be told, NO ONE should be using the default reference implementation of ESAPI's Authenticator (i.e., FileBasedAuthenticator) for many of other reasons, mainly because storing user accounts in an unsorted flat file does not scale well at an enterprise level. (Sure, *nix systems do this with /etc/passwd and /etc/shadow, but they rarely have more than 10k users or so [yes; I know there are exceptions, so save your argument for another day!] and those users are not constantly authenticating as they are on web applications and new users are not constantly being added, etc.)

The major thing about changing a password hashing algorithm however, is for those using the previous one, HOW do you transition to the new presumably more secure algorithm? Unless one is just willing to force a password reset of all your users at once, the old algorithm is still needed until all the old hashes are converted to the new format. However, since the new algorithm is needed as well the old one (until all old hashes have been converted to the new format), we can't call them both the same thing (or, more specifically, they both cannot have the same exact method signature). It is not an easy problem to address and an approach that is acceptable to one company (e.g., a forced password reset of all users via the user's security questions/answers) is likely to NOT be acceptable to all companies.

Jim Manico is absolutely right about the entire password storage mechanism offered as ESAPI's reference implementation being bunk. But I at this point, his suggested PBKDF2 is probably not best approach either. (Bcrypt, scrypt, and the newer Argon2 are considered stronger for traditional secure hashes, but a lot depends on what your specific threat model is.) Clearly, if ESAPI is to offer such a user account / password storage mechanism in a reference implementation, many things need to be considered here. First is the password hashing algorithm itself. Developers should be able to choose from a select # of available algorithms such as scrypt, bcrypt, argon2, PBKDF2, and even plug-in custom implementations of their own. Secondly, random salts should be used and stored as part of the hash itself; only the length of the salt, in bits, is something that developers should be allowed to set and there should be some minimum imposed length for that (to be debated; just not now). Lastly the # of hash iterations should also be specifiable by the developer (also with some pre-agreed, but TBD, minimum number iteration # as well).

We are a LONG way from that. In the meantime, so as not to let the perfect become the enemy of the good here, my plan for this is to just add warnings in the class Javadoc for FileBasedAuthenticator as well as for Authenticator.hashPassword() and Authenticator.createUser() methods specifically.

But let's just not just cry wolf and claim this is a Critical priority when no one should be using ESAPI's reference implementation of Authenticator in the first place.

[kwwall]

Looking at this implementation more closely, the accountName is combined with the (presumably?) secret Encryptor.MasterSalt from the ESAPI.properties file. SHA-512 is used as the hash algorithm, but only 1024 iterations are used which is way too few. So, still not as good as it should be, but better than I expected, at least if Encryptor.MasterSalt is actually kept secret.

[kwwall]

Keep in mind that any changes that we make in any ESAPI 2.x release need to be backward compatible, so that minimally would mean that we would have to keep the old method and its settings and provide a means to increase specify the # of iterations or provide a unique random salt.

We will see if we can get this into the 2.3 release, but if not, at least not in the Javadoc some sort of warning if we can't figure out an easier way to make it more or less backward compatible with earlier 2.x versions.

[kwwall]

Note that implementing issue #189 would make this unnecessary.