Hash collisions for UTF-8 passwords

[shanemangudi]

Hash collisions for UTF-8 passwords

[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.