diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 0aa903803e..afd928d666 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -8,6 +8,7 @@ const { InvalidPasswordError, } = require('./AuthenticationErrors') const util = require('util') +const HaveIBeenPwned = require('./HaveIBeenPwned') const BCRYPT_ROUNDS = Settings.security.bcryptRounds || 12 const BCRYPT_MINOR_VERSION = Settings.security.bcryptMinorVersion || 'a' @@ -49,6 +50,7 @@ const AuthenticationManager = { return callback(err) } callback(null, user) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) } ) }) @@ -183,6 +185,7 @@ const AuthenticationManager = { return callback(updateError) } _checkWriteResult(result, callback) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) } ) }) diff --git a/services/web/app/src/Features/Authentication/HaveIBeenPwned.js b/services/web/app/src/Features/Authentication/HaveIBeenPwned.js new file mode 100644 index 0000000000..a15d9692f4 --- /dev/null +++ b/services/web/app/src/Features/Authentication/HaveIBeenPwned.js @@ -0,0 +1,110 @@ +/* + This module is operating on raw user passwords. Be very defensive. + Pay special attention when passing the password or even a hash/prefix around. + We need to ensure that no parts of it get logged or returned on either the + happy path or via an error (message or attributes). + */ + +const request = require('request-promise-native') +const crypto = require('crypto') +const Settings = require('@overleaf/settings') +const Metrics = require('@overleaf/metrics') +const logger = require('@overleaf/logger') + +const HEX_CHARS_UPPER = '1234567890ABCDEF' +const API_ERROR = new Error('cannot contact HaveIBeenPwned api') +const INVALID_PREFIX = new Error( + 'This is not a valid hex prefix. Rejecting to pass it to HaveIBeenPwned' +) +const INVALID_RESPONSE = new Error('cannot consume HaveIBeenPwned api response') +const INVALID_SCORE = new Error( + 'non integer score returned by HaveIBeenPwned api' +) +const CODED_ERROR_MESSAGES = [ + API_ERROR, + INVALID_PREFIX, + INVALID_RESPONSE, + INVALID_SCORE, +].map(err => err.message) + +async function getScoresForPrefix(prefix) { + if ( + typeof prefix !== 'string' || + prefix.length !== 5 || + Array.from(prefix).some(c => !HEX_CHARS_UPPER.includes(c)) + ) { + // Make sure we do not pass arbitrary objects to the api. + throw INVALID_PREFIX + } + try { + return await request({ + uri: `${Settings.apis.haveIBeenPwned.url}/range/${prefix}`, + headers: { + 'User-Agent': 'www.overleaf.com', + // Docs: https://haveibeenpwned.com/API/v3#PwnedPasswordsPadding + 'Add-Padding': true, + }, + timeout: Settings.apis.haveIBeenPwned.timeout, + }) + } catch (_errorWithPotentialReferenceToPrefix) { + // NOTE: Do not leak request details by passing the original error up. + throw API_ERROR + } +} + +async function isPasswordReused(password) { + const sha1 = crypto + .createHash('sha1') + .update(password) + .digest('hex') + .toUpperCase() + const prefix = sha1.slice(0, 5) + const body = await getScoresForPrefix(prefix) + + let score = 0 + try { + for (const line of body.split('\r\n')) { + const [candidate, scoreRaw] = line.split(':') + if (prefix + candidate === sha1) { + score = parseInt(scoreRaw) + break + } + } + } catch (_errorWithPotentialReferenceToHash) { + // NOTE: Do not leak password details by logging the original error. + throw INVALID_RESPONSE + } + + if (Number.isNaN(score)) { + // NOTE: Do not leak password details by logging the score. + throw INVALID_SCORE + } + return score > 0 +} + +function checkPasswordForReuseInBackground(password) { + if (!Settings.apis.haveIBeenPwned.enabled) { + return + } + + isPasswordReused(password) + .then(isReused => { + Metrics.inc('password_re_use', 1, { + status: isReused ? 're-used' : 'unique', + }) + }) + .catch(err => { + // Make sure we do not leak any password details. + if (!CODED_ERROR_MESSAGES.includes(err.message)) { + err = new Error('hidden message') + } + err = new Error(err.message) + + logger.err({ err }, 'cannot check password for re-use') + Metrics.inc('password_re_use', 1, { status: 'failure' }) + }) +} + +module.exports = { + checkPasswordForReuseInBackground, +} diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index 4112453af5..df15ba34d3 100644 --- a/services/web/config/settings.defaults.js +++ b/services/web/config/settings.defaults.js @@ -216,6 +216,13 @@ module.exports = { url: `http://${process.env.WEBPACK_HOST || 'localhost'}:3808`, }, + haveIBeenPwned: { + enabled: process.env.HAVE_I_BEEN_PWNED_ENABLED === 'true', + url: + process.env.HAVE_I_BEEN_PWNED_URL || 'https://api.pwnedpasswords.com', + timeout: parseInt(process.env.HAVE_I_BEEN_PWNED_TIMEOUT, 10) || 5 * 1000, + }, + // For legacy reasons, we need to populate the below objects. v1: {}, recurly: {}, diff --git a/services/web/test/acceptance/config/settings.test.defaults.js b/services/web/test/acceptance/config/settings.test.defaults.js index deb387061a..af9b7f99ac 100644 --- a/services/web/test/acceptance/config/settings.test.defaults.js +++ b/services/web/test/acceptance/config/settings.test.defaults.js @@ -33,6 +33,11 @@ module.exports = { user: httpAuthUser, pass: httpAuthPass, }, + + haveIBeenPwned: { + enabled: true, + url: 'http://localhost:1337', + }, }, // for registration via SL, set enableLegacyRegistration to true diff --git a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js new file mode 100644 index 0000000000..720b47c835 --- /dev/null +++ b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js @@ -0,0 +1,211 @@ +const { expect } = require('chai') +const User = require('./helpers/User').promises +const MockHaveIBeenPwnedApiClass = require('./mocks/MockHaveIBeenPwnedApi') +const { db } = require('../../../app/src/infrastructure/mongodb') +const { getMetric } = require('./helpers/metrics').promises +const sleep = require('util').promisify(setTimeout) + +let MockHaveIBeenPwnedApi +before(function () { + MockHaveIBeenPwnedApi = MockHaveIBeenPwnedApiClass.instance() +}) + +async function letPasswordCheckRunInBackground() { + await sleep(100) +} + +async function getMetricReUsed() { + return getMetric( + line => line.includes('password_re_use') && line.includes('re-used') + ) +} + +async function getMetricUnique() { + return getMetric( + line => line.includes('password_re_use') && line.includes('unique') + ) +} + +async function getMetricFailure() { + return getMetric( + line => line.includes('password_re_use') && line.includes('failure') + ) +} + +let user, previous +async function resetPassword(password) { + await user.getCsrfToken() + await user.doRequest('POST', { + url: '/user/password/reset', + form: { + email: user.email, + }, + }) + const token = ( + await db.tokens.findOne({ + 'data.user_id': user._id.toString(), + }) + ).token + + await user.doRequest('GET', { + url: `/user/password/set?passwordResetToken=${token}&email=${user.email}`, + }) + await user.doRequest('POST', { + url: '/user/password/set', + form: { + passwordResetToken: token, + password, + }, + }) +} + +describe('HaveIBeenPwnedApi', function () { + describe('login with weak password', function () { + beforeEach(function () { + user = new User() + user.password = 'aLeakedPassword42' + + // echo -n aLeakedPassword42 | sha1sum + MockHaveIBeenPwnedApi.addPasswordByHash( + 'D1ABBDEEE70CBE8BBCE5D9D039C53C0CE91C0C16' + ) + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous count', async function () { + previous = await getMetricReUsed() + }) + beforeEach('login', async function () { + await user.loginNoUpdate() + await letPasswordCheckRunInBackground() + }) + it('should track the weak password', async function () { + const after = await getMetricReUsed() + expect(after).to.equal(previous + 1) + }) + }) + + describe('login with strong password', function () { + beforeEach(function () { + user = new User() + user.password = 'this-is-a-strong-password' + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous count', async function () { + previous = await getMetricUnique() + }) + beforeEach('login', async function () { + await user.loginNoUpdate() + await letPasswordCheckRunInBackground() + }) + it('should track the strong password', async function () { + const after = await getMetricUnique() + expect(after).to.equal(previous + 1) + }) + }) + + describe('when the api is producing garbage', function () { + beforeEach(function () { + user = new User() + user.password = 'trigger-garbage-output' + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous count', async function () { + previous = await getMetricFailure() + }) + beforeEach('login', async function () { + await user.loginNoUpdate() + await letPasswordCheckRunInBackground() + }) + it('should track the failure to collect a score', async function () { + const after = await getMetricFailure() + expect(after).to.equal(previous + 1) + }) + }) + + describe('login attempt with weak password', function () { + beforeEach(function () { + user = new User() + // echo -n aLeakedPassword42 | sha1sum + MockHaveIBeenPwnedApi.addPasswordByHash( + 'D1ABBDEEE70CBE8BBCE5D9D039C53C0CE91C0C16' + ) + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous counts', async function () { + previous = { + reUsed: await getMetricReUsed(), + unique: await getMetricUnique(), + failure: await getMetricFailure(), + } + }) + beforeEach('login', async function () { + await user.loginWithEmailPassword(user.email, 'aLeakedPassword42') + await letPasswordCheckRunInBackground() + }) + it('should not increment any counter', async function () { + expect(previous).to.deep.equal({ + reUsed: await getMetricReUsed(), + unique: await getMetricUnique(), + failure: await getMetricFailure(), + }) + }) + }) + + describe('password reset with a weak password', function () { + beforeEach(function () { + user = new User() + // echo -n aLeakedPassword42 | sha1sum + MockHaveIBeenPwnedApi.addPasswordByHash( + 'D1ABBDEEE70CBE8BBCE5D9D039C53C0CE91C0C16' + ) + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous count', async function () { + previous = await getMetricReUsed() + }) + beforeEach('set password', async function () { + await resetPassword('aLeakedPassword42') + await letPasswordCheckRunInBackground() + }) + it('should track the weak password', async function () { + const after = await getMetricReUsed() + expect(after).to.equal(previous + 1) + }) + }) + + describe('password reset with a strong password', function () { + beforeEach(function () { + user = new User() + }) + beforeEach('create the user', async function () { + await user.ensureUserExists() + await letPasswordCheckRunInBackground() + }) + beforeEach('fetch previous count', async function () { + previous = await getMetricUnique() + }) + beforeEach('set password', async function () { + await resetPassword('a-strong-new-password') + await letPasswordCheckRunInBackground() + }) + it('should track the strong password', async function () { + const after = await getMetricUnique() + expect(after).to.equal(previous + 1) + }) + }) +}) diff --git a/services/web/test/acceptance/src/Init.js b/services/web/test/acceptance/src/Init.js index 33471df154..42e791272d 100644 --- a/services/web/test/acceptance/src/Init.js +++ b/services/web/test/acceptance/src/Init.js @@ -12,6 +12,7 @@ const MockProjectHistoryApi = require('./mocks/MockProjectHistoryApi') const MockSpellingApi = require('./mocks/MockSpellingApi') const MockV1Api = require('./mocks/MockV1Api') const MockV1HistoryApi = require('./mocks/MockV1HistoryApi') +const MockHaveIBeenPwnedApi = require('./mocks/MockHaveIBeenPwnedApi') const mockOpts = { debug: ['1', 'true', 'TRUE'].includes(process.env.DEBUG_MOCKS), @@ -24,6 +25,7 @@ MockDocUpdaterApi.initialize(3003, mockOpts) MockFilestoreApi.initialize(3009, mockOpts) MockNotificationsApi.initialize(3042, mockOpts) MockSpellingApi.initialize(3005, mockOpts) +MockHaveIBeenPwnedApi.initialize(1337, mockOpts) if (Features.hasFeature('saas')) { MockAnalyticsApi.initialize(3050, mockOpts) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index ce8390a43a..154d156110 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -104,29 +104,37 @@ class User { if (error != null) { return callback(error) } - this.getCsrfToken(error => { - if (error != null) { - return callback(error) - } - this.request.post( - { - url: settings.enableLegacyLogin ? '/login/legacy' : '/login', - json: { email, password: this.password }, - }, - (error, response, body) => { - if (error != null) { - return callback(error) - } - // get new csrf token, then return result of login - this.getCsrfToken(err => { - if (err) { - return callback(err) - } - callback(null, response, body) - }) + this.loginWithEmailPassword(email, this.password, callback) + }) + } + + loginNoUpdate(callback) { + this.loginWithEmailPassword(this.email, this.password, callback) + } + + loginWithEmailPassword(email, password, callback) { + this.getCsrfToken(error => { + if (error != null) { + return callback(error) + } + this.request.post( + { + url: settings.enableLegacyLogin ? '/login/legacy' : '/login', + json: { email, password: password }, + }, + (error, response, body) => { + if (error != null) { + return callback(error) } - ) - }) + // get new csrf token, then return result of login + this.getCsrfToken(err => { + if (err) { + return callback(err) + } + callback(null, response, body) + }) + } + ) }) } diff --git a/services/web/test/acceptance/src/mocks/MockHaveIBeenPwnedApi.js b/services/web/test/acceptance/src/mocks/MockHaveIBeenPwnedApi.js new file mode 100644 index 0000000000..576fdcaa79 --- /dev/null +++ b/services/web/test/acceptance/src/mocks/MockHaveIBeenPwnedApi.js @@ -0,0 +1,53 @@ +const AbstractMockApi = require('./AbstractMockApi') +const { + plainTextResponse, +} = require('../../../../app/src/infrastructure/Response') + +class MockHaveIBeenPwnedApi extends AbstractMockApi { + reset() { + this.seenPasswords = {} + } + + addPasswordByHash(hash) { + this.seenPasswords[hash] |= 0 + this.seenPasswords[hash]++ + } + + getPasswordsByRange(prefix) { + if (prefix.length !== 5) { + throw new Error('prefix must be of length 5') + } + const matches = [ + // padding + '274CCEF6AB4DFAAF86599792FA9C3FE4689:42', + '29780E39FF6511C0FC227744B2817D122F4:1337', + ] + for (const [hash, score] of Object.entries(this.seenPasswords)) { + if (hash.startsWith(prefix)) { + matches.push(hash.slice(5) + ':' + score) + } + } + return matches.join('\r\n') + } + + applyRoutes() { + this.app.get('/range/:prefix', (req, res) => { + const { prefix } = req.params + if (prefix === 'C8893') { + plainTextResponse(res, '74D74EFD7B158D2ADD283D67FF3E53B55D7:broken') + } else { + plainTextResponse(res, this.getPasswordsByRange(prefix)) + } + }) + } +} + +module.exports = MockHaveIBeenPwnedApi + +// type hint for the inherited `instance` method +/** + * @function instance + * @memberOf MockHaveIBeenPwnedApi + * @static + * @returns {MockHaveIBeenPwnedApi} + */ diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 0fa27b829d..16f3c511d0 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -23,6 +23,9 @@ describe('AuthenticationManager', function () { '@overleaf/settings': this.settings, '../User/UserGetter': (this.UserGetter = {}), './AuthenticationErrors': AuthenticationErrors, + './HaveIBeenPwned': { + checkPasswordForReuseInBackground: sinon.stub(), + }, }, }) this.callback = sinon.stub()