Authentication bypass in cryptography library
Tags: security, appsec, bcrpyt
Published: 2021-09-25 17:17:42

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](https://www.synopsys.com/blogs/software-security/cve-2020-28052-bouncy-castle/) 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. 
```java
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:

```java 
        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.