mozilla · vladikoff · May 29, 2017 · May 26, 2017 · May 26, 2017 · May 26, 2017
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,14 @@
+<a name="1.87.1"></a>
+## [1.87.1](https://github.com/mozilla/fxa-auth-server/compare/v1.87.0...v1.87.1) (2017-05-26)
+
+
+### Bug Fixes
+
+* **push:** add extra logs ([5362c64](https://github.com/mozilla/fxa-auth-server/commit/5362c64))
+* **push:** Validate push public keys at registration time. ([8920a01](https://github.com/mozilla/fxa-auth-server/commit/8920a01))
+
+
+
 <a name="1.87.0"></a>
 # [1.87.0](https://github.com/mozilla/fxa-auth-server/compare/v1.86.0...v1.87.0) (2017-05-17)
 

diff --git a/lib/push.js b/lib/push.js
@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+var crypto = require('crypto')
+var base64url = require('base64url')
 var webpush = require('web-push')
 var P = require('./promise')
 
@@ -169,7 +171,49 @@ module.exports = function (log, db, config) {
     }, {})
   }
 
+  /**
+   * Checks whether the given string is a valid public key for push.
+   * This is a little tricky because we need to work around a bug in nodejs
+   * where using an invalid ECDH key can cause a later (unrelated) attempt
+   * to generate an RSA signature to fail:
+   *
+   *   https://github.com/nodejs/node/pull/13275
+   *
+   * @param key
+   * The public key as a b64url string.
+   */
+
+  var dummySigner = crypto.createSign('RSA-SHA256')
+  var dummyKey = Buffer.alloc(0)
+  var dummyCurve = crypto.createECDH('prime256v1')
+  dummyCurve.generateKeys()
+
+  function isValidPublicKey(publicKey) {
+    // Try to use the key in an ECDH agreement.
+    // If the key is invalid then this will throw an error.
+    try {
+      dummyCurve.computeSecret(base64url.toBuffer(publicKey))
+      return true
+    } catch (err) {
+      log.info({
+        op: 'push.isValidPublicKey',
+        name: 'Bad public key detected'
+      })
+      // However!  The above call might have left some junk
+      // sitting around on the openssl error stack.
+      // Clear it by deliberately triggering a signing error
+      // before anything yields the event loop.
+      try {
+        dummySigner.sign(dummyKey)
+      } catch (e) {}
+      return false
+    }
+  }
+
   return {
+
+    isValidPublicKey: isValidPublicKey,
+
     /**
      * Notifies all devices that there was an update to the account
      *
@@ -413,10 +457,19 @@ module.exports = function (log, db, config) {
               incrementPushAction(events.success)
             },
             function (err) {
+              // If we've stored an invalid key in the db for some reason, then we
+              // might get an encryption failure here.  Check the key, which also
+              // happens to work around bugginess in node's handling of said failures.
+              var keyWasInvalid = false
+              if (! err.statusCode && device.pushPublicKey) {
+                if (! isValidPublicKey(device.pushPublicKey)) {
+                  keyWasInvalid = true
+                }
+              }
               // 404 or 410 error from the push servers means
               // the push settings need to be reset.
               // the clients will check this and re-register push endpoints
-              if (err.statusCode === 404 || err.statusCode === 410) {
+              if (err.statusCode === 404 || err.statusCode === 410 || keyWasInvalid) {
                 // reset device push configuration
                 // Warning: this method is called without any session tokens or auth validation.
                 device.pushCallback = ''

diff --git a/lib/routes/account.js b/lib/routes/account.js
@@ -1256,6 +1256,13 @@ module.exports = (
         var payload = request.payload
         var sessionToken = request.auth.credentials
 
+        // Some additional, slightly tricky validation to detect bad public keys.
+        if (payload.pushPublicKey) {
+          if (! push.isValidPublicKey(payload.pushPublicKey)) {
+            throw error.invalidRequestParameter('invalid pushPublicKey')
+          }
+        }
+
         if (payload.id) {
           // Don't write out the update if nothing has actually changed.
           if (isSpuriousUpdate(payload, sessionToken)) {

diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "fxa-auth-server",
-  "version": "1.87.0",
+  "version": "1.87.1",
   "description": "Firefox Accounts, an identity provider for Mozilla cloud services",
   "bin": {
     "fxa-auth": "./bin/key_server.js"

diff --git a/test/local/push.js b/test/local/push.js
@@ -14,7 +14,8 @@ var fs = require('fs')
 var path = require('path')
 
 const P = require(`${ROOT_DIR}/lib/promise`)
-const mockLog = require('../mocks').mockLog
+const mocks = require('../mocks')
+const mockLog = mocks.mockLog
 var mockUid = Buffer.from('foo')
 var mockConfig = {}
 
@@ -36,7 +37,7 @@ var mockDevices = [
     'name': 'My Phone',
     'type': 'mobile',
     'pushCallback': 'https://updates.push.services.mozilla.com/update/abcdef01234567890abcdefabcdef01234567890abcdef',
-    'pushPublicKey': 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEJXwAdITiPFcSUsaRI2nlzTNRn++q36F38XrH8m8sf28DQ+2Oob5SUzvgjVS0e70pIqH6bSXDgPc8mKtSs9Zi26Q==',
+    'pushPublicKey': mocks.MOCK_PUSH_KEY,
     'pushAuthKey': 'w3b14Zjc-Afj2SDOLOyong=='
   },
   {
@@ -46,7 +47,7 @@ var mockDevices = [
     'name': 'My Desktop',
     'type': null,
     'pushCallback': 'https://updates.push.services.mozilla.com/update/d4c5b1e3f5791ef83896c27519979b93a45e6d0da34c75',
-    'pushPublicKey': 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEJXwAdITiPFcSUsaRI2nlzTNRn++q36F38XrH8m8sf28DQ+2Oob5SUzvgjVS0e70pIqH6bSXDgPc8mKtSs9Zi26Q==',
+    'pushPublicKey': mocks.MOCK_PUSH_KEY,
     'pushAuthKey': 'w3b14Zjc-Afj2SDOLOyong=='
   }
 ]
@@ -401,6 +402,88 @@ describe('push', () => {
         }
       }
 
+      var push = proxyquire(pushModulePath, mocks)(thisMockLog, mockDb, mockConfig)
+      // Careful, the argument gets modified in-place.
+      var device = JSON.parse(JSON.stringify(mockDevices[0]))
+      return push.sendPush(mockUid, [device], 'accountVerify')
+        .then(() => {
+          assert.equal(count, 1)
+        })
+    }
+  )
+
+  it(
+    'push resets device push data when a failure is caused by bad encryption keys',
+    () => {
+      var mockDb = {
+        updateDevice: sinon.spy(function () {
+          return P.resolve()
+        })
+      }
+
+      let count = 0
+      var thisMockLog = mockLog({
+        info: function (log) {
+          if (log.name === 'push.account_verify.reset_settings') {
+            // web-push failed
+            assert.equal(mockDb.updateDevice.callCount, 1, 'db.updateDevice was called once')
+            var args = mockDb.updateDevice.args[0]
+            assert.equal(args.length, 3, 'db.updateDevice was passed three arguments')
+            assert.equal(args[1], null, 'sessionTokenId argument was null')
+            count++
+          }
+        }
+      })
+
+      var mocks = {
+        'web-push': {
+          sendNotification: function (sub, payload, options) {
+            var err = new Error('Failed')
+            return P.reject(err)
+          }
+        }
+      }
+
+      var push = proxyquire(pushModulePath, mocks)(thisMockLog, mockDb, mockConfig)
+      // Careful, the argument gets modified in-place.
+      var device = JSON.parse(JSON.stringify(mockDevices[0]))
+      device.pushPublicKey = 'E' + device.pushPublicKey.substring(1) // make the key invalid
+      return push.sendPush(mockUid, [device], 'accountVerify')
+        .then(() => {
+          assert.equal(count, 1)
+        })
+    }
+  )
+
+  it(
+    'push does not reset device push data after an unexpected failure',
+    () => {
+      var mockDb = {
+        updateDevice: sinon.spy(function () {
+          return P.resolve()
+        })
+      }
+
+      let count = 0
+      var thisMockLog = mockLog({
+        info: function (log) {
+          if (log.name === 'push.account_verify.failed') {
+            // web-push failed
+            assert.equal(mockDb.updateDevice.callCount, 0, 'db.updateDevice was not called')
+            count++
+          }
+        }
+      })
+
+      var mocks = {
+        'web-push': {
+          sendNotification: function (sub, payload, options) {
+            var err = new Error('Failed')
+            return P.reject(err)
+          }
+        }
+      }
+
       var push = proxyquire(pushModulePath, mocks)(thisMockLog, mockDb, mockConfig)
       return push.sendPush(mockUid, [mockDevices[0]], 'accountVerify')
         .then(() => {
@@ -476,7 +559,14 @@ describe('push', () => {
   it(
     'notifyDeviceDisconnected calls pushToAllDevices',
     () => {
-      var push = require(pushModulePath)(mockLog(), mockDbResult, mockConfig)
+      var mocks = {
+        'web-push': {
+          sendNotification: function (sub, payload, options) {
+            return P.resolve()
+          }
+        }
+      }
+      var push = proxyquire(pushModulePath, mocks)(mockLog(), mockDbResult, mockConfig)
       sinon.spy(push, 'pushToAllDevices')
       var idToDisconnect = mockDevices[0].id
       var expectedData = {
@@ -649,7 +739,7 @@ describe('push', () => {
       var push = proxyquire(pushModulePath, mocks)(thisMockLog, mockDbResult, mockConfig)
       return push.sendPush(mockUid, mockDevices, 'accountVerify')
         .then(() => {
-          assert.equal(count, 1)
+          assert.equal(count, 2)
         })
     }
   )

diff --git a/test/local/routes/account_devices.js b/test/local/routes/account_devices.js
@@ -122,7 +122,7 @@ describe('/account/device', function () {
     payload.name = 'my even awesomer device'
     payload.type = 'phone'
     payload.pushCallback = 'https://push.services.mozilla.com/123456'
-    payload.pushPublicKey = 'SomeEncodedBinaryStuffThatDoesntGetValidedByThisTest'
+    payload.pushPublicKey = mocks.MOCK_PUSH_KEY
 
     return runTest(route, mockRequest, function (response) {
       assert.equal(mockLog.increment.callCount, 5, 'the counters were incremented')

diff --git a/test/mocks.js b/test/mocks.js
@@ -113,6 +113,7 @@ const PUSH_METHOD_NAMES = [
 ]
 
 module.exports = {
+  MOCK_PUSH_KEY: 'BDLugiRzQCANNj5KI1fAqui8ELrE7qboxzfa5K_R0wnUoJ89xY1D_SOXI_QJKNmellykaW_7U2BZ7hnrPW3A3LM',
   generateMetricsContext: generateMetricsContext,
   mockBounces: mockObject(['check']),
   mockCustoms: mockObject(CUSTOMS_METHOD_NAMES),

diff --git a/test/remote/device_tests.js b/test/remote/device_tests.js
@@ -11,6 +11,7 @@ var config = require('../../config').getProperties()
 var crypto = require('crypto')
 var base64url = require('base64url')
 var P = require('../../lib/promise')
+var mocks = require('../mocks')
 
 describe('remote device', function() {
   this.timeout(15000)
@@ -262,7 +263,7 @@ describe('remote device', function() {
         name: 'test device',
         type: 'desktop',
         pushCallback: badPushCallback,
-        pushPublicKey: base64url(Buffer.concat([Buffer.from('\x04'), crypto.randomBytes(64)])),
+        pushPublicKey: mocks.MOCK_PUSH_KEY,
         pushAuthKey: base64url(crypto.randomBytes(16))
       }
       return Client.create(config.publicUrl, email, password)
@@ -372,7 +373,7 @@ describe('remote device', function() {
         name: 'test device',
         type: 'desktop',
         pushCallback: badPushCallback,
-        pushPublicKey: base64url(Buffer.concat([Buffer.from('\x04'), crypto.randomBytes(64)])),
+        pushPublicKey: mocks.MOCK_PUSH_KEY,
         pushAuthKey: base64url(crypto.randomBytes(16))
       }
       return Client.create(config.publicUrl, email, password)
@@ -476,7 +477,7 @@ describe('remote device', function() {
         name: 'test device',
         type: 'desktop',
         pushCallback: 'https://updates.push.services.mozilla.com/qux',
-        pushPublicKey: base64url(Buffer.concat([Buffer.from('\x04'), crypto.randomBytes(64)])),
+        pushPublicKey: mocks.MOCK_PUSH_KEY,
         pushAuthKey: base64url(crypto.randomBytes(16))
       }
       return Client.create(config.publicUrl, email, password)
@@ -516,6 +517,58 @@ describe('remote device', function() {
     }
   )
 
+  it(
+    'invalid public keys are cleanly rejected',
+    () => {
+      var email = server.uniqueEmail()
+      var password = 'test password'
+      var invalidPublicKey = Buffer.alloc(65)
+      invalidPublicKey.fill('\0')
+      var deviceInfo = {
+        name: 'test device',
+        type: 'desktop',
+        pushCallback: 'https://updates.push.services.mozilla.com/qux',
+        pushPublicKey: base64url(invalidPublicKey),
+        pushAuthKey: base64url(crypto.randomBytes(16))
+      }
+      return Client.createAndVerify(config.publicUrl, email, password, server.mailbox)
+      .then(
+        function (client) {
+          return client.updateDevice(deviceInfo)
+            .then(
+              function () {
+                assert(false, 'request should have failed')
+              },
+              function (err) {
+                assert.equal(err.code, 400, 'err.code was 400')
+                assert.equal(err.errno, 107, 'err.errno was 107')
+              }
+            )
+            // A rather strange nodejs bug means that invalid push keys
+            // can cause a subsequent /certificate/sign to fail.
+            // Test that we've successfully mitigated that bug.
+            .then(
+              function () {
+                var publicKey = {
+                  'algorithm': 'RS',
+                  'n': '4759385967235610503571494339196749614544606692567785' +
+                       '7909539347682027142806529730913413168629935827890798' +
+                       '72007974809511698859885077002492642203267408776123',
+                  'e': '65537'
+                }
+                return client.sign(publicKey, 1000 * 60 * 5)
+              }
+            )
+            .then(
+              function (cert) {
+                assert.equal(typeof(cert), 'string', 'cert was successfully signed')
+              }
+            )
+        }
+      )
+    }
+  )
+
   after(() => {
     return TestServer.stop(server)
   })

diff --git a/test/remote/password_forgot_tests.js b/test/remote/password_forgot_tests.js
@@ -387,7 +387,7 @@ describe('remote password forgot', function() {
               name: 'baz',
               type: 'mobile',
               pushCallback: 'https://updates.push.services.mozilla.com/qux',
-              pushPublicKey: base64url(Buffer.concat([Buffer.from('\x04'), crypto.randomBytes(64)])),
+              pushPublicKey: mocks.MOCK_PUSH_KEY,
               pushAuthKey: base64url(crypto.randomBytes(16))
             })
           }