Spring Security Bcrypt with strength/log rounds = 31 results in 'Bad number of rounds' error although 31 should be ok

[sabinewinklerboi]

Describe the bug:
I implement a webapp using Spring Boot and Spring Security. Passwords of my users are stored in my DB after encoding with Bcrypt (BCryptVersion $2a).
I use Bcrypt of Spring Security with strength = 31 (new BCryptPasswordEncoder(31)).
When I run my webapp and try to do a login with a user, I get the error Bad number of rounds.

Analysis:
As far as I have seen this is because within org.springframework.security.crypto.bcrypt.BCrypt, method crypt_raw(...) roundsForLogRounds method is called with log_rounds = 31 and returns the value 2147483648, but this calculated value is greater than Integer.MAX_VALUE (=2147483647) and thus results in the mentioned error.
See the following code in crypt_raw():

rounds = roundsForLogRounds(log_rounds);
if (rounds < 16 || rounds > Integer.MAX_VALUE) {
    throw new IllegalArgumentException("Bad number of rounds");
}

Expected behaviour:
As far as I have seen in the JavaDoc and in the source of Spring Security, BCryptPasswordEncoder accepts a strength (= log rounds) between 4 (inclusive) and 31 (inclusive). So 31 should be ok and the resulting Bad number of rounds error is probably a bug.

Version
Spring Boot v2.7.1
Spring v5.3.21
Spring Security v5.7.2

With Spring Boot v2.6.6 and Spring Security v5.6.2 strength = 31 worked as expected. Maybe the behaviour changed in commit e6297d3 ?

[jzheaux]

I agree that the check probably needs an adjustment, though I wonder if you really want the rounds to be set to 31 as this will cause each hash to take 2-3 days to compute on typical hardware.

Are you able to submit a PR that corrects the logic so that 31 rounds works again?

[sabinewinklerboi]

@jzheaux Thanks for the hint regarding if 31 is really a meaningful value to use. I first did not question why the code was executed so fast (with the older version Spring Security v5.6.2) although 31 should lead to a computation of 2-3 days. 🙈
I will try to change it from 31 to a faster alternative (probably 12 so that the algorithm is maximally strong and encoding a pwd takes a reasonable time).

I'm new to Spring Security and new to contributing to open source projects so I would feel better if you could do the changes. Would that be ok?

I further analysed the code a little bit and as far as I have seen, if you want to support log_rounds = 31, probably the check of rounds > Integer.MAX_VALUE has to be removed and the variable i in the for loop has to be of type long instead of int.

rounds = roundsForLogRounds(log_rounds);
if (rounds < 16 || rounds > Integer.MAX_VALUE) {  // remove "rounds > Integer.MAX_VALUE" here?
    throw new IllegalArgumentException("Bad number of rounds");
}

for (int i = 0; i < rounds; i++) {  // use "long i" here?
    key(password, sign_ext_bug, safety);
    key(salt, false, safety);
}

Alternatively, from my point of view, it would also be possible to restrict strength/log_rounds to the range 4 (inclusive) to 30 (inclusive) to avoid the int overflows. But then you have to adapt the checks where the range 4 to 30 is checked and you have to adapt the samples/unit tests, JavaDoc, etc.

[jzheaux]

For help with changing your existing hashes, please see https://github.com/spring-io/cve-2022-22976-bcrypt-skips-salt.

I'm new to Spring Security and new to contributing to open source projects so I would feel better if you could do the changes. Would that be ok?

Sure no problem.