kafsemo.org

jBCrypt is a pure Java implementation of the bcrypt key derivation function, used for password hashing. The first release (0.1) was in May 2006, and subsequent releases have fixed a number of bugs. That most recent release (0.4, in January 2015) fixed an integer overflow bug in the case of trying to use the maximum number of rounds. It’s unlikely that you would currently run into that bug, but it’s still a good idea to fix it.

As someone who has forked jBCrypt, in order to get fixes into Spring Security, here’s a little about the history of that issue and three possible ways to fix it.

The bug

It’s an integer overflow bug with the number of rounds. bcrypt’s tunable work factor is the base-2 log of the number of rounds, so a natural way to implement this is:

int log_rounds = 10;

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

int rounds = 1 << log_rounds;

for (int i = 0; i < rounds; i++) {
    // Iterate
}

Here’s the problem; we’re checking that log_rounds is not more than 31, but what about the edge case?

System.out.println(1 << 30);
1073741824

System.out.println(1 << 31);
-2147483648

Oops; bug. 0 < -2147483648 is immediately false, the loop ends without ever running, and the result is... not secure.

The fixes

One fork is on Google Code (jbcrypt). It’s notable for getting org.mindrot:jbcrypt:0.3m published in the Central Repository.

There, this bug was filed as Issue 1: Integer overflow when log_rounds = 31 back in 2011:

I expected BCrypt.hashpw(p, BCrypt.gensalt(31)) to take a really long time but instead it returns immediately. It's because int overflows on 2^31 and the key setup loop returns immediately.

The attached patch switches the type of rounds to long:

System.out.println(1L << 30);
1073741824

System.out.println(1L << 31);
2147483648

Excellent; it stays positive, and the loop runs a couple of billion times (rather than not at all).

However, despite being reported, this patch was never applied to that fork, never made it to Central and never made it back upstream.

Spring Security

Just after that bug report, Spring Security took another fork of jBCrypt (SEC-1472 - Add support for bcrypt password encoding), from upstream but with their own fix for the bug:

rounds = 1 << log_rounds;

for (int i = 0; i != rounds; i++) {
    // Iterate
}

Here, the use of != rounds instead of < rounds avoids the problem with the overflow. The loop doesn’t stop immediately and continues with the correct number of iterations.

Later, in 2012, I wanted to use bcrypt with Spring Security, so I tried to solve the proliferation of forks with the obvious solution: another fork. Here, I combined the history from the Google Code project with the suggested fix, merged in some reformatting from Spring Security and submitted it back to Spring Security (SEC-1990 : Code cleanup on bcrypt implementation). That had the effect of switching the fix Spring Security was using.

Next, in 2013, the issue was reported to the original project’s issue tracker (Bug 2097 - if gensalt’s log_rounds parameter is set to 31 it does 0 (ZERO) rounds!). A good clear explanation and a suggestion to use a long for the rounds.

CVE-2015-0886

CVE-2015-0886 was created in January 2015, with the same issue again. Promptly afterwards, jBCrypt 0.4 was released with a combination fix:

if (log_rounds < 4 || log_rounds > 30)
    throw new IllegalArgumentException ("Bad number of rounds");
...
for (i = 0; i != rounds; i++) {

rounds stays as an integer, the loop logic is fixed to cope with overflow (as with Spring Security) and the maximum number of rounds is now set to 30, to also fail ahead of the loop. This is the third way to fix this bug: reject values which would cause overflow.

Combining two fixes is belt-and-braces, but it works (at least, until you need 2^31 rounds, predicted by one Information Security Stack Exchange answer to be around 2037).

Constant-time string comparison

The original Spring Security import also brought with it a constant-time string comparison (A Lesson In Timing Attacks). That’s also fixed in the upstream 0.4, taking the original implementation from the Spring Security contribution.

In conclusion

Depending on whether you take jBCrypt from the Central Repository, from the original project or from one of the forks, you’ll have had various bugs fixed at varying times over the last four years. For a password hash, that’s not ideal.

Spring Security’s version has, generally, been more secure than the original upstream project. That is also not ideal.

Using the version from Central, which is a fairly likely thing for a Java developer to do, currently leaves you vulnerable (albeit to a bug you’re fairly unlikely to encounter), until Issue 11: Update for 0.4 is resolved.

The ideal would be a single version, actively developed, tracked in Git, available in Maven and consistently used across projects. Without that, keep an eye out for bugs, and make sure you know who’s responsible for the security-related code that you’re running.