Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix bug where non-salts compared to true.

The Compare functions were taking any data in without validating it as
acceptable input. This means that a "hash" could have been invalid.
Instead of checking this in advance, the code just pushed forward with
hashing and didn't look back. This would result in behavior where a
password of ':' would compare positively to a hash of 'blarp'.

Now, the c++ code just makes sure to validate that the hash is as least
a correctly formatted salt before continuing on. This doesn't give away
any special properties of the salt itself so it shouldn't be an issue in
terms of security.

Signed-off-by: Nick Campbell <nicholas.j.campbell@gmail.com>
  • Loading branch information...
commit d413fe00c14b79e16a05ba87f4690c111677017a 1 parent 5b95dfb
@ncb000gt authored
Showing with 36 additions and 6 deletions.
  1. +1 −1  package.json
  2. +11 −5 src/bcrypt_node.cc
  3. +12 −0 test/async.js
  4. +12 −0 test/sync.js
View
2  package.json
@@ -11,7 +11,7 @@
"crypto"
],
"main": "./bcrypt",
- "version": "0.7.4",
+ "version": "0.7.5",
"author": "Nick Campbell (http://github.com/ncb000gt)",
"engines": {
"node": ">= 0.6.0"
View
16 src/bcrypt_node.cc
@@ -361,7 +361,7 @@ bool CompareStrings(const char* s1, const char* s2) {
int s2_len = strlen(s2);
if (s1_len != s2_len) {
- eq = false;
+ return false;
@defunctzombie Collaborator

Um, is this not introducing a previously closed bug with timing attacks?

@ncb000gt Owner

Sum bitch. I thought I changed that line back.

That's what I get for doing this while on cold meds...thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
const int max_len = (s2_len < s1_len) ? s1_len : s2_len;
@@ -381,8 +381,10 @@ void CompareAsync(uv_work_t* req) {
compare_baton* baton = static_cast<compare_baton*>(req->data);
char bcrypted[_PASSWORD_LEN];
- bcrypt(baton->input.c_str(), baton->encrypted.c_str(), bcrypted);
- baton->result = CompareStrings(bcrypted, baton->encrypted.c_str());
+ if (ValidateSalt(baton->encrypted.c_str())) {
+ bcrypt(baton->input.c_str(), baton->encrypted.c_str(), bcrypted);
+ baton->result = CompareStrings(bcrypted, baton->encrypted.c_str());
+ }
}
void CompareAsyncAfter(uv_work_t* req) {
@@ -439,8 +441,12 @@ Handle<Value> CompareSync(const Arguments& args) {
String::Utf8Value hash(args[1]->ToString());
char bcrypted[_PASSWORD_LEN];
- bcrypt(*pw, *hash, bcrypted);
- return Boolean::New(CompareStrings(bcrypted, *hash));
+ if (ValidateSalt(*hash)) {
+ bcrypt(*pw, *hash, bcrypted);
+ return Boolean::New(CompareStrings(bcrypted, *hash));
+ } else {
+ return Boolean::New(false);
+ }
}
Handle<Value> GetRounds(const Arguments& args) {
View
12 test/async.js
@@ -133,5 +133,17 @@ module.exports = {
assert.done();
});
});
+ },
+ test_hash_compare_invalid_strings: function(assert) {
+ var fullString = 'envy1362987212538';
+ var hash = '$2a$10$XOPbrlUPQdwdJUpSrIF6X.LbE14qsMmKGhM1A8W9iqaG3vv1BD7WC';
+ var wut = ':';
+ bcrypt.compare(fullString, hash, function(err, res) {
+ assert.ok(res);
+ bcrypt.compare(fullString, wut, function(err, res) {
+ assert.ok(!res);
+ assert.done();
+ });
+ });
}
};
View
12 test/sync.js
@@ -95,6 +95,18 @@ module.exports = {
assert.ok(!(bcrypt.compareSync("password", "")), "These hashes should not be equal.");
assert.done();
},
+ test_hash_compare_invalid_strings: function(assert) {
+ var fullString = 'envy1362987212538';
+ var hash = '$2a$10$XOPbrlUPQdwdJUpSrIF6X.LbE14qsMmKGhM1A8W9iqaG3vv1BD7WC';
+ var wut = ':';
+ bcrypt.compareSync(fullString, hash, function(err, res) {
+ assert.ok(res);
+ });
+ bcrypt.compareSync(fullString, wut, function(err, res) {
+ assert.ok(!res)
+ });
+ assert.done();
+ },
test_getRounds: function(assert) {
var hash = bcrypt.hashSync("test", bcrypt.genSaltSync(9));
assert.equals(9, bcrypt.getRounds(hash), "getRounds can't extract rounds");
@defunctzombie

Um, is this not introducing a previously closed bug with timing attacks?

@ncb000gt

Sum bitch. I thought I changed that line back.

That's what I get for doing this while on cold meds...thanks.

Please sign in to comment.
Something went wrong with that request. Please try again.