Post originally created on github.io on January 2021

Intro

Vulnerability was found by the Synopsys CyRC researchers in the Bouncy Castle java library, in OpenBSDBcrypt class - see following article for more info from their side.

Intro

As it was already written in that post, the issue was with implementation method doCheckPassword- method that checks password against a 60 character Bcrypt string. Let's dive into that and see what was the core issue and how it was fixed.

Analysis

Beginning

In file core/src/main/java/org/bouncycastle/crypto/generators/OpenBSDBCrypt.java we can directly go to method doCheckPassword, where we have couple major checks: whether the Bcrypt string is really Bcrypt string, and if cost factor is from proper range. Nothing special, nothing wrong, just ifs. Relevant to our issue, line 268, states that only string with excatly 60 characters, will be analysed.

if (sLength != 60)

There is nothing wrong with that statment but let's remember the number 60.

Then, in line 307, the Bcrypt hash newBcryptString is generated from password and salt.

String newBcryptString = doGenerate(version, password, salt, cost);

Next, in following lines, created newBcryptString is checked against the provided hash bcryptString. And here we had vulnerability:

        boolean isEqual = sLength == newBcryptString.length();
        for (int i = 0; i != sLength; i++)
        {
            isEqual &= (bcryptString.indexOf(i) == newBcryptString.indexOf(i));
        }
        return isEqual;

In first line of code block there was declaration of a variable with primitive boolean, initialization with the sLength value and comparison between it with an int value from length of newBcryptString. The result should be true, as the freshly created hashed string is 60 char long.
Then there is the for loop, where the first interesting bit is the second statment, where the sLength is used. As you migth remember: 60! So this loop is enumerating from 0 to 59, comparing the outcomes from indexOf method of these two Bcrypt strings.

indexOf

Now I would like to focus a little bit on the indexOf. From the documentation: Returns the index within this string of the first occurrence of the specified character.. In the code there is one parameter of the method indexOf, and it is i which basically is an integer in range from 0 to 59. In 34th iteration the following statment is checked:

  isEqual &= (bcryptString.indexOf(33) == newBcryptString.indexOf(33));

And you migth ask, what's the outcome? This operation is checking if index of first occurence of ! in both string is the same. Wait, what? Method overload with one parameter of indexOf method makes i treated as character in Unicode. In other words, this part of code checks if first occurance of unicode characters from 0 to 59 is the same for both strings.

Let's go back - Bcrypt

https://www.usenix.org/legacy/events/usenix99/provos/provos_html/node1.html
The Bcrypt is hash algorythm, based on the Blowfish cipher, intruduced in 1999. After 22 years it should be considered as secure (but is it https://dl.acm.org/doi/10.5555/2671293.2671303) and is very popular when it comes to storing passwords. Not going into crypto details, this is how Bcrypt string looks:

$2a$10$J4oVzGAgiyWfFqYbHINmbOyaq8NUYn60sRUWf1/Dm5GjkJDeVt/VS
|__|__|_____________________|______________________________|
ALG    SALT                  HASH  
   COST

For this issue however, interesting part is what kind of characters are allowed/possible in Bcrypt String. The answer comes from Bcrypt documentation: both strings the salt and hash are base64-encoded: alphanumeric, + and /.

Unicode

So what kind of characters we can get from 0 to 59? The answer is: some, but interesting for us are these:
36 for $,
43 for +,
47 for /,
and range 48-57 for the 0-9 digits.

Combine all of them together

Combining the Bcrypt and Unicode characters could produce "colissions" on /, + and digits range. According to article, around 20% passwords were checked positively in 1000 cases. For example this Bcrypt string $2a$10$J4oVzGAgiyWfFqYbHINmbOyaq8NUYn60sRUWf1/Dm5Gj38DeVt/VS against that one $2a$10$g4YFiQSjPuYvvg4NMwmwROjTB8ODmu6cKYigA1/i15XP38HXHq/ZK would be the same using this code.

Mitigation

The indexOf method was replaced with charAt, which is way more suitable for checking if character in one string is same as in the other.