Skip to content
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.

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

Some compatibility issue with emoji #776

Closed
pool683 opened this issue Jan 25, 2020 · 16 comments
Closed

Some compatibility issue with emoji #776

pool683 opened this issue Jan 25, 2020 · 16 comments

Comments

@pool683
Copy link

@pool683 pool683 commented Jan 25, 2020

When using 64+ emoji compatibility between bcrypt <> php and bcrypt <> bcryptjs is broken, whereas bcryptjs <> php is fine.
Code to represent the issue.

const {spawnSync} = require('child_process');
const bcrypt = require('bcrypt');
const bcryptjs = require('bcryptjs');

let chr = '😃'; // emoji
let len = 64; // 64+
let data = chr.repeat(len);

let bcryptAHash = bcrypt.hashSync(data, bcrypt.genSaltSync(8, 'a'));
let bcryptBHash = bcrypt.hashSync(data, bcrypt.genSaltSync(8, 'b'));
let bcryptjsHash = bcryptjs.hashSync(data, bcryptjs.genSaltSync(8));
let phpHash = spawnSync("php", ["-r", "echo password_hash('"+data+"', PASSWORD_BCRYPT, ['cost' => 8]);"]).stdout.toString();

let bcrypta_php = spawnSync("php", ["-r", "echo password_verify('"+data+"', str_replace('$2a$', '$2y$', '"+bcryptAHash+"')) ? 'true' : 'false';"]).stdout.toString();
let bcryptb_php = spawnSync("php", ["-r", "echo password_verify('"+data+"', str_replace('$2b$', '$2y$', '"+bcryptBHash+"')) ? 'true' : 'false';"]).stdout.toString();
let bcryptjs_php = spawnSync("php", ["-r", "echo password_verify('"+data+"', str_replace('$2a$', '$2y$', '"+bcryptjsHash+"')) ? 'true' : 'false';"]).stdout.toString();
let php_php = spawnSync("php", ["-r", "echo password_verify('"+data+"', '"+phpHash+"') ? 'true' : 'false';"]).stdout.toString();

let bcrypta_bcrypt = bcrypt.compareSync(data, bcryptAHash).toString();
let bcryptb_bcrypt = bcrypt.compareSync(data, bcryptBHash).toString();
let bcryptjs_bcrypt = bcrypt.compareSync(data, bcryptjsHash).toString();
let php_bcrypta = bcrypt.compareSync(data, phpHash.replace("$2y$", "$2a$")).toString();
let php_bcryptb = bcrypt.compareSync(data, phpHash.replace("$2y$", "$2b$")).toString();

let bcrypta_bcryptjs = bcryptjs.compareSync(data, bcryptAHash).toString();
let bcryptjs_bcryptjs = bcryptjs.compareSync(data, bcryptjsHash).toString();
let php_bcryptjs = bcryptjs.compareSync(data, phpHash.replace("$2y$", "$2a$")).toString();

console.log("hash\\module php   bcryptjs bcrypt-a bcrypt-b");
console.log("php        ", php_php.padEnd(5, " "), php_bcryptjs.padEnd(8, " "), php_bcrypta.padEnd(8, " "), php_bcryptb.padEnd(8, " "));
console.log("bcryptjs   ", bcryptjs_php.padEnd(5, " "), bcryptjs_bcryptjs.padEnd(8, " "), bcryptjs_bcrypt.padEnd(8, " "), "".padEnd(8, " "));
console.log("bcrypt-a   ", bcrypta_php.padEnd(5, " "), bcrypta_bcryptjs.padEnd(8, " "), bcrypta_bcrypt.padEnd(8, " "), "".padEnd(8, " "));
console.log("bcrypt-b   ", bcryptb_php.padEnd(5, " "), "".padEnd(8, " "), "".padEnd(8, " "), bcryptb_bcrypt.padEnd(8, " "));

The output:

hash\module php   bcryptjs bcrypt-a bcrypt-b
php         true  true     false    false   
bcryptjs    true  true     false            
bcrypt-a    false false    true             
bcrypt-b    false                   true    

Fedora 31 x86_64, bcrypt 3.0.7, node v12.13.1

@pool683
Copy link
Author

@pool683 pool683 commented Jan 26, 2020

Data is truncated wrong when its length is greater than 255 bytes.

const bcrypt = require('bcrypt');

let chr = 'abcd';
var data72 = chr.repeat(18);
var data254 = chr.repeat(63) + 'ab';
var data255 = chr.repeat(63) + 'abc';
var data256 = chr.repeat(64);
var data257 = chr.repeat(64) + 'a';
var data258 = chr.repeat(64) + 'ab';
var data259 = chr.repeat(64) + 'abc';
var data260 = chr.repeat(65);

let hash254 = bcrypt.hashSync(data254, bcrypt.genSaltSync());
let hash255 = bcrypt.hashSync(data255, bcrypt.genSaltSync());
let hash256 = bcrypt.hashSync(data256, bcrypt.genSaltSync());
let hash257 = bcrypt.hashSync(data257, bcrypt.genSaltSync());
let hash258 = bcrypt.hashSync(data258, bcrypt.genSaltSync());
let hash259 = bcrypt.hashSync(data259, bcrypt.genSaltSync());
let hash260 = bcrypt.hashSync(data260, bcrypt.genSaltSync());

let data72_hash254 = bcrypt.compareSync(data72, hash254);
let data72_hash255 = bcrypt.compareSync(data72, hash255);
let data72_hash256 = bcrypt.compareSync(data72, hash256);
let data72_hash257 = bcrypt.compareSync(data72, hash257);
let data72_hash258 = bcrypt.compareSync(data72, hash258);
let data72_hash259 = bcrypt.compareSync(data72, hash259);
let data72_hash260 = bcrypt.compareSync(data72, hash260);

console.log("data72+hash254", data72_hash254);
console.log("data72+hash255", data72_hash255);
console.log("data72+hash256", data72_hash256);
console.log("data72+hash257", data72_hash257);
console.log("data72+hash258", data72_hash258);
console.log("data72+hash259", data72_hash259);
console.log("data72+hash260", data72_hash260);
data72+hash254 true
data72+hash255 true
data72+hash256 false
data72+hash257 false
data72+hash258 false
data72+hash259 true
data72+hash260 false

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Feb 5, 2020

Can you verify if the same character encoding is being used in PHP and Node?

Loading

@pool683
Copy link
Author

@pool683 pool683 commented Feb 11, 2020

message # 1 is not relevant because of message # 2. Issue is not with encoding or emoji but with data length(64 emoji characters has 256 bytes length). When data length greater than 72 bytes(not characters) it should be truncated, and truncation works fine with data length up to 255 bytes.

As shown in message # 2 comparison data 72 bytes length with hash created from data 255 bytes(with same first 72 bytes) is true but not with data 256+ bytes length(with some exceptions...)

Loading

@techhead
Copy link
Contributor

@techhead techhead commented May 31, 2020

I see what is happening.

u_int16_t j;
u_int8_t key_len, salt_len, logr, minor;

/* strlen() returns a size_t, but the function calls
* below result in implicit casts to a narrower integer
* type, so cap key_len at the actual maximum supported
* length here to avoid integer wraparound */
key_len = strlen(key);
if (key_len > 72)
key_len = 72;
key_len++; /* include the NUL */

key_len is u_int8_t. When assigning the value of strlen(key), the value wraps around if larger than 255. This is a big problem and a security hole for any code that relies on the library to automatically truncate input!

Loading

@techhead
Copy link
Contributor

@techhead techhead commented May 31, 2020

This issue is actually what caused the minor version bump from a to b. It was just never correctly fixed here.

https://en.wikipedia.org/wiki/Bcrypt#Versioning_history
https://marc.info/?l=openbsd-misc&m=139320023202696

While working on a patch, I also noticed that the current tests are broken.

test_long_passwords: function(assert) {
// bcrypt wrap-around bug in $2a$
assert.strictEqual(bcrypt.hashSync("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.strictEqual(bcrypt.hashSync("0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
// tests for $2b$ which fixes wrap-around bugs
assert.strictEqual(bcrypt.hashSync("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234", "$2b$05$CCCCCCCCCCCCCCCCCCCCC."), "$2b$05$CCCCCCCCCCCCCCCCCCCCC.XxrQqgBi/5Sxuq9soXzDtjIZ7w5pMfK");
assert.strictEqual(bcrypt.hashSync("0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345", "$2b$05$CCCCCCCCCCCCCCCCCCCCC."), "$2b$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.done();
},

They seem to be misguided as to what they should actually be testing for.

For the $2a$ tests, they actually both cause a wrap here.

if (minor <= 'a')
key_len = (u_int8_t)(strlen(key) + (minor >= 'a' ? 1 : 0));

The first test ends up with a key_len of 0 and the second with a key_len of 1. That they both end up with the same hash is a "feature" of the bug. But you get a better picture of what's happening when you go just a little further, attempt to hash a 257 byte input and get a different output. And also when you notice that it doesn't matter what is past the first two characters.

assert.strictEqual(bcrypt.hashSync("012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.strictEqual(bcrypt.hashSync("0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.strictEqual(bcrypt.hashSync("01XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.strictEqual(bcrypt.hashSync("01XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.6.O1dLNbjod2uo0DVcW.jHucKbPDdHS");
assert.strictEqual(bcrypt.hashSync("01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456", "$2a$05$CCCCCCCCCCCCCCCCCCCCC."), "$2a$05$CCCCCCCCCCCCCCCCCCCCC.iR4w4CV15J.nwTvWxPojXRxApAByroS");

But... all of that is "okay"... maybe. It's actually expected and like I said, it's what caused the minor version bump from a to b in the first place. I'm just not sure what the tests think they are trying to accomplish.

The tests for $2b$ are obviously off the mark, as they seem to be asserting that broken behavior stays broken. The second test shows that key_len wraps back to 1 at 256 bytes, which is not the expected behavior. It should assert that the second hash is the same as the first, if the input was truncated correctly.

You can also try for yourself and see that the two inputs below generate the same hash... which is a problem.

bcrypt.hashSync(
  "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345",
  "$2b$05$CCCCCCCCCCCCCCCCCCCCC."
);
bcrypt.hashSync(
  "01XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
  "$2b$05$CCCCCCCCCCCCCCCCCCCCC."
);

Loading

@Piccirello
Copy link

@Piccirello Piccirello commented Jun 19, 2020

This should definitely be assigned a CVE. Users on v4 may not be quick to update without a prompt from npm/yarn informing them of the vulnerability.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 20, 2020

We are deprecating v4, but I don't think this warrants a CVE. The original bug in OpenBSD didn't get assigned a CVE

Loading

@Piccirello
Copy link

@Piccirello Piccirello commented Jun 20, 2020

The original bug was assigned CVE-2011-2483. In fact, the initial notification of this issue was on a mailing list with the subject "CVE request". Given the nature of this issue, a CVE is definitely warranted.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 20, 2020

The bug you have linked is a sign extension bug that affected crypt_blowfish. bcrypt is based on OpenBSD sources which are not affected by the issue. The issue was about signed chars being used for bytes in JtR sources. OpenBSD based sources including bcrypt used unsigned chars for representing bytes.

Loading

@Piccirello
Copy link

@Piccirello Piccirello commented Jun 20, 2020

You're right, I misread what the CVE was issued for. Nevertheless, I still believe this should be assigned a CVE. Do you have any justification as to why this clear security issue shouldn't be assigned a CVE, other than an example of one case where a CVE wasn't assigned? Is your stance that this isn't a security issue?

Loading

@Piccirello
Copy link

@Piccirello Piccirello commented Jun 29, 2020

Just following up on this and still trying to figure out why this hasn't been assigned a CVE. I've contacted Security@GitHub and they'll only issue a CVE to the project maintainer. Users running v4 in prod should be alerted that their password hashing library has a security flaw.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 30, 2020

@Piccirello I don't have access to create a CVE from Github as well. I'm not a maintainer. @ncb000gt I guess you can file an advisory on GitHub to alert users. I've already put a deprecation notice on the npm package.

Loading

@ncb000gt
Copy link
Member

@ncb000gt ncb000gt commented Jun 30, 2020

@agathver i've made you a maintainer on the project. let me know if you need more access than that. :)

Loading

@AdamGold
Copy link

@AdamGold AdamGold commented Jun 30, 2020

@Piccirello Hey! 👋
Adam from Snyk here. We can take care of the CVE assignment if you'd like us to, and add this vulnerability to our database.

Loading

@AdamGold
Copy link

@AdamGold AdamGold commented Jul 1, 2020

Hey folks, just updating that the vulnerability has been added to our DB: https://snyk.io/vuln/SNYK-JS-BCRYPT-572911 and was assigned CVE-2020-7689.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jul 1, 2020

Thanks @AdamGold

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants