From ebbb1d05d2be246231378915aa74620784ff6a3f Mon Sep 17 00:00:00 2001 From: Danny Coates Date: Thu, 14 Mar 2019 21:33:18 -0700 Subject: [PATCH] use crypto.timingSafeEqual in hmac and ownerToken authentication --- package-lock.json | 207 ++++++++++++++++++++++++++++++++---- server/middleware/auth.js | 9 +- test/backend/owner-tests.js | 10 +- 3 files changed, 201 insertions(+), 25 deletions(-) diff --git a/package-lock.json b/package-lock.json index fac62dce..708592c8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2413,8 +2413,7 @@ "buffer-crc32": { "version": "0.2.13", "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", - "integrity": "sha1-DTM+PwDqxQqhRUq9MO+MKl2ackI=", - "dev": true + "integrity": "sha1-DTM+PwDqxQqhRUq9MO+MKl2ackI=" }, "buffer-equal-constant-time": { "version": "1.0.1", @@ -5863,8 +5862,7 @@ "fs-constants": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/fs-constants/-/fs-constants-1.0.0.tgz", - "integrity": "sha512-y6OAwoSIf7FyjMIv94u+b5rdheZEjzR63GTyZJm5qh4Bi+2YgwLCcI/fPFZkL5PSixOt6ZNKm+w+Hfp/Bciwow==", - "dev": true + "integrity": "sha512-y6OAwoSIf7FyjMIv94u+b5rdheZEjzR63GTyZJm5qh4Bi+2YgwLCcI/fPFZkL5PSixOt6ZNKm+w+Hfp/Bciwow==" }, "fs-extra": { "version": "4.0.3", @@ -8054,8 +8052,7 @@ "isexe": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", - "integrity": "sha1-6PvzdNxVb/iUehDcsFctYz8s+hA=", - "dev": true + "integrity": "sha1-6PvzdNxVb/iUehDcsFctYz8s+hA=" }, "isobject": { "version": "3.0.1", @@ -8305,6 +8302,15 @@ "integrity": "sha512-s5kLOcnH0XqDO+FvuaLX8DDjZ18CGFk7VygH40QoKPUQhW4e2rvM0rwUq0t8IQDOwYSeLK01U90OjzBTme2QqA==", "dev": true }, + "klaw": { + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/klaw/-/klaw-1.3.1.tgz", + "integrity": "sha1-QIhDO0azsbolnXh4XY6W9zugJDk=", + "dev": true, + "requires": { + "graceful-fs": "^4.1.9" + } + }, "known-css-properties": { "version": "0.11.0", "resolved": "https://registry.npmjs.org/known-css-properties/-/known-css-properties-0.11.0.tgz", @@ -9616,8 +9622,7 @@ "nice-try": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/nice-try/-/nice-try-1.0.5.tgz", - "integrity": "sha512-1nh45deeb5olNY7eX82BkPO7SSxR5SSYJiPTrTdFUVYwAl8CKMA5N9PjTYkHiRjisVcxcQ1HXdLhx2qxxJzLNQ==", - "dev": true + "integrity": "sha512-1nh45deeb5olNY7eX82BkPO7SSxR5SSYJiPTrTdFUVYwAl8CKMA5N9PjTYkHiRjisVcxcQ1HXdLhx2qxxJzLNQ==" }, "nise": { "version": "1.4.10", @@ -11426,8 +11431,7 @@ "path-key": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/path-key/-/path-key-2.0.1.tgz", - "integrity": "sha1-QRyttXTFoUDTpLGRDUDYDMn0C0A=", - "dev": true + "integrity": "sha1-QRyttXTFoUDTpLGRDUDYDMn0C0A=" }, "path-parse": { "version": "1.0.6", @@ -11475,8 +11479,7 @@ "pend": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/pend/-/pend-1.2.0.tgz", - "integrity": "sha1-elfrVQpng/kRUzH89GY9XI4AelA=", - "dev": true + "integrity": "sha1-elfrVQpng/kRUzH89GY9XI4AelA=" }, "perfectionist": { "version": "2.4.0", @@ -12778,8 +12781,7 @@ "progress": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/progress/-/progress-2.0.3.tgz", - "integrity": "sha512-7PiHtLll5LdnKIMw100I+8xJXR5gW2QwWYkT6iJva0bXitZKa/XMrSbdmg3r2Xnaidz9Qumd0VPaMrZlF9V9sA==", - "dev": true + "integrity": "sha512-7PiHtLll5LdnKIMw100I+8xJXR5gW2QwWYkT6iJva0bXitZKa/XMrSbdmg3r2Xnaidz9Qumd0VPaMrZlF9V9sA==" }, "promise-inflight": { "version": "1.0.1", @@ -12915,6 +12917,15 @@ "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.1.tgz", "integrity": "sha512-tgp+dl5cGk28utYktBsrFqA7HKgrhgPsg6Z/EfhWI4gl1Hwq8B/GmY/0oXZ6nF8hDVesS/FpnYaD/kOWhYQvyg==", "dev": true + }, + "ws": { + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/ws/-/ws-6.2.0.tgz", + "integrity": "sha512-deZYUNlt2O4buFCa3t5bKLf8A7FPP/TVjwOeVNpw818Ma5nk4MLXls2eoEGS39o8119QIYxTrTDoPQ5B/gTD6w==", + "dev": true, + "requires": { + "async-limiter": "~1.0.0" + } } } }, @@ -13840,6 +13851,130 @@ "integrity": "sha1-Yl2GWPhlr0Psliv8N2o3NZpJlMo=", "dev": true }, + "selenium-standalone": { + "version": "6.16.0", + "resolved": "https://registry.npmjs.org/selenium-standalone/-/selenium-standalone-6.16.0.tgz", + "integrity": "sha512-tl7HFH2FOxJD1is7Pzzsl0pY4vuePSdSWiJdPn+6ETBkpeJDiuzou8hBjvWYWpD+eIVcOrmy3L0R3GzkdHLzDw==", + "requires": { + "async": "^2.6.2", + "commander": "^2.19.0", + "cross-spawn": "^6.0.5", + "debug": "^4.1.1", + "lodash": "^4.17.11", + "minimist": "^1.2.0", + "mkdirp": "^0.5.1", + "progress": "2.0.3", + "request": "2.88.0", + "tar-stream": "2.0.0", + "urijs": "^1.19.1", + "which": "^1.3.1", + "yauzl": "^2.10.0" + }, + "dependencies": { + "async": { + "version": "2.6.2", + "resolved": "https://registry.npmjs.org/async/-/async-2.6.2.tgz", + "integrity": "sha512-H1qVYh1MYhEEFLsP97cVKqCGo7KfCyTt6uEWqsTBr9SO84oK9Uwbyd/yCW+6rKJLHksBNUVWZDAjfS+Ccx0Bbg==", + "requires": { + "lodash": "^4.17.11" + } + }, + "bl": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/bl/-/bl-2.2.0.tgz", + "integrity": "sha512-wbgvOpqopSr7uq6fJrLH8EsvYMJf9gzfo2jCsL2eTy75qXPukA4pCgHamOQkZtY5vmfVtjB+P3LNlMHW5CEZXA==", + "requires": { + "readable-stream": "^2.3.5", + "safe-buffer": "^5.1.1" + }, + "dependencies": { + "readable-stream": { + "version": "2.3.6", + "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", + "requires": { + "core-util-is": "~1.0.0", + "inherits": "~2.0.3", + "isarray": "~1.0.0", + "process-nextick-args": "~2.0.0", + "safe-buffer": "~5.1.1", + "string_decoder": "~1.1.1", + "util-deprecate": "~1.0.1" + } + } + } + }, + "commander": { + "version": "2.19.0", + "resolved": "https://registry.npmjs.org/commander/-/commander-2.19.0.tgz", + "integrity": "sha512-6tvAOO+D6OENvRAh524Dh9jcfKTYDQAqvqezbCW82xj5X0pSrcpxtvRKHLG0yBY6SD7PSDrJaj+0AiOcKVd1Xg==" + }, + "cross-spawn": { + "version": "6.0.5", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-6.0.5.tgz", + "integrity": "sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==", + "requires": { + "nice-try": "^1.0.4", + "path-key": "^2.0.1", + "semver": "^5.5.0", + "shebang-command": "^1.2.0", + "which": "^1.2.9" + } + }, + "debug": { + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/debug/-/debug-4.1.1.tgz", + "integrity": "sha512-pYAIzeRo8J6KPEaJ0VWOh5Pzkbw/RetuzehGM7QRRX5he4fPHx2rdKMB256ehJCkX+XRQm16eZLqLNS8RSZXZw==", + "requires": { + "ms": "^2.1.1" + } + }, + "fd-slicer": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/fd-slicer/-/fd-slicer-1.1.0.tgz", + "integrity": "sha1-JcfInLH5B3+IkbvmHY85Dq4lbx4=", + "requires": { + "pend": "~1.2.0" + } + }, + "ms": { + "version": "2.1.1", + "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.1.tgz", + "integrity": "sha512-tgp+dl5cGk28utYktBsrFqA7HKgrhgPsg6Z/EfhWI4gl1Hwq8B/GmY/0oXZ6nF8hDVesS/FpnYaD/kOWhYQvyg==" + }, + "readable-stream": { + "version": "3.2.0", + "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-3.2.0.tgz", + "integrity": "sha512-RV20kLjdmpZuTF1INEb9IA3L68Nmi+Ri7ppZqo78wj//Pn62fCoJyV9zalccNzDD/OuJpMG4f+pfMl8+L6QdGw==", + "requires": { + "inherits": "^2.0.3", + "string_decoder": "^1.1.1", + "util-deprecate": "^1.0.1" + } + }, + "tar-stream": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/tar-stream/-/tar-stream-2.0.0.tgz", + "integrity": "sha512-n2vtsWshZOVr/SY4KtslPoUlyNh06I2SGgAOCZmquCEjlbV/LjY2CY80rDtdQRHFOYXNlgBDo6Fr3ww2CWPOtA==", + "requires": { + "bl": "^2.2.0", + "end-of-stream": "^1.4.1", + "fs-constants": "^1.0.0", + "inherits": "^2.0.3", + "readable-stream": "^3.1.1" + } + }, + "yauzl": { + "version": "2.10.0", + "resolved": "https://registry.npmjs.org/yauzl/-/yauzl-2.10.0.tgz", + "integrity": "sha1-x+sXyT4RLLEIb6bY5R+wZnt5pfk=", + "requires": { + "buffer-crc32": "~0.2.3", + "fd-slicer": "~1.1.0" + } + } + } + }, "selfsigned": { "version": "1.10.4", "resolved": "https://registry.npmjs.org/selfsigned/-/selfsigned-1.10.4.tgz", @@ -14000,7 +14135,6 @@ "version": "1.2.0", "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-1.2.0.tgz", "integrity": "sha1-RKrGW2lbAzmJaMOfNj/uXer98eo=", - "dev": true, "requires": { "shebang-regex": "^1.0.0" } @@ -14008,8 +14142,7 @@ "shebang-regex": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-1.0.0.tgz", - "integrity": "sha1-2kL0l0DAtC2yypcoVxyxkMmO/qM=", - "dev": true + "integrity": "sha1-2kL0l0DAtC2yypcoVxyxkMmO/qM=" }, "shell-quote": { "version": "1.6.1", @@ -16199,6 +16332,11 @@ "punycode": "^2.1.0" } }, + "urijs": { + "version": "1.19.1", + "resolved": "https://registry.npmjs.org/urijs/-/urijs-1.19.1.tgz", + "integrity": "sha512-xVrGVi94ueCJNrBSTjWqjvtgvl3cyOTThp2zaMaFNGp3F542TR6sM3f2o8RqZl+AwteClSVmoCyt0ka4RjQOQg==" + }, "urix": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/urix/-/urix-0.1.0.tgz", @@ -16576,6 +16714,40 @@ "sauce-connect-launcher": "~1.2.3" } }, + "wdio-selenium-standalone-service": { + "version": "0.0.12", + "resolved": "https://registry.npmjs.org/wdio-selenium-standalone-service/-/wdio-selenium-standalone-service-0.0.12.tgz", + "integrity": "sha512-R8iUL30SkFfZictAG5wRofeCsHQ4bIucDtaArCQWZkUqS+DlGTStIk3TaIOCaX7dS7UW1YN/lJt9Vsn4Ekmoxg==", + "dev": true, + "requires": { + "fs-extra": "^0.30.0", + "selenium-standalone": "^6.15.4" + }, + "dependencies": { + "fs-extra": { + "version": "0.30.0", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-0.30.0.tgz", + "integrity": "sha1-8jP/zAjU2n1DLapEl3aYnbHfk/A=", + "dev": true, + "requires": { + "graceful-fs": "^4.1.2", + "jsonfile": "^2.1.0", + "klaw": "^1.0.0", + "path-is-absolute": "^1.0.0", + "rimraf": "^2.2.8" + } + }, + "jsonfile": { + "version": "2.4.0", + "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-2.4.0.tgz", + "integrity": "sha1-NzaitCi4e72gzIO1P6PWM6NcKug=", + "dev": true, + "requires": { + "graceful-fs": "^4.1.6" + } + } + } + }, "wdio-spec-reporter": { "version": "0.1.5", "resolved": "https://registry.npmjs.org/wdio-spec-reporter/-/wdio-spec-reporter-0.1.5.tgz", @@ -17284,7 +17456,6 @@ "version": "1.3.1", "resolved": "https://registry.npmjs.org/which/-/which-1.3.1.tgz", "integrity": "sha512-HxJdYWq1MTIQbJ3nw0cqssHoTNU267KlrDuGZ1WYlxDStUtKUhOaJmh112/TZmHxxUfuJqPXSOm7tDyas0OSIQ==", - "dev": true, "requires": { "isexe": "^2.0.0" } diff --git a/server/middleware/auth.js b/server/middleware/auth.js index aae1b877..133b0992 100644 --- 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 index 380e3ce7..8e2ff9af 100644 --- 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);