diff --git a/CHANGELOG.md b/CHANGELOG.md index 322e32a1d..93cef02d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ + +## [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)) + + + # [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 index 925e1a47f..4433af4cb 100644 --- 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 index fcb702787..b9fe56bc7 100644 --- 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 index ac18659b1..2ae357e4c 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "fxa-auth-server", - "version": "1.87.0", + "version": "1.87.1", "dependencies": { "acorn": { "version": "5.0.3", diff --git a/package.json b/package.json index f7017aa03..9e82dd18f 100644 --- 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 index f981b0522..3619459b4 100644 --- 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 index 298357f1e..2ce6a37b4 100644 --- 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 index 79ad3e39b..629ccb387 100644 --- 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 index aa3e56d5a..63df7eea1 100644 --- 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 index 439dc63f6..51dd6bb74 100644 --- 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)) }) }