Skip to content

Commit

Permalink
github #627- introduced constant time equals, added support for byte[…
Browse files Browse the repository at this point in the history 
…] passwords.
  • Loading branch information
@dghgit
dghgit committed Jan 28, 2020
1 parent 7142996 commit 00dfe74
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 56 deletions.
219 changes: 163 additions & 56 deletions core/src/main/java/org/bouncycastle/crypto/generators/OpenBSDBCrypt.java
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -55,64 +55,94 @@ public class OpenBSDBCrypt
} }
} }


public OpenBSDBCrypt() private OpenBSDBCrypt()
{ {


} }


/**
* Creates a 60 character Bcrypt String, including
* version, cost factor, salt and hash, separated by '$' using version
* '2y'.
*
* @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt
* @param password the password
* @return a 60 character Bcrypt String
*/
public static String generate(
char[] password,
byte[] salt,
int cost)
{
return generate(defaultVersion, password, salt, cost);
}

/**
* Creates a 60 character Bcrypt String, including
* version, cost factor, salt and hash, separated by '$' using version
* '2y'.
*
* @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt
* @param password the password
* @return a 60 character Bcrypt String
*/
public static String generate(
byte[] password,
byte[] salt,
int cost)
{
return generate(defaultVersion, password, salt, cost);
}

/** /**
* Creates a 60 character Bcrypt String, including * Creates a 60 character Bcrypt String, including
* version, cost factor, salt and hash, separated by '$' * version, cost factor, salt and hash, separated by '$'
* *
* @param version the version, 2y,2b or 2a. (2a is not backwards compatible.) * @param version the version, may be 2b, 2y or 2a. (2a is not backwards compatible.)
* @param cost the cost factor, treated as an exponent of 2 * @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt * @param salt a 16 byte salt
* @param password the password * @param password the password
* @return a 60 character Bcrypt String * @return a 60 character Bcrypt String
*/ */
private static String createBcryptString(String version, public static String generate(
byte[] password, String version,
byte[] salt, char[] password,
int cost) byte[] salt,
int cost)
{ {
if (!allowedVersions.contains(version)) if (password == null)
{ {
throw new IllegalArgumentException("Version " + version + " is not accepted by this implementation."); throw new IllegalArgumentException("Password required.");
} }


StringBuffer sb = new StringBuffer(60); return doGenerate(version, Strings.toUTF8ByteArray(password), salt, cost);
sb.append('$');
sb.append(version);
sb.append('$');
sb.append(cost < 10 ? ("0" + cost) : Integer.toString(cost));
sb.append('$');
sb.append(encodeData(salt));

byte[] key = BCrypt.generate(password, salt, cost);

sb.append(encodeData(key));

return sb.toString();
} }


/** /**
* Creates a 60 character Bcrypt String, including * Creates a 60 character Bcrypt String, including
* version, cost factor, salt and hash, separated by '$' using version * version, cost factor, salt and hash, separated by '$'
* '2y'.
* *
* @param version the version, may be 2b, 2y or 2a. (2a is not backwards compatible.)
* @param cost the cost factor, treated as an exponent of 2 * @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt * @param salt a 16 byte salt
* @param password the password * @param password the password already encoded as a byte array.
* @return a 60 character Bcrypt String * @return a 60 character Bcrypt String
*/ */
public static String generate( public static String generate(
char[] password, String version,
byte[] password,
byte[] salt, byte[] salt,
int cost) int cost)
{ {
return generate(defaultVersion, password, salt, cost); if (password == null)
} {
throw new IllegalArgumentException("Password required.");
}


return doGenerate(version, Arrays.clone(password), salt, cost);
}


/** /**
* Creates a 60 character Bcrypt String, including * Creates a 60 character Bcrypt String, including
Expand All @@ -121,12 +151,12 @@ public static String generate(
* @param version the version, may be 2b, 2y or 2a. (2a is not backwards compatible.) * @param version the version, may be 2b, 2y or 2a. (2a is not backwards compatible.)
* @param cost the cost factor, treated as an exponent of 2 * @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt * @param salt a 16 byte salt
* @param password the password * @param psw the password
* @return a 60 character Bcrypt String * @return a 60 character Bcrypt String
*/ */
public static String generate( private static String doGenerate(
String version, String version,
char[] password, byte[] psw,
byte[] salt, byte[] salt,
int cost) int cost)
{ {
Expand All @@ -135,10 +165,6 @@ public static String generate(
throw new IllegalArgumentException("Version " + version + " is not accepted by this implementation."); throw new IllegalArgumentException("Version " + version + " is not accepted by this implementation.");
} }


if (password == null)
{
throw new IllegalArgumentException("Password required.");
}
if (salt == null) if (salt == null)
{ {
throw new IllegalArgumentException("Salt required."); throw new IllegalArgumentException("Salt required.");
Expand All @@ -152,8 +178,6 @@ else if (salt.length != 16)
throw new IllegalArgumentException("Invalid cost factor."); throw new IllegalArgumentException("Invalid cost factor.");
} }


byte[] psw = Strings.toUTF8ByteArray(password);

// 0 termination: // 0 termination:


byte[] tmp = new byte[psw.length >= 72 ? 72 : psw.length + 1]; byte[] tmp = new byte[psw.length >= 72 ? 72 : psw.length + 1];
Expand Down Expand Up @@ -190,11 +214,61 @@ public static boolean checkPassword(
String bcryptString, String bcryptString,
char[] password) char[] password)
{ {
if (password == null)
{
throw new IllegalArgumentException("Missing password.");
}

return doCheckPassword(bcryptString, Strings.toUTF8ByteArray(password));
}

/**
* Checks if a password corresponds to a 60 character Bcrypt String
*
* @param bcryptString a 60 character Bcrypt String, including
* version, cost factor, salt and hash,
* separated by '$'
* @param password the password as an array of bytes
* @return true if the password corresponds to the
* Bcrypt String, otherwise false
*/
public static boolean checkPassword(
String bcryptString,
byte[] password)
{
if (password == null)
{
throw new IllegalArgumentException("Missing password.");
}

return doCheckPassword(bcryptString, Arrays.clone(password));
}

/**
* Checks if a password corresponds to a 60 character Bcrypt String
*
* @param bcryptString a 60 character Bcrypt String, including
* version, cost factor, salt and hash,
* separated by '$'
* @param password the password as an array of chars
* @return true if the password corresponds to the
* Bcrypt String, otherwise false
*/
private static boolean doCheckPassword(
String bcryptString,
byte[] password)
{
if (bcryptString == null)
{
throw new IllegalArgumentException("Missing bcryptString.");
}

// validate bcryptString: // validate bcryptString:
if (bcryptString.length() != 60) final int sLength = bcryptString.length();
if (sLength != 60)
{ {
throw new DataLengthException("Bcrypt String length: " throw new DataLengthException("Bcrypt String length: "
+ bcryptString.length() + ", 60 required."); + sLength + ", 60 required.");
} }


if (bcryptString.charAt(0) != '$' if (bcryptString.charAt(0) != '$'
Expand Down Expand Up @@ -226,17 +300,53 @@ public static boolean checkPassword(
throw new IllegalArgumentException("Invalid cost factor: " + cost + ", 4 < cost < 31 expected."); throw new IllegalArgumentException("Invalid cost factor: " + cost + ", 4 < cost < 31 expected.");
} }
// check password: // check password:
if (password == null)
{
throw new IllegalArgumentException("Missing password.");
}
byte[] salt = decodeSaltString( byte[] salt = decodeSaltString(
bcryptString.substring(bcryptString.lastIndexOf('$') + 1, bcryptString.substring(bcryptString.lastIndexOf('$') + 1,
bcryptString.length() - 31)); sLength - 31));

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

boolean isEqual = sLength == newBcryptString.length();
for (int i = 0; i != sLength; i++)

This comment has been minimized.

Sign in to view

Copy link
@ArashPartow

ArashPartow Dec 19, 2020

People are saying this loop is suspicious.

This comment has been minimized.

Sign in to view

Copy link
@rzwitserloot

rzwitserloot Dec 19, 2020

It's using indexOf instead of charAt. sLength is 60 (see line 268, which throws if it isn't precisely 60). This code checks if the position of first occurrence of unicode points 0-59 are in the same location in both strings. If yes, the hash is considered equal. The bcrypt strings contain ascii: It's your standard /bin/crypt string, for example $2y$10$.vGA1O9wmRjrwAVXD98HNOgsNpDczlqm3Jq7KnEd1rVAGv3Fykk1a.

unicode 0-60 covers almost nothing - of all characters that could possibly be in string, only $, 1, 0, 2, . and / (The dot and the slash are the only 2 of the the 64 chars used for base64 encoding).

So, it checks if e.g. the position of the first 'tab' character in bcryptString is the same as the position of the first 'tab' in newBcryptString: bcryptString.indexOf(9) == newBcryptString.indexOf(9). This is trivially true: Neither string can possibly contain a tab, so the indexOf call dutifully returns -1.

Almost all hashes are considered equal as long as they use the same # of rounds. A few won't be, because you get 'lucky' due to the first slash / first dot not appearing in the same place.

This comment has been minimized.

Sign in to view

Copy link
@LoupVaillant

LoupVaillant Dec 20, 2020

I believe the real problem is that this loop even exists. The constant time comparison code should be concentrated in one place (perhaps in some utility class), and then called everywhere. This would have prevented this bug.

This comment has been minimized.

Sign in to view

Copy link
@LoupVaillant

LoupVaillant Dec 21, 2020

I have reported #855 and they seem to be taking it seriously.
Note that BouncyCastle is a big project. If someone could scour the code for constant time comparisons and report any occurrence they spotted on #855, that would help with the refactoring.

This comment has been minimized.

Sign in to view

Copy link
@schreddies

schreddies Jan 24, 2021

@rzwitserloot But unicode 48-57 are numbers also possible in Bcrypt?
{
isEqual &= (bcryptString.indexOf(i) == newBcryptString.indexOf(i));
}
return isEqual;
}

/**
* Creates a 60 character Bcrypt String, including
* version, cost factor, salt and hash, separated by '$'
*
* @param version the version, 2y,2b or 2a. (2a is not backwards compatible.)
* @param cost the cost factor, treated as an exponent of 2
* @param salt a 16 byte salt
* @param password the password
* @return a 60 character Bcrypt String
*/
private static String createBcryptString(String version,
byte[] password,
byte[] salt,
int cost)
{
if (!allowedVersions.contains(version))
{
throw new IllegalArgumentException("Version " + version + " is not accepted by this implementation.");
}

StringBuilder sb = new StringBuilder(60);
sb.append('$');
sb.append(version);
sb.append('$');
sb.append(cost < 10 ? ("0" + cost) : Integer.toString(cost));
sb.append('$');
encodeData(sb, salt);


String newBcryptString = generate(version, password, salt, cost); byte[] key = BCrypt.generate(password, salt, cost);


return bcryptString.equals(newBcryptString); encodeData(sb, key);

return sb.toString();
} }


/* /*
Expand All @@ -245,9 +355,9 @@ public static boolean checkPassword(
* @param a byte representation of the salt or the password * @param a byte representation of the salt or the password
* @return the Bcrypt base64 String * @return the Bcrypt base64 String
*/ */
private static String encodeData( private static void encodeData(
StringBuilder sb,
byte[] data) byte[] data)

{ {
if (data.length != 24 && data.length != 16) // 192 bit key or 128 bit salt expected if (data.length != 24 && data.length != 16) // 192 bit key or 128 bit salt expected
{ {
Expand All @@ -266,7 +376,6 @@ private static String encodeData(
data[data.length - 1] = (byte)0; data[data.length - 1] = (byte)0;
} }


ByteArrayOutputStream out = new ByteArrayOutputStream();
int len = data.length; int len = data.length;


int a1, a2, a3; int a1, a2, a3;
Expand All @@ -277,24 +386,22 @@ private static String encodeData(
a2 = data[i + 1] & 0xff; a2 = data[i + 1] & 0xff;
a3 = data[i + 2] & 0xff; a3 = data[i + 2] & 0xff;


out.write(encodingTable[(a1 >>> 2) & 0x3f]); sb.append((char)encodingTable[(a1 >>> 2) & 0x3f]);
out.write(encodingTable[((a1 << 4) | (a2 >>> 4)) & 0x3f]); sb.append((char)encodingTable[((a1 << 4) | (a2 >>> 4)) & 0x3f]);
out.write(encodingTable[((a2 << 2) | (a3 >>> 6)) & 0x3f]); sb.append((char)encodingTable[((a2 << 2) | (a3 >>> 6)) & 0x3f]);
out.write(encodingTable[a3 & 0x3f]); sb.append((char)encodingTable[a3 & 0x3f]);
} }


String result = Strings.fromByteArray(out.toByteArray());
if (salt == true)// truncate padding if (salt == true)// truncate padding
{ {
return result.substring(0, 22); sb.setLength(sb.length() - 2);
} }
else else
{ {
return result.substring(0, result.length() - 1); sb.setLength(sb.length() -1);
} }
} }



/* /*
* decodes the bcrypt base 64 encoded SaltString * decodes the bcrypt base 64 encoded SaltString
* *
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/org/bouncycastle/crypto/test/OpenBSDBCryptTest.java
View file
Original file line number Original file line Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.bouncycastle.crypto.test; package org.bouncycastle.crypto.test;


import org.bouncycastle.crypto.generators.OpenBSDBCrypt; import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
import org.bouncycastle.util.Strings;
import org.bouncycastle.util.test.SimpleTest; import org.bouncycastle.util.test.SimpleTest;


public class OpenBSDBCryptTest public class OpenBSDBCryptTest
Expand Down Expand Up @@ -104,6 +105,18 @@ public String getName()
public void performTest() public void performTest()
throws Exception throws Exception
{ {
byte[] salt = new byte[16];
for (int i = 0; i < bcryptTest1.length; i++)
{
String[] testString = bcryptTest1[i];
char[] password = testString[1].toCharArray();

String s1 = OpenBSDBCrypt.generate(password, salt, 4);
String s2 = OpenBSDBCrypt.generate(Strings.toByteArray(password), salt, 4);

isEquals(s1, s2);
}

for (int i = 0; i < bcryptTest1.length; i++) for (int i = 0; i < bcryptTest1.length; i++)
{ {
String[] testString = bcryptTest1[i]; String[] testString = bcryptTest1[i];
Expand Down Expand Up @@ -175,6 +188,17 @@ public void performTest()
fail("twoBVec mismatch: " + "[" + i + "] " + password); fail("twoBVec mismatch: " + "[" + i + "] " + password);
} }
} }

for (int i = 0; i < twoBVec.length; i++)
{
password = twoBVec[i][0];
encoded = twoBVec[i][1];

if (!OpenBSDBCrypt.checkPassword(encoded, Strings.toUTF8ByteArray(password)))
{
fail("twoBVec mismatch: " + "[" + i + "] " + password);
}
}
} }
} }


1 comment on commit 00dfe74

@dghgit
Copy link
Contributor Author

@dghgit dghgit commented on 00dfe74 Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the original commit is not suspicious, it's just totally wrong. The problem with the bug is it still "appears" to work, it's only when you look at the code it's obvious that it's not correct.

Please sign in to comment.