use crypto.timingSafeEqual in hmac and ownerToken authentication

fidiego · Mar 15, 2019 · ebbb1d0 · ebbb1d0
1 parent 67b55d1
commit ebbb1d0
Show file tree

Hide file tree

Showing 3 changed files with 201 additions and 25 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/server/middleware/auth.js b/server/middleware/auth.js
@@ -1,3 +1,4 @@
+const assert = require('assert');
 const crypto = require('crypto');
 const storage = require('../storage');
 const fxa = require('../fxa');
@@ -19,7 +20,7 @@ module.exports = {
         );
         hmac.update(Buffer.from(meta.nonce, 'base64'));
         const verifyHash = hmac.digest();
-        if (verifyHash.equals(Buffer.from(auth, 'base64'))) {
+        if (crypto.timingSafeEqual(verifyHash, Buffer.from(auth, 'base64'))) {
           req.nonce = crypto.randomBytes(16).toString('base64');
           storage.setField(id, 'nonce', req.nonce);
           res.set('WWW-Authenticate', `send-v1 ${req.nonce}`);
@@ -48,7 +49,11 @@ module.exports = {
         if (!req.meta) {
           return res.sendStatus(404);
         }
-        req.authorized = req.meta.owner === ownerToken;
+        const metaOwner = Buffer.from(req.meta.owner, 'utf8');
+        const owner = Buffer.from(ownerToken, 'utf8');
+        assert(metaOwner.length > 0);
+        assert(metaOwner.length === owner.length);
+        req.authorized = crypto.timingSafeEqual(metaOwner, owner);
       } catch (e) {
         req.authorized = false;
       }

diff --git a/test/backend/owner-tests.js b/test/backend/owner-tests.js
@@ -32,7 +32,7 @@ describe('Owner Middleware', function() {
     const next = sinon.stub();
     storage.metadata.returns(Promise.resolve(null));
     const res = response();
-    await ownerMiddleware(request('x', 'y'), res);
+    await ownerMiddleware(request('a', 'y'), res, next);
     sinon.assert.notCalled(next);
     sinon.assert.calledWith(res.sendStatus, 404);
   });
@@ -42,7 +42,7 @@ describe('Owner Middleware', function() {
     const meta = { owner: 'y' };
     storage.metadata.returns(Promise.resolve(meta));
     const res = response();
-    await ownerMiddleware(request('x', null), res);
+    await ownerMiddleware(request('b', null), res, next);
     sinon.assert.notCalled(next);
     sinon.assert.calledWith(res.sendStatus, 401);
   });
@@ -52,7 +52,7 @@ describe('Owner Middleware', function() {
     const meta = { owner: 'y' };
     storage.metadata.returns(Promise.resolve(meta));
     const res = response();
-    await ownerMiddleware(request('x', 'z'), res);
+    await ownerMiddleware(request('c', 'z'), res, next);
     sinon.assert.notCalled(next);
     sinon.assert.calledWith(res.sendStatus, 401);
   });
@@ -61,7 +61,7 @@ describe('Owner Middleware', function() {
     const next = sinon.stub();
     storage.metadata.returns(Promise.reject(new Error()));
     const res = response();
-    await ownerMiddleware(request('x', 'y'), res);
+    await ownerMiddleware(request('d', 'y'), res, next);
     sinon.assert.notCalled(next);
     sinon.assert.calledWith(res.sendStatus, 401);
   });
@@ -70,7 +70,7 @@ describe('Owner Middleware', function() {
     const next = sinon.stub();
     const meta = { owner: 'y' };
     storage.metadata.returns(Promise.resolve(meta));
-    const req = request('x', 'y');
+    const req = request('e', 'y');
     const res = response();
     await ownerMiddleware(req, res, next);
     assert.equal(req.meta, meta);