Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Salt not extracted from hash when performing check #35

Closed
zakhdar opened this issue Apr 13, 2021 · 7 comments
Closed

Salt not extracted from hash when performing check #35

zakhdar opened this issue Apr 13, 2021 · 7 comments
Milestone

Comments

@zakhdar
Copy link

zakhdar commented Apr 13, 2021

Describe the bug
in the documentation (for Argon2) it is mentioned that the SALT is stored in the computed hash and therefore automatically used during a call to check, which is not the case and needs to be specified again manually during a verification call.

When the documentation refers to the hash, does it refer to the com.password4j.Hash object ? or the computed hash (hash.getResult()) ? it maybe not a bug but the documentation is unclear on this point.

To Reproduce

psw4j.properties :

global.random.strong=true
global.pepper=MyPepper
hash.argon2.memory=4096
hash.argon2.iterations=160
hash.argon2.length=128
hash.argon2.parallelism=2
hash.argon2.type=id
hash.argon2.version=19

Test :

Hash hash = Password.hash("my password").addRandomSalt().withArgon2();
String result = hash.getResult();
Password.check("my password", result).withArgon2()  <== return false

Working code :

Hash hash = Password.hash("my password").addRandomSalt().withArgon2();
String salt = hash.getSalt();
String result = hash.getResult();

Password.check("my password", result).addSalt(salt).withArgon2();  <== return true

Expected behavior
If salt is already embedded in the computed hash (hash.getResult()) it need to automatically extracted from here (if it exists) in the verification function , this would avoid having in database (result has saved in db in my case) a column for the result and another column which would contain the salt.

Environment:

  • OS: Windows 10 Pro
    java version "11.0.9" 2020-10-20 LTS
    Java(TM) SE Runtime Environment 18.9 (build 11.0.9+7-LTS)
    Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.9+7-LTS, mixed mode)
@firaja
Copy link
Member

firaja commented Apr 14, 2021

Hi @zakhdar,

your test works fine on my machine (Ubuntu 18.04, Java 11.0.10+9) so it has to be analyzed. Can you please re-do your test but with global.random.strong=false and see if the check is successful?
Along with your answer, please send me the hash.getResult() string both with global.random.strong=true and global.random.strong=false.

It may be that your machine does not support a strong secure random source or, if it is supported, something in the library is not working as expected on your environment.
Thank you very much!

@firaja firaja added this to the 1.5.3 milestone Apr 14, 2021
@zakhdar
Copy link
Author

zakhdar commented Apr 14, 2021

Dear @firaja ,

You will find bellow the results of various tests requested.

Hash hash = Password.hash("my password").addRandomSalt().withArgon2();
String result = hash.getResult();
System.out.println(hash.getResult());
System.out.println(Password.check("my password", result).withArgon2());

With global.random.strong=true

Result :

$argon2id$v=19$m=4096,t=160,p=2$JS9Sw7tjw7JEwr7DqiNkeuKAlMK2JsWTQsKgwrFew4MPeyN7w6VpJ1hrMk3CqijDue+/vQ3DvMOz4oCYAsODw5tpxaHCtMOYDQTDszXCvCMFw73CoSgmw6suL2hAw44$kmUOS+6q6AikagNROjMc3enR6fZd9qnNXdeO4xSDZ7oOCh+7nBs19Vuo4a0XgfBjTjwwk0Q5xUdFxFnTmTJcxFov9VmmOxTYsX3Ql5/TxdK6ia8KbedZkXZaPzo0tfB7CY20TVsjw5b3a8TeF/0YIey1YU3K6Q1gUzD73K+/UB8

Check : false

With global.random.strong=false

Result :

$argon2id$v=19$m=4096,t=160,p=2$w7/igKFRJArigKYbH8O9GG94Y38+xZNYQT7DncK7NsO8wqRZw6rDojxp77+94oCmUwDCv33DoMKrwqPvv73vv71cwqvDjMO7wrolxZMgOMOGwqPigJ7DuQHDnsO2wrDCs8OTwrtww4TDs8OA$g+vpEUMg3aG6xqmJEJdV4/QggVXN37K9PbGk2h6+PJecpaGU8cGOtIaMw9/47pLLEw8ljcecHQsgmVc1a98+Gz/37rci/UzpulgKKI9U9Tp7bMY2iiokP1cx+Pxinlr76JO6eentDy2v2MF8B28RlJMgJgFBsAwK01dEgkJm4hc

Check : false

thank you for your support.

@firaja
Copy link
Member

firaja commented Apr 14, 2021

@zakhdar thanks for the info.

This is probably due to a not specified encoding in Argon2Function#check(CharSequence, String, String, CharSequence).

The extracted salt in byte[] si converted into a String but the encoding is not specified. In this case Charset.defaultCharset() is used to determine the encoding, which is dependent on the environment .
On my machine the JVM's default encoding is UTF-8 and it's also the default encoding used by the library (except for this piece of code 😠).

Please, can you send me the result of the following piece of code?

System.out.println(Charset.defaultCharset().name());

If it is not UTF-8 then we have almost certainly found the issue.

@zakhdar
Copy link
Author

zakhdar commented Apr 14, 2021

@firaja

You got it right, indeed the charset on my machine is windows-1252 hence the problem, if I add as an argument to the JVM -Dfile.encoding = UTF-8 the library works correctly.

$argon2id$v=19$m=4096,t=160,p=2$77+9RErvv71YyqVH77+9NyEN77+9Su+/vXkA77+977+977+9Be+/ve+/ve+/ve+/vV0277+977+9cu+/ve+/ve+/ve+/vRvvv71fKu+/vT3vv73vv73vv71r77+9ae+/ve+/vWfvv71k77+977+9P++/vTQHW0zvv73vv70qRA$zAkcYJX+fDpvr/IovW1hDCbg2ekSCVMnYScpCCzm2LtTdoZGB+2rOW3IW2iUrqrJzZNcNi0cm8WGoJ2hEbcRAGFahKg2RRBZOW85e4x/XxN7+6qON51Cv/ZqrUaWGOc0eP/VZdrm6byHVF/srhMEjPfzNb7x/8K91Up3+gZ5Rbo

Check : true

It might be interresting to provide an addition in the documentation or possibly raise an exception in the program if the encoding of the machine is different from that used by the library , or maybe the librairy can use charset of the machine by default.

Thank you for your help and your work !!

@firaja
Copy link
Member

firaja commented Apr 14, 2021

@zakhdar

this commit solves the issue: the encoding must be the same along with the library and be consistent across different environments.
The fix is available on master branch and by the end of the day (CEST) is going to be released (1.5.3).

Thank you for your time 😉

@firaja
Copy link
Member

firaja commented Apr 14, 2021

Released in 1.5.3.

@firaja firaja closed this as completed Apr 14, 2021
@Sparklll
Copy link

Sparklll commented Apr 18, 2021

@firaja

Great job!!! Thank you so much!!! I just ran into this problem these days when local project works fine (JVM's default encoding is UTF-8) and deployed version behaves in an incomprehensible way, encryption doesn't work in the right way. It turned out that the encoding on the server's JVM was ISO-8859-1. After the update up to v1.5.3 everything works stably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants