From 8920a012d841df852b678d8cf734460c66f71384 Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Fri, 26 May 2017 14:40:04 +1000 Subject: [PATCH 1/4] fix(push): Validate push public keys at registration time. We currently allow devices to submit invalid public keys with their push registration, causing attempts to notify those devices to fail in an ugly way. This adds additional validation so that only known-good keys get stored in the db. --- lib/push.js | 49 ++++++++++++- lib/routes/account.js | 7 ++ test/local/push.js | 100 +++++++++++++++++++++++++-- test/local/routes/account_devices.js | 2 +- test/mocks.js | 1 + test/remote/device_tests.js | 59 +++++++++++++++- test/remote/password_forgot_tests.js | 2 +- 7 files changed, 209 insertions(+), 11 deletions(-) diff --git a/lib/push.js b/lib/push.js index 4e2d429cb..0897c042d 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') @@ -155,7 +157,43 @@ 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 :-( + * + * @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) { + // 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 * @@ -380,10 +418,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 0266b1418..2dc0167df 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/test/local/push.js b/test/local/push.js index 048b5e28d..bab6e900b 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': 'BCp93zru09_hab2Bg37LpTNG__Pw6eMPEP2hrQpwuytoj3h4chXpGc-3qqdKyqjuvAiEupsnOd_RLyc7erJHWgA=', + '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': 'BCp93zru09_hab2Bg37LpTNG__Pw6eMPEP2hrQpwuytoj3h4chXpGc-3qqdKyqjuvAiEupsnOd_RLyc7erJHWgA=', + '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 pushToDevice', () => { - 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, 'pushToDevice') var idToDisconnect = mockDevices[0].id var expectedData = { @@ -611,7 +701,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 30a88bc0f..e1f2624cc 100644 --- a/test/mocks.js +++ b/test/mocks.js @@ -111,6 +111,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)) }) } From 5362c64ee6e50ed8a9975364bbfa29cb7911878d Mon Sep 17 00:00:00 2001 From: Vlad Filippov Date: Fri, 26 May 2017 09:18:19 -0400 Subject: [PATCH 2/4] fix(push): add extra logs --- lib/push.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/push.js b/lib/push.js index 0897c042d..303bb7ef4 100644 --- a/lib/push.js +++ b/lib/push.js @@ -179,6 +179,10 @@ module.exports = function (log, db, config) { 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 From 90185f7740570001c41ca028c4bd328730be19c9 Mon Sep 17 00:00:00 2001 From: Vlad Filippov Date: Fri, 26 May 2017 09:21:47 -0400 Subject: [PATCH 3/4] Release v1.87.1 --- CHANGELOG.md | 11 +++++++++++ npm-shrinkwrap.json | 2 +- package.json | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) 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/npm-shrinkwrap.json b/npm-shrinkwrap.json index db796a310..4d64f246e 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": { "ajv": { "version": "4.1.7", diff --git a/package.json b/package.json index f93a0ac2a..03379e153 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" From 05034798bbc9a071b274a7a6b17f318666614df1 Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Mon, 29 May 2017 14:53:36 +1000 Subject: [PATCH 4/4] chore(push): Add a link to nodejs ECDH issue in code comments. --- lib/push.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/push.js b/lib/push.js index ca41b2454..4433af4cb 100644 --- a/lib/push.js +++ b/lib/push.js @@ -175,7 +175,9 @@ 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 :-( + * to generate an RSA signature to fail: + * + * https://github.com/nodejs/node/pull/13275 * * @param key * The public key as a b64url string.