Security Review: Review the code and find the bugs that would otherwise pass a functional code review or tests, but have serious security flaws.
Suppose a system exists with some run-of-the-mill accounts implementation. Somewhere down the line the business decides they want to be able to support shared/joint accounts. The new requirement is such that access to the account is only granted if passwords for all users of the account are provided when signing in. To save the hassle of having to modify the DB schema, the multi-passwords are implemented so that they’re stored in the same single DB field.
The following code is written to implement the new functionality. What could possibly be wrong?
But I thought bcrypt was secure…
The vulnerability lay with a particular detail of bcrypt combined with the method being used to combine passwords. The docs for bcrypt state the following:
The bcrypt algorithm only handles passwords up to 72 characters, any characters beyond that are ignored.
Normally, this isn’t such a big deal, but to support multi-passwords this code is concatenating the passwords before hashing them with bcrypt.
To exploit this an attacker needs to make sure they’re the first one to provide their password when creating the hash, and that their password is at least 72 characters long. So long as they can provide a password of at least 72 character length, then any other passwords provided will be irrelevant.
Looking further into the bcrypt module docs, it goes on to say:
To work around this, a common approach is to hash a password with a cryptographic hash (such as
sha256) and then base64 encode it to prevent NULL byte problems before hashing the result with
Having the module emit a warning when providing a password with more than 72 characters may also be a nice way to alert developers to this potential issue, but none of the bcrypt implementations I tried do this ¯\_(ツ)_/¯
Securing The Code
There’s a few ways to fix the vulnerabilities here. One would be to use a password hashing algorithm that doesn’t truncate the passwords, but this would be more of a band-aid fix than a proper fix. The better strategy would obviously be to separate each password into separate entities so that each is hashed individually. Not only would this correct the truncation issue, but should the hashes get exposed, then an attacker would need to crack two hashes instead of one.
An often repeated idiom of digital security is “never roll your own crypto“, but at the same time, blindly using crypto modules can cause problems, too. BCrypt is a generally well reputed password hashing algorithm, but it’s still important to read the docs and have a basic understanding of the implementation, otherwise you may end up with vulnerabilities as exemplified in the code.
The other lesson, as said by a friend: don’t store your data in stupid ways. Concatenating passwords so that they can be stuffed in a single field is not wise. Not only did it lead to the truncation issue, but it is functionally less capable as well: if one of the users on the account wants to change their password, then both passwords would be required to do so.