Update Sha2Crypt.java #300
Conversation
The old regular expression is incorrect.
If you want to match [a-zA-Z0-9./] and limit the length to 16, I think it should be the following expression：
"^\\$([56])\\$(rounds=(\\d+)\\$)?([\\.\\/a-zA-Z0-9]{1,16})$")
|
There are no tests in this PR. |
|
I agree that the regex looks wrong. As it stands, it will accept any salt that has at least one alphanumeric character after the However, the Javadoc for Crypt.crypt(String,String) [1] says: " ... It is therefore valid to enter a complete hash value as salt ..." This would not be possible if the regex was changed as per this PR. |
|
I've added some more tests to CryptTest based on the Javadoc. The Javadoc says that '$' will terminate the salt. An alternative might be to allow an optional '$' followed by any characters. For example, replace '.' at the end of the RE with '($|\$.)' |
|
The proposed RE does not work because it does not allow for truncating the salt after 16 characters, as per the following test failures: Sha256CryptTest.testSha256CryptStrings:63 » IllegalArgument Invalid salt value: $5$1234567890123456789 There needs to be further work before the RE can be made stricter. |
Both of the above salt values are wrong. We need the salt to be 16 bytes or less |
Not so, see below.
Agreed. The JavaDoc for Crypt includes the phrase " ... and is cut at the maximum length ...." See It should be possible to allow for a longer salt, but it may be tricky to do so in a single regex. The first task is to document exactly what should be allowed and what is not allowed. |
3 participants
The old regular expression is incorrect.
If you want to match [a-zA-Z0-9./] and limit the length to 16, I think it should be the following expression： "^\$([56])\$(rounds=(\d+)\$)?([\.\/a-zA-Z0-9]{1,16})$")