Merge pull request #6349 from overleaf/jpa-password-strength-checking

[web] data collection for password strength using HaveIBeenPwned api

GitOrigin-RevId: 7e4d57a979c29027fb7ca5294f3935500a0b4cf3

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)
         }
       )
     })

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,
+}

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: {},

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

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)
+    })
+  })
+})

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)

services/web/test/acceptance/src/helpers/User.js

@@ -104,6 +104,15 @@ class User {
       if (error != null) {
         return callback(error)
       }
+      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)
@@ -111,7 +120,7 @@ class User {
       this.request.post(
         {
           url: settings.enableLegacyLogin ? '/login/legacy' : '/login',
-            json: { email, password: this.password },
+          json: { email, password: password },
         },
         (error, response, body) => {
           if (error != null) {
@@ -127,7 +136,6 @@ class User {
         }
       )
     })
-    })
   }
 
   ensureUserExists(callback) {

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}
+ */

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()