Use constant time string compare, fixes #29

dcodeIO · Sep 19, 2015 · 594428a · 594428a
1 parent dd6eaf7
commit 594428a
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 44 deletions.
diff --git a/bower.json b/bower.json
@@ -1,7 +1,7 @@
 {
     "name": "bcryptjs",
     "description": "Optimized bcrypt in plain JavaScript with zero dependencies.",
-    "version": "2.2.1",
+    "version": "2.2.2",
     "main": "dist/bcrypt-isaac.js",
     "license": "New-BSD",
     "homepage": "http://dcode.io/",

diff --git a/dist/bcrypt.js b/dist/bcrypt.js
@@ -194,6 +194,28 @@
             nextTick(callback.bind(this, Error("Illegal arguments: "+(typeof s)+', '+(typeof salt))));
     };
 
+    /**
+     * Compares two strings of the same length in constant time.
+     * @param {string} known Must be of the correct length
+     * @param {string} unknown Must be the same length as `known`
+     * @returns {boolean}
+     * @inner
+     */
+    function safeStringCompare(known, unknown) {
+    	var right = 0,
+    	    wrong = 0;
+    	for (var i=0, k=known.length; i<k; ++i) {
+    		if (known.charCodeAt(i) === unknown.charCodeAt(i))
+    			++right;
+    		else
+    			++wrong;
+    	}
+    	// Prevent removal of unused variables (never true, actually)
+    	if (right < 0)
+    		return false;
+    	return wrong === 0;
+    }
+
     /**
      * Synchronously tests a string against a hash.
      * @param {string} s String to compare
@@ -207,15 +229,7 @@
             throw Error("Illegal arguments: "+(typeof s)+', '+(typeof hash));
         if (hash.length !== 60)
             return false;
-        var comp = bcrypt.hashSync(s, hash.substr(0, hash.length-31)),
-            same = comp.length === hash.length,
-            max_length = (comp.length < hash.length) ? comp.length : hash.length;
-        // to prevent timing attacks, should check entire string
-        // don't exit after found to be false
-        for (var i = 0; i < max_length; ++i)
-            if (comp.length >= i && hash.length >= i && comp[i] != hash[i])
-                same = false;
-        return same;
+        return safeStringCompare(bcrypt.hashSync(s, hash.substr(0, hash.length-31)), hash);
     };
 
     /**
@@ -235,8 +249,15 @@
             nextTick(callback.bind(this, Error("Illegal arguments: "+(typeof s)+', '+(typeof hash))));
             return;
         }
+        if (hash.length !== 60) {
+        	nextTick(callback.bind(this, null, false));
+        	return;
+        }
         bcrypt.hash(s, hash.substr(0, 29), function(err, comp) {
-            callback(err, hash === comp);
+        	if (err)
+        		callback(err);
+        	else
+            	callback(null, safeStringCompare(comp, hash));
         }, progressCallback);
     };
 
@@ -1032,7 +1053,7 @@
          */
         function next() {
             if (progressCallback)
-                progressCallback(i/rounds);
+                progressCallback(i / rounds);
             if (i < rounds) {
                 var start = Date.now();
                 for (; i < rounds;) {

diff --git a/dist/bcrypt.min.js b/dist/bcrypt.min.js
diff --git a/dist/bcrypt.min.map b/dist/bcrypt.min.map
diff --git a/package.json b/package.json
@@ -1,7 +1,7 @@
 {
     "name": "bcryptjs",
     "description": "Optimized bcrypt in plain JavaScript with zero dependencies. Compatible to 'bcrypt'.",
-    "version": "2.2.1",
+    "version": "2.2.2",
     "author": "Daniel Wirtz <dcode@dcode.io>",
     "contributors": [
         "Shane Girish <shaneGirish@gmail.com> (https://github.com/shaneGirish)",

diff --git a/src/bcrypt.js b/src/bcrypt.js
@@ -154,6 +154,28 @@ bcrypt.hash = function(s, salt, callback, progressCallback) {
         nextTick(callback.bind(this, Error("Illegal arguments: "+(typeof s)+', '+(typeof salt))));
 };
 
+/**
+ * Compares two strings of the same length in constant time.
+ * @param {string} known Must be of the correct length
+ * @param {string} unknown Must be the same length as `known`
+ * @returns {boolean}
+ * @inner
+ */
+function safeStringCompare(known, unknown) {
+	var right = 0,
+	    wrong = 0;
+	for (var i=0, k=known.length; i<k; ++i) {
+		if (known.charCodeAt(i) === unknown.charCodeAt(i))
+			++right;
+		else
+			++wrong;
+	}
+	// Prevent removal of unused variables (never true, actually)
+	if (right < 0)
+		return false;
+	return wrong === 0;
+}
+
 /**
  * Synchronously tests a string against a hash.
  * @param {string} s String to compare
@@ -167,15 +189,7 @@ bcrypt.compareSync = function(s, hash) {
         throw Error("Illegal arguments: "+(typeof s)+', '+(typeof hash));
     if (hash.length !== 60)
         return false;
-    var comp = bcrypt.hashSync(s, hash.substr(0, hash.length-31)),
-        same = comp.length === hash.length,
-        max_length = (comp.length < hash.length) ? comp.length : hash.length;
-    // to prevent timing attacks, should check entire string
-    // don't exit after found to be false
-    for (var i = 0; i < max_length; ++i)
-        if (comp.length >= i && hash.length >= i && comp[i] != hash[i])
-            same = false;
-    return same;
+    return safeStringCompare(bcrypt.hashSync(s, hash.substr(0, hash.length-31)), hash);
 };
 
 /**
@@ -195,8 +209,15 @@ bcrypt.compare = function(s, hash, callback, progressCallback) {
         nextTick(callback.bind(this, Error("Illegal arguments: "+(typeof s)+', '+(typeof hash))));
         return;
     }
+    if (hash.length !== 60) {
+    	nextTick(callback.bind(this, null, false));
+    	return;
+    }
     bcrypt.hash(s, hash.substr(0, 29), function(err, comp) {
-        callback(err, hash === comp);
+    	if (err)
+    		callback(err);
+    	else
+        	callback(null, safeStringCompare(comp, hash));
     }, progressCallback);
 };
 

diff --git a/src/bcrypt/impl.js b/src/bcrypt/impl.js
@@ -419,7 +419,7 @@ function _crypt(b, salt, rounds, callback, progressCallback) {
      */
     function next() {
         if (progressCallback)
-            progressCallback(i/rounds);
+            progressCallback(i / rounds);
         if (i < rounds) {
             var start = Date.now();
             for (; i < rounds;) {