vertx-auth doesn't use current OWASP password hashing recommendation

[sonOfRa]

This is basically a followup to #133.

The addition of what you call "nonces" to the salted SHA512 is not what the state of the art in cryptographic password hashing is. You want to use either bcrypt (https://github.com/jeremyh/jBCrypt) or the newer argon2 (https://github.com/P-H-C/phc-winner-argon2). Java bindings for argon2 are available.

The problem with just using a single round of SHA512 is that it can still be computed very quickly. Someone with access to a modern GPU like a GTX 1080 can very easily try millions of passwords per second. The point of functions like argon2 and bcrypt is to make such attacks harder to compute: A single password attempt actually takes a configurable amount of memory and CPU time to compute. That way, even a very sophisticated attacker can only run maybe a few thousand (or even less, with higher parameters) passwords per second. Adding these nonces is mostly just spreading out the problem; if an attacker acquires the nonces, then they can still mount the brute force attack as easily as without the nonces. And since in most setups, the database and the nonces will sit on the same server, a compromise of one of the two is almost always a compromise of both anyways.

Now, you might think "what if we encrypt the nonces?". But that is an equally bad idea: Now where do you store the encryption key for the nonces? So the solution to this issue is to actually migrate to a real password hashing algorithm.

[sonOfRa]

In addition to this, hash verification is done in a non-constant-time manner, which is also a security bug:

https://github.com/vert-x3/vertx-auth/blob/master/vertx-auth-jdbc/src/main/java/io/vertx/ext/auth/jdbc/impl/JDBCAuthImpl.java#L93

This uses .equals() to compare hashes, so an attacker can extract timing information about the passwords. There's MessageDigest.isEqual(), which takes two byte arrays, and does a constant time comparison, which is what you want for secure authentication.

[pmlopes]

@sonOfRa I do agree that we should fix all equality check with MessageDigest.isEqual however regarding the different algorithm it's a bit more tricky but not a complex issue. Changing the algorithm will break existing deployments BUT the algorithm is kept inside the hashing strategy implementation, so adding a extra, stronger one is easily feasable.

Up to now we did not include any crypto libraries (besides what is provided by the JDK) that is the reason why htpasswd is incomplete with a open PR regarding bcrypt.

[yawkat]

Also, the HashStrategy interface is kind of difficult to implement for some hashes - hash(String) + verify(String, String) would be much simpler than the current generateSalt + hash. Not every hash algorithm has a normal concept of a salt like that.

See the java scrypt API as an example - though in that case you could probably whip something up just with salt+hash.

[pmlopes]

OWASP recently updated their password storage recommendations where SHA512 has been dropped from the list. SHA-512 is not insecure as it is still part of FIPS but easily crackable with modern GPU/FPGAs attacks.

[sonOfRa]

Looking at the API a bit, it's going to be difficult to create a new, better API without breaking compatibility. How willing are you to make this a breaking change (that can be merged with a new major version), instead of trying to retain compatibility? I suppose one could hack in support for PBKDF2 into the current scheme without replacing the API, but other functions are going to be a lot harder; things like argon2 etc. generate the salt automatically by default, instead of letting it be specified. Also, these algorithms output a single string like $2a$05$bvIG6Nmid91Mu9RcmmWZfO5HJIMCT8riNW0hEp8f6/FuA2/mHZFpe (similar to the CRYPT format). This format contains the function (2a for bcrypt), the options (bcrypt only has one option, the round count), and the salt, which is the first 22-base64 characters, and the hash itself, which is the remainder.

A HashStrategy using an API like this: https://gist.github.com/sonOfRa/be32884ba1deea00f2b2453245043343 would allow for such functions to be used. However, this would clearly be a breaking change from the current setup. But sometimes it is better to make a clean cut in order to improve security, rather than dragging around old code that does not use current best practices.