Bad Shift in Bcrypt cryptRaw

[david-hawley]

password4j/src/main/java/com/password4j/BcryptFunction.java

Line 768 in bf3a601

rounds = 1 << logRounds;

When logRounds is 31 (the max allowed value), this shift results in a negative number. Effectively the rounds is set to 0.

[firaja]

Hi @david-hawley

actually the maximum allowed is 30 and not 31.

password4j/src/main/java/com/password4j/BcryptFunction.java

Lines 766 to 767 in bf3a601

	if (logRounds < 4 || logRounds > 31)
	throw new BadParametersException("Bad number of rounds");

When logRounds is 31 (the max allowed value), this shift results in a negative number. Effectively the rounds is set to 0.

[david-hawley]

@firaja that BadParametersException check is greater than 31, not greater or equals.
But just try it. Set the hash.bcrypt.rounds=31 and measure how long it takes. Or use a debugger. rounds is set to -2147483648.

[firaja]

@david-hawley sorry I completely misunderstood your point.

I think that converting rounds from int to long should do the trick.

[david-hawley]

Converting to long would fix it. I have a draft PR for that. Given the possible issues below I wonder if changing the limit to 30 would be better.

One of the tests in BcryptFunctionTest.genSaltGeneratesCorrectSaltPrefix is now impractical to run (unless you want the test to take 2 days).

If anyone were currently using 31 rounds this fix is going to cause issues.

1. Checking existing passwords is going to take a very long time (days). As will making new passwords.
2. When that time passes the check will fail against passwords encoded with the previous version.

I am not sure how/if the PR should have any unit tests for this 31 case.

[firaja]

Converting to long would fix it. I have a draft PR for that. Given the possible issues below I wonder if changing the limit to 30 would be better.

One of the tests in BcryptFunctionTest.genSaltGeneratesCorrectSaltPrefix is now impractical to run (unless you want the test to take 2 days).

If anyone were currently using 31 rounds this fix is going to cause issues.

1. Checking existing passwords is going to take a very long time (days). As will making new passwords.
2. When that time passes the check will fail against passwords encoded with the previous version.

I am not sure how/if the PR should have any unit tests for this 31 case.

In this case a unit test is not doable and just checking the parameters encoded in the hash wouldn't add any additional level of correctness that the other tests are already giving.

Even if there may be cases of projects using 31, their hashes would be not correct. A migration approach would be to use version 1.7.0 and update the hash to 30 log rounds. After that install 1.7.1 and migrate back to 31 log rounds. But I think that using 31 log rounds is not usable for any system.

[david-hawley]

Agreed that no one would legit be using 31 or even 30 rounds. The hypothetical situation would be they tried the max allowed and found it to be fast enough (because it was actually 0) and just went with that.

Ignoring any conversion hints/help I think my PR fixes the main issue. I updated one unit test as well.

[codylerum]

Should this be closed?

[david-hawley]

@codylerum The fix was merged. I don't know the close policy for this project. It hasn't been released yet.

I hope no one is unintentionally using 31 rounds. Mitigation is going to be tricky without something to assist.

[firaja]

Hello @codylerum @david-hawley,

this fix is now present in 1.7.1