SCryptFunction.check() should honor derived0.length

[dpatriarche]

Describe the bug
I am considering migrating from com.lambdaworks:scrypt to com.password4j:password4j. The derived key length of hashes generated by the lambdaworks implementation is 32 bytes, while password4j's scrypt implementation produces 64 byte derived keys. This is fine. The problem is that when checking against an existing hash, password4j's scrypt's check() method requires that the input hash's derived key be 64 bytes. If the derived key length of the input hash is not 64 bytes then check() returns false (com.password4j.SCryptFunction line 151).

To Reproduce
Verify with these (valid) hashes generated by com.lambdaworks:scrypt:

Test hash 1: password = "Hello world!"
$s0$e0801$fl+gNAicpGG4gLMkUTCvLw==$N5wE1IKsr4LPBoetJVW6jLzEH4kTVXuKGafvAA8Z+88=

Test hash 2: password = "!@#$%^&*()_+";
$s0$e0801$1uFqXES/I17Wj6W85SLXNA==$GvPgo5L9KMtde+jpR5rRd5U7Qa6mcRMFAoy5E50iBro=

Expected behavior
I would expect that password4j's HashingFunction.check() methods respect the derived key lengths of the input hashes they are checking against. Looking at the code for password4j's argon2 implementation it does in fact appear to respect the derived key length of the input hash.

It would also be nice if password4j's scrypt had a configurable derived key length, similar to argon2's "hash.argon2.length".

Environment:

• n/a

Additional context
n/a

NOTE
If you agree with the above change then I would be happy to submit a pull request.

[firaja]

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!

[dpatriarche]

Okay, I'll do that.

[dpatriarche]

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.

[firaja]

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.

[dpatriarche]

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.

[firaja]

Thank you for your work @dpatriarche!

[dpatriarche]

My pleasure, happy to contribute!

[firaja]

Hi @dpatriarche

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

[dpatriarche]

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