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 - seefollowing article for more info from their side.

Intro

As it was already written in that post, the issue was with implementation methoddoCheckPassword- 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 filecore/src/main/java/org/bouncycastle/crypto/generators/OpenBSDBCrypt.java we can directly go to methoddoCheckPassword, 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, justifs. 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 number60.

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

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

Next, in following lines, creatednewBcryptString is checked against the provided hashbcryptString. And here we had vulnerability:

booleanisEqual=sLength==newBcryptString.length();for(inti=0;i!=sLength;i++){isEqual&=(bcryptString.indexOf(i)==newBcryptString.indexOf(i));}returnisEqual;

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

indexOf

Now I would like to focus a little bit on theindexOf. 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 methodindexOf, and it isi 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 ofindexOf method makesi 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 ithttps://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 range48-57 for the0-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

TheindexOf method was replaced withcharAt, which is way more suitable for checking if character in one string is same as in the other.