Просмотр исходного кода

Merge pull request #11508 from overleaf/jk-password-disallow-substring

[web] Metric for passwords too similar to email

GitOrigin-RevId: cf8320fc3c9561b4dc6d54a3e97db96400ece2a9
June Kelly 3 лет назад
Родитель
Сommit
c4ecded316

+ 51 - 0
services/web/app/src/Features/Authentication/AuthenticationManager.js

@@ -14,10 +14,21 @@ const util = require('util')
 const HaveIBeenPwned = require('./HaveIBeenPwned')
 const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 const logger = require('@overleaf/logger')
+const DiffHelper = require('../Helpers/DiffHelper')
 const Metrics = require('@overleaf/metrics')
 
 const BCRYPT_ROUNDS = Settings.security.bcryptRounds || 12
 const BCRYPT_MINOR_VERSION = Settings.security.bcryptMinorVersion || 'a'
+const MAX_SIMILARITY = 0.7
+
+function _exceedsMaximumLengthRatio(password, maxSimilarity, value) {
+  const passwordLength = password.length
+  const lengthBoundSimilarity = (maxSimilarity / 2) * passwordLength
+  const valueLength = value.length
+  return (
+    passwordLength >= 10 * valueLength && valueLength < lengthBoundSimilarity
+  )
+}
 
 const _checkWriteResult = function (result, callback) {
   // for MongoDB
@@ -203,6 +214,7 @@ const AuthenticationManager = {
       })
     }
     if (typeof email === 'string' && email !== '') {
+      // TODO: remove this check once the password-too-similar check below is active
       const startOfEmail = email.split('@')[0]
       if (
         password.indexOf(email) !== -1 ||
@@ -213,6 +225,18 @@ const AuthenticationManager = {
           info: { code: 'contains_email' },
         })
       }
+      try {
+        const passwordTooSimilarError =
+          AuthenticationManager._validatePasswordNotTooSimilar(password, email)
+        if (passwordTooSimilarError) {
+          Metrics.inc('password-too-similar-to-email')
+        }
+      } catch (error) {
+        logger.error(
+          { error },
+          'error while checking password similarity to email'
+        )
+      }
     }
     return null
   },
@@ -342,6 +366,33 @@ const AuthenticationManager = {
     }
     return true
   },
+
+  /**
+   * Check if the password is similar to (parts of) the email address.
+   * For now, this merely sends a metric when the password and
+   * email address are deemed to be too similar to each other.
+   * Later we will reject passwords that fail this check.
+   *
+   * This logic was borrowed from the django project:
+   * https://github.com/django/django/blob/fa3afc5d86f1f040922cca2029d6a34301597a70/django/contrib/auth/password_validation.py#L159-L214
+   */
+  _validatePasswordNotTooSimilar(password, email) {
+    password = password.toLowerCase()
+    email = email.toLowerCase()
+    const stringsToCheck = [email].concat(email.split(/\W+/))
+    for (const emailPart of stringsToCheck) {
+      if (!_exceedsMaximumLengthRatio(password, MAX_SIMILARITY, emailPart)) {
+        const similarity = DiffHelper.stringSimilarity(password, emailPart)
+        if (similarity > MAX_SIMILARITY) {
+          logger.warn(
+            { email, emailPart, similarity, maxSimilarity: MAX_SIMILARITY },
+            'Password too similar to email'
+          )
+          return new Error('password is too similar to email')
+        }
+      }
+    }
+  },
 }
 
 AuthenticationManager.promises = {

+ 55 - 0
services/web/app/src/Features/Helpers/DiffHelper.js

@@ -0,0 +1,55 @@
+const MAX_LENGTH = 254
+
+function _calculateRatio(matches, length) {
+  if (length) {
+    const ratio = (2.0 * matches) / length
+    const rounded = Math.floor(ratio * 100) / 100
+    return rounded
+  }
+  return 1.0
+}
+
+/**
+ * Ported from python's `difflib`:
+ * https://github.com/python/cpython/blob/0415cf895f96ae3f896f1f25f0c030a820845e13/Lib/difflib.py#L622-L649
+ *
+ * Accepts two strings, `a` and `b`, and returns a float ratio
+ * corresponding (approximatey) to the overlap between the strings.
+ * Identical strings produce 1.0, completely different strings produce 0.0
+ * */
+function stringSimilarity(a, b) {
+  if (
+    typeof a !== 'string' ||
+    typeof b !== 'string' ||
+    a.length > MAX_LENGTH ||
+    b.length > MAX_LENGTH
+  ) {
+    throw new Error('Invalid input to quickMatchRatio')
+  }
+  // Count how many times each character occurs in `b`
+  const fullBCount = {}
+  b.split('').forEach(e => {
+    fullBCount[e] = (fullBCount[e] || 0) + 1
+  })
+  // avail[x] is the number of times x appears in 'b' less the
+  // number of times we've seen it in 'a' so far ... kinda
+  const avail = {}
+  let matches = 0
+  a.split('').forEach(e => {
+    let n = null
+    if (Object.hasOwn(avail, e)) {
+      n = avail[e]
+    } else {
+      n = fullBCount[e] || 0
+    }
+    avail[e] = n - 1
+    if (n > 0) {
+      matches = matches + 1
+    }
+  })
+  return _calculateRatio(matches, a.length + b.length)
+}
+
+module.exports = {
+  stringSimilarity,
+}

+ 89 - 0
services/web/test/unit/src/Authentication/AuthenticationManagerTests.js

@@ -672,6 +672,52 @@ describe('AuthenticationManager', function () {
     })
   })
 
+  describe('_validatePasswordNotTooSimilar', function () {
+    beforeEach(function () {
+      this.metrics.inc.reset()
+    })
+
+    it('should return an error when the password is too similar to email', function () {
+      const password = 'someuser1234'
+      const email = 'someuser@example.com'
+      const error = this.AuthenticationManager._validatePasswordNotTooSimilar(
+        password,
+        email
+      )
+      expect(error).to.exist
+    })
+
+    it('should return an error when the password is re-arranged elements of the email', function () {
+      const password = 'su2oe1em3re'
+      const email = 'someuser@example.com'
+      const error = this.AuthenticationManager._validatePasswordNotTooSimilar(
+        password,
+        email
+      )
+      expect(error).to.exist
+    })
+
+    it('should return nothing when the password different from email', function () {
+      const password = '58WyLvr'
+      const email = 'someuser@example.com'
+      const error = this.AuthenticationManager._validatePasswordNotTooSimilar(
+        password,
+        email
+      )
+      expect(error).to.not.exist
+    })
+
+    it('should return nothing when the password is much longer than parts of the email', function () {
+      const password = new Array(30).fill('a').join('')
+      const email = 'a@cd.com'
+      const error = this.AuthenticationManager._validatePasswordNotTooSimilar(
+        password,
+        email
+      )
+      expect(error).to.not.exist
+    })
+  })
+
   describe('setUserPassword', function () {
     beforeEach(function () {
       this.user_id = ObjectId()
@@ -813,8 +859,45 @@ describe('AuthenticationManager', function () {
       })
     })
 
+    describe('password too similar to email', function () {
+      beforeEach(function () {
+        this.user.email = 'foobarbazquux@example.com'
+        this.password = 'foobarbaz'
+        this.metrics.inc.reset()
+      })
+
+      it('should send a metric when the password is too similar to the email', function (done) {
+        this.AuthenticationManager.setUserPassword(
+          this.user,
+          this.password,
+          err => {
+            expect(err).to.not.exist
+            expect(
+              this.metrics.inc.calledWith('password-too-similar-to-email')
+            ).to.equal(true)
+            done()
+          }
+        )
+      })
+
+      it('should send a metric when the password is too similar to the email, regardless of case', function (done) {
+        this.AuthenticationManager.setUserPassword(
+          this.user,
+          this.password.toUpperCase(),
+          err => {
+            expect(err).to.not.exist
+            expect(
+              this.metrics.inc.calledWith('password-too-similar-to-email')
+            ).to.equal(true)
+            done()
+          }
+        )
+      })
+    })
+
     describe('successful password set attempt', function () {
       beforeEach(function () {
+        this.metrics.inc.reset()
         this.UserGetter.getUser = sinon.stub().yields(null, { overleaf: null })
         this.AuthenticationManager.setUserPassword(
           this.user,
@@ -843,6 +926,12 @@ describe('AuthenticationManager', function () {
         this.bcrypt.hash.calledWith(this.password, this.salt).should.equal(true)
       })
 
+      it('should not send a metric for password-too-similar-to-email', function () {
+        expect(
+          this.metrics.inc.calledWith('password-too-similar-to-email')
+        ).to.equal(false)
+      })
+
       it('should call the callback', function () {
         this.callback.called.should.equal(true)
       })

+ 28 - 0
services/web/test/unit/src/HelperFiles/DiffHelperTests.js

@@ -0,0 +1,28 @@
+const { expect } = require('chai')
+const {
+  stringSimilarity,
+} = require('../../../../app/src/Features/Helpers/DiffHelper')
+
+describe('DiffHelper', function () {
+  describe('stringSimilarity', function () {
+    it('should have a ratio of 1 for identical strings', function () {
+      expect(stringSimilarity('abcdef', 'abcdef')).to.equal(1.0)
+    })
+
+    it('should have a ratio of 0 for completely different strings', function () {
+      expect(stringSimilarity('abcdef', 'qmglzxv')).to.equal(0.0)
+    })
+
+    it('should have a ratio of between 0 and 1 for strings that are similar', function () {
+      const ratio = stringSimilarity('abcdef', 'abcdef@zxvkp')
+      expect(ratio).to.equal(0.66)
+    })
+
+    it('should reject non-string inputs', function () {
+      expect(() => stringSimilarity(1, 'abc')).to.throw
+      expect(() => stringSimilarity('abc', 2)).to.throw
+      expect(() => stringSimilarity('abc', new Array(1000).fill('a').join('')))
+        .to.throw
+    })
+  })
+})