Skip to content

Update to jBCrypt 0.4 to correct an integer overflow #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
reedloden wants to merge 1 commit into from
+38 −10
Closed

Update to jBCrypt 0.4 to correct an integer overflow #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions ext/jruby/bcrypt_jruby/BCrypt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package bcrypt_jruby;

import java.io.UnsupportedEncodingException;

import java.security.SecureRandom;

/**
Expand Down Expand Up @@ -56,7 +55,7 @@
* <p>
* The amount of work increases exponentially (2**log_rounds), so
* each increment is twice as much work. The default log_rounds is
* 10, and the valid range is 4 to 31.
* 10, and the valid range is 4 to 30.
*
* @author Damien Miller
* @version 0.2
Expand Down Expand Up @@ -336,7 +335,9 @@ public class BCrypt {
0xb74e6132, 0xce77e25b, 0x578fdfe3, 0x3ac372e6
};

// bcrypt IV: "OrpheanBeholderScryDoubt"
// bcrypt IV: "OrpheanBeholderScryDoubt". The C implementation calls
// this "ciphertext", but it is really plaintext or an IV. We keep
// the name to make code comparison easier.
static private final int bf_crypt_ciphertext[] = {
0x4f727068, 0x65616e42, 0x65686f6c,
0x64657253, 0x63727944, 0x6f756274
Expand Down Expand Up @@ -602,23 +603,24 @@ private void ekskey(byte data[], byte key[]) {
* @param salt the binary salt to hash with the password
* @param log_rounds the binary logarithm of the number
* of rounds of hashing to apply
* @param cdata the plaintext to encrypt
* @return an array containing the binary hashed password
*/
private byte[] crypt_raw(byte password[], byte salt[], int log_rounds) {
public byte[] crypt_raw(byte password[], byte salt[], int log_rounds,
int cdata[]) {
int rounds, i, j;
int cdata[] = (int[])bf_crypt_ciphertext.clone();
int clen = cdata.length;
byte ret[];

if (log_rounds < 4 || log_rounds > 31)
if (log_rounds < 4 || log_rounds > 30)
throw new IllegalArgumentException ("Bad number of rounds");
rounds = 1 << log_rounds;
if (salt.length != BCRYPT_SALT_LEN)
throw new IllegalArgumentException ("Bad salt length");

init_key();
ekskey(salt, password);
for (i = 0; i < rounds; i++) {
for (i = 0; i != rounds; i++) {
key(password);
key(salt);
}
Expand Down Expand Up @@ -679,14 +681,19 @@ public static String hashpw(String password, String salt) {
saltb = decode_base64(real_salt, BCRYPT_SALT_LEN);

B = new BCrypt();
hashed = B.crypt_raw(passwordb, saltb, rounds);
hashed = B.crypt_raw(passwordb, saltb, rounds,
(int[])bf_crypt_ciphertext.clone());

rs.append("$2");
if (minor >= 'a')
rs.append(minor);
rs.append("$");
if (rounds < 10)
rs.append("0");
if (rounds > 30) {
throw new IllegalArgumentException(
"rounds exceeds maximum (30)");
}
rs.append(Integer.toString(rounds));
rs.append("$");
rs.append(encode_base64(saltb, saltb.length));
Expand All @@ -712,6 +719,10 @@ public static String gensalt(int log_rounds, SecureRandom random) {
rs.append("$2a$");
if (log_rounds < 10)
rs.append("0");
if (log_rounds > 30) {
throw new IllegalArgumentException(
"log_rounds exceeds maximum (30)");
}
rs.append(Integer.toString(log_rounds));
rs.append("$");
rs.append(encode_base64(rnd, rnd.length));
Expand Down Expand Up @@ -747,6 +758,20 @@ public static String gensalt() {
* @return true if the passwords match, false otherwise
*/
public static boolean checkpw(String plaintext, String hashed) {
Copy link
Author

@reedloden reedloden Feb 12, 2015

Choose a reason for hiding this comment

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

@Oscil8, does the below change end up fixing #82? Haven't had a chance to test yet, but if so, awesome...

Copy link

@Oscil8 Oscil8 Feb 13, 2015

Choose a reason for hiding this comment

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

The problem with character encoding has already failed by this point -- JRuby => Java conversion of the string has already turned out of range characters into 0xfffd (Replacement character); and the use of getBytes("UTF-8") at https://github.com/reedloden/bcrypt-ruby/blob/jBCrypt-0.4/ext/jruby/bcrypt_jruby/BCrypt.java#L676 further distorts the byte values. We've implemented a workaround here https://github.com/lookout/bcrypt-ruby/tree/lookout

return (hashed.compareTo(hashpw(plaintext, hashed)) == 0);
byte hashed_bytes[];
byte try_bytes[];
try {
String try_pw = hashpw(plaintext, hashed);
hashed_bytes = hashed.getBytes("UTF-8");
try_bytes = try_pw.getBytes("UTF-8");
} catch (UnsupportedEncodingException uee) {
return false;
}
if (hashed_bytes.length != try_bytes.length)
return false;
byte ret = 0;
for (int i = 0; i < try_bytes.length; i++)
ret |= hashed_bytes[i] ^ try_bytes[i];
return ret == 0;
}
}
5 changes: 4 additions & 1 deletion spec/TestBCrypt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

package bcrypt_jruby;

import bcrypt_jruby.BCrypt;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -178,7 +181,7 @@ public void testCheckpw_failure() {
*/
public void testInternationalChars() {
System.out.print("BCrypt.hashpw w/ international chars: ");
String pw1 = "ππππππππ";
String pw1 = "\u2605\u2605\u2605\u2605\u2605\u2605\u2605\u2605";
String pw2 = "????????";

String h1 = BCrypt.hashpw(pw1, BCrypt.gensalt());
Expand Down