Hash collisions for UTF-8 passwords

[NicolasPelletier]

Before I start, thanks for a very good module. I like the idea of using a pure javascript code in my node application instead of having to compile native code. I don't know however if I shall use your module in my project due to the following issue.

The following code shows a hash collision between two different password ( one each a different chinese character ).

/*jslint node: true, indent: 4, stupid: true */
var bcrypt = require('bcrypt-nodejs'),
    pw1 = '\u6e2f',  // http://www.fileformat.info/info/unicode/char/6e2f/index.htm
    pw2 = '\u6f2f',  // http://www.fileformat.info/info/unicode/char/6f2f/index.htm
    salt = '$2a$05$0000000000000000000000',
    hash1 = bcrypt.hashSync(pw1, salt),
    hash2 = bcrypt.hashSync(pw2, salt);

console.log(hash1);
console.log(hash2);
if (hash1 === hash2) {
    console.log('Hash collision !!!!');
} else {
    console.log('Hashes are different, as expected.');
}

This is most probably related to the following loop in hashpw():

for (var r = 0; r < password.length; r++) {
    passwordb.push(getByte(password.charAt(r)));
}

This look goes through the password string using only 1 byte from each characters. This means that multi-byte UTF-8 characters will result in potentially the same hash if the last byte of the corresponding characters are similar.

I already opened an issue at http://code.google.com/p/javascript-bcrypt/issues/detail?id=8 since this is the basis for your implementation. I compared the results from your package to https://github.com/ncb000gt/node.bcrypt.js/ which is an implementation around the FreeBSD native C++ code and your module gives different results.

The C++ implementation uses a string as an array of UTF-8 bytes. For example, the UTF-8 of Chinese character 0x6e2F used above is three bytes: [0xE6, 0xB8, 0xAF]. If I feed that array to your algorithm, I get the same exact result as the C++ implementation.

The next step to me is to find a way to convert a UTF-8 string into the C++ equivalent array of UTF-8 bytes. I want character 0x1234 to become [0xE1 0x88 0xB4], not [0x12, 0x34]. I'll investigate that next but I wanted to let you know immediately about the problem in your module.

Regards.

[NicolasPelletier]

I have time on my hands so here is a little comparison with ncb000gt module. It demonstrate that the two modules don't give the same results. This is problematic as one may want to make sure that the implementation of bcrypt is correct in the module one uses.

/*jslint node: true, indent: 4, stupid: true */
var bcrypt = require('bcrypt'),
    bcrypt_nodejs = require('bcrypt-nodejs'),
    salt = '$2a$05$0000000000000000000000',
    pw,
    hash,
    hash_nodejs;

pw = 'ThisIsAPassword';
console.log('----- Using latin password: \"' + pw + '\" -----');
console.log('Password length in bytes: ' + Buffer.byteLength(pw));
hash = bcrypt.hashSync(pw, salt);
hash_nodejs = bcrypt_nodejs.hashSync(pw, salt);
console.log('Hash with  ncb000gt / node.bcrypt.js  :\n\t ' + hash);
console.log('Hash with  shaneGirish / bcrypt-nodejs:\n\t ' + hash_nodejs);
console.log((hash === hash_nodejs) ? 'PASSED' : 'FAILED');

pw = '\u6e2f';
console.log('----- Using utf-8 password: \"' + pw + '\" -----');
console.log('Password length in bytes: ' + Buffer.byteLength(pw));
hash = bcrypt.hashSync(pw, salt);
hash_nodejs = bcrypt_nodejs.hashSync(pw, salt);
console.log('Hash with  ncb000gt / node.bcrypt.js  :\n\t ' + hash);
console.log('Hash with  shaneGirish / bcrypt-nodejs:\n\t ' + hash_nodejs);
console.log((hash === hash_nodejs) ? 'PASSED' : 'FAILED');

I don't know yet how to submit a patch so I'll work on that next, but for the moment, here is a fix which make the above code pass both tests.

function hashpw(password, salt, progress) {
   ...
    // Old code
    //    for (var r = 0; r < password.length; r++) {
    //        passwordb.push(getByte(password.charAt(r)));
    //    }
    //
    // New working code.
    var buf = new Buffer(password);
    for (var r = 0; r < buf.length; r++) {
        passwordb.push(buf[r]);
    }

[NicolasPelletier]

I proposed a correction in #4, including a unit test demonstrating that the change works.

[shaneMangudi]

The proposed correction was merged in. Closing this issue.