SCryptFunction.check() should honor derived0.length about password4j HOT 9 CLOSED

Hi @dpatriarche that sounds a great feature!
The desired key length should be part of the input parameters!
Feel free to open a pull request!

from password4j.

Okay, I'll do that.

from password4j.

Looking at the code further, I believe that Password.check(password, inputHash).withSCrypt() doesn't look at the input hash's scrypt parameters, i.e. workfactor (N), resources (r), and parallelization (p). If the any of the input hash's "Nrp" values differs from the values specified in the psw4j.properties file (or the defaults in AlgorithmFinder.getSCryptInstance()) then the check will always fail.

Do you agree that this is the case? If so, then I can address this too in HashChecker. I notice that HashUpdater actually does look at the scrypt parameters, so it seems like a straightforward change to make HashChecker call getInstanceFromHash() methods like HashUpdater does. I propose making this change for all the parameterized algorithms (scrypt, argon2, etc.).

One last thing: Is there a reason the algorithms keep a persistent INSTANCES map of instances in memory? It's not a big deal for scrypt, but for an argon2 instance with m=4096, that 1MiB of memory that is allocated forever.

from password4j.

Yes that was done on purpose: the system configurations determine the way the password are checked, not the data retrieved from database.
In case you want to follow the configurations stored in the hash you can always do something like this:

Password.check(userPassword, hashFromDB).with(SCrypt.getInstanceFromHash(hashFromDB));

or

Password.check(userPassword, hashFromDB).with(SCrypt.getInstance(N, r, p));

The problem in both approaches, as you correctly pointed out, is that there is no way to explicit a key length different from 64 bytes.

A new method getInstance(int, int, int, int) must be used (the old one should use a default key length, like Argon2 does with the version) and a change on getInstanceFromHash(String) so that it reads the length of the key from the hash.

A few thoughts about the persistent INSTANCES:

• Most of the target systems check passwords with a constant configuration (again, the system configurations must lead) rather than using different configurations for different hashes (but you can still do it with Password4j with something like my first example).
• In a multi threaded application (e.g. any web application) building the very same object hundred or even thousand times per second it's a waste of time and space. Especially in the case of Argon2, where at least the initialBlockMemory is computed just once and it is shared among all the instances.

from password4j.

Ah okay, thanks for the insight! I will rework my pull request to conform to the above, with the new method getInstance(int, int, int, int) that you suggest.

from password4j.

Thank you for your work @dpatriarche!

from password4j.

My pleasure, happy to contribute!

from password4j.

Hi @dpatriarche

release 1.5.1 contains your fix and it has been published in maven central.

from password4j.

Thanks very much, I have updated my project to use the new version.

from password4j.