fix(codes): update recovery code requirements (#333), r=@philbooth

Author: vbudhram

config/config.js

@@ -168,6 +168,20 @@ module.exports = function (fs, path, url, convict) {
       default: '',
       format: 'String',
       env: 'SENTRY_DSN'
+    },
+    recoveryCodes: {
+      keyspace: {
+        doc: 'Characters allowed in a recovery code',
+        default: 'abcdefghijklmnopqrstuvwxyz0123456789',
+        format: 'String',
+        env: 'RECOVERY_CODE_KEYSPACE'
+      },
+      length: {
+        doc: 'The length of a recovery code',
+        default: 10,
+        format: 'nat',
+        env: 'RECOVERY_CODE_LENGTH'
+      }
     }
   })
 

db-server/test/backend/db_tests.js

@@ -2058,11 +2058,16 @@ module.exports = function (config, DB) {
       })
 
       const codeLengthTest = [0, 4, 8]
+      const codeTest = /^[a-zA-Z0-9]{0,20}$/
       codeLengthTest.forEach((num) => {
         it('should generate ' + num + ' recovery codes', () => {
           return db.replaceRecoveryCodes(account.uid, num)
             .then((codes) => {
               assert.equal(codes.length, num, 'correct number of codes')
+              codes.forEach((code) => {
+                assert.equal(codeTest.test(code), true, 'matches recovery code format')
+              })
+              assert.equal()
             }, (err) => {
               assert.equal(err.errno, 116, 'correct errno, not found')
             })

lib/db/mem.js

@@ -50,6 +50,9 @@ const SESSION_DEVICE_FIELDS = [
   'lastAccessTime'
 ]
 
+const RECOVERY_CODE_KEYSPACE = config.recoveryCodes.keyspace
+const RECOVERY_CODE_LENGTH = config.recoveryCodes.length
+
 module.exports = function (log, error) {
 
   function Memory(db) {}
@@ -1307,24 +1310,29 @@ module.exports = function (log, error) {
 
   Memory.prototype.replaceRecoveryCodes = function (uid, count) {
     uid = uid.toString('hex')
+    let codes = []
     return getAccountByUid(uid)
       .then(() => {
-        return dbUtil.generateRecoveryCodes(count)
-          .then((codes) => {
-            recoveryCodes[uid] = codes.map((code) => {
-              return {
-                codeHash: dbUtil.createHashSha512(code)
-              }
-            })
-            return codes
-          })
+        return dbUtil.generateRecoveryCodes(count, RECOVERY_CODE_KEYSPACE, RECOVERY_CODE_LENGTH)
+      })
+      .then((result) => {
+        codes = result
+        return codes.map((code) => dbUtil.createHashScrypt(code))
+      })
+      .all()
+      .then((hashes) => {
+        recoveryCodes[uid] = hashes.map((value) => {
+          return {
+            codeHash: value.hash,
+            salt: value.salt
+          }
+        })
+        return codes
       })
   }
 
   Memory.prototype.consumeRecoveryCode = function (uid, code) {
     uid = uid.toString('hex')
-    const codeHash = dbUtil.createHashSha512(code).toString('hex')
-
     return getAccountByUid(uid)
       .then(() => {
         const codes = recoveryCodes[uid]
@@ -1333,23 +1341,24 @@ module.exports = function (log, error) {
           throw error.notFound()
         }
 
-        let foundCode, foundIndex
-        for (let i = 0; i < codes.length; i++) {
-          const code = codes[i]
-          if (codeHash === code.codeHash.toString('hex')) {
-            foundCode = code
-            foundIndex = i
-            break
-          }
-        }
+        const hashes = codes.map((item, index) => {
+          return dbUtil.compareHashScrypt(code, item.codeHash, item.salt)
+            .then((equals) => {
+              return {index, equals}
+            })
+        })
 
-        if (! foundCode) {
-          throw error.notFound()
-        }
+        // Filter to only matching hashes
+        return P.filter(hashes, item => item.equals)
+          .then((result) => {
+            if (result.length === 0) {
+              throw error.notFound()
+            }
 
-        codes.splice(foundIndex, 1)
+            codes.splice(result[0].index, 1)
 
-        return {createdAt: foundCode.createdAt, remaining: codes.length}
+            return {remaining: codes.length}
+          })
       })
   }
 

lib/db/mysql.js

@@ -11,6 +11,7 @@ const P = require('../promise')
 
 const patch = require('./patch')
 const dbUtil = require('./util')
+const config = require('../../config')
 
 const REQUIRED_CHARSET = 'UTF8MB4_BIN'
 const DATABASE_NAME = require('../constants').DATABASE_NAME
@@ -32,6 +33,10 @@ const ER_LOCK_ABORTED = 1689
 const ER_DELETE_PRIMARY_EMAIL = 2100
 const ER_EXPIRED_TOKEN_VERIFICATION_CODE = 2101
 
+
+const RECOVERY_CODE_KEYSPACE = config.recoveryCodes.keyspace
+const RECOVERY_CODE_LENGTH = config.recoveryCodes.length
+
 module.exports = function (log, error) {
 
   var LOCK_ERRNOS = [
@@ -1451,34 +1456,30 @@ module.exports = function (log, error) {
   }
 
   const DELETE_RECOVERY_CODES = 'CALL deleteRecoveryCodes_1(?)'
-  const INSERT_RECOVERY_CODE = 'CALL createRecoveryCode_1(?, ?)'
+  const INSERT_RECOVERY_CODE = 'CALL createRecoveryCode_2(?, ?, ?)'
   MySql.prototype.replaceRecoveryCodes = function (uid, count) {
 
     // Because of the hashing requirements the process of replacing
     // recovery codes is done is two separate procedures. First one
     // deletes all current codes and the second one inserts the
     // hashed randomly generated codes.
-    return dbUtil.generateRecoveryCodes(count)
-      .then((codeList) => {
+    return dbUtil.generateRecoveryCodes(count, RECOVERY_CODE_KEYSPACE, RECOVERY_CODE_LENGTH)
+      .then((codes) => {
         return this.read(DELETE_RECOVERY_CODES, [uid])
-          .then(() => {
-            if (codeList <= 0) {
-              return P.resolve([])
-            }
-
+          .then(() => codes.map((code) => dbUtil.createHashScrypt(code)))
+          .all()
+          .then((items) => {
             const queries = []
-            codeList.forEach((code) => {
+            items.forEach((item) => {
               queries.push({
                 sql: INSERT_RECOVERY_CODE,
-                params: [uid, dbUtil.createHashSha512(code)]
+                params: [uid, item.hash, item.salt]
               })
             })
 
             return this.writeMultiple(queries)
           })
-          .then(() => {
-            return P.resolve(codeList)
-          })
+          .then(() => codes)
           .catch((err) => {
             if (err.errno === 1643) {
               throw error.notFound()
@@ -1490,9 +1491,36 @@ module.exports = function (log, error) {
       })
   }
 
-  const CONSUME_RECOVERY_CODE = 'CALL consumeRecoveryCode_1(?, ?)'
-  MySql.prototype.consumeRecoveryCode = function (uid, code) {
-    return this.readFirstResult(CONSUME_RECOVERY_CODE, [uid, dbUtil.createHashSha512(code)])
+  const CONSUME_RECOVERY_CODE = 'CALL consumeRecoveryCode_2(?, ?)'
+  const RECOVERY_CODES = 'CALL recoveryCodes_1(?)'
+  MySql.prototype.consumeRecoveryCode = function (uid, submittedCode) {
+    // Consuming a recovery code is done in a two step process because
+    // the stored scrypt hash will need to be calculated against the recovery
+    // code salt.
+    return this.readOneFromFirstResult(RECOVERY_CODES, [uid])
+      .then((results) => {
+        // Throw if this user has no recovery codes
+        if (results.length === 0) {
+          throw error.notFound()
+        }
+
+        const compareResults = results.map((code) => {
+          return dbUtil.compareHashScrypt(submittedCode, code.codeHash, code.salt)
+            .then((equals) => {
+              return {code, equals}
+            })
+        })
+
+        // Filter only matching code
+        return P.filter(compareResults, result => result.equals)
+          .map((result) => result.code)
+      })
+      .then((result) => {
+        if (result.length === 0) {
+          throw error.notFound()
+        }
+        return this.readFirstResult(CONSUME_RECOVERY_CODE, [uid, result[0].codeHash])
+      })
       .then((result) => {
         return P.resolve({
           remaining: result.count

lib/db/patch.js

@@ -4,4 +4,4 @@
 
 // The expected patch level of the database. Update if you add a new
 // patch in the ./schema/ directory.
-module.exports.level = 78
+module.exports.level = 79

lib/db/schema/patch-078-079.sql

@@ -0,0 +1,55 @@
+SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+ALTER TABLE recoveryCodes MODIFY COLUMN codeHash BINARY(32);
+
+ALTER TABLE recoveryCodes ADD COLUMN salt BINARY(32);
+
+CREATE PROCEDURE `recoveryCodes_1` (
+  IN `uidArg` BINARY(16)
+)
+BEGIN
+
+  SELECT * FROM recoveryCodes WHERE uid = uidArg;
+
+END;
+
+CREATE PROCEDURE `consumeRecoveryCode_2` (
+  IN `uidArg` BINARY(16),
+  IN `codeHashArg` BINARY(32)
+)
+BEGIN
+  DECLARE EXIT HANDLER FOR SQLEXCEPTION
+  BEGIN
+    ROLLBACK;
+    RESIGNAL;
+  END;
+
+  START TRANSACTION;
+
+  SET @deletedCount = 0;
+
+  DELETE FROM `recoveryCodes` WHERE `uid` = `uidArg` AND `codeHash` = `codeHashArg`;
+
+  SELECT ROW_COUNT() INTO @deletedCount;
+  IF @deletedCount = 0 THEN
+    SIGNAL SQLSTATE '45000' SET MYSQL_ERRNO = 1643, MESSAGE_TEXT = 'Unknown recovery code.';
+  END IF;
+
+  SELECT COUNT(*) AS count FROM `recoveryCodes` WHERE `uid` = `uidArg`;
+
+  COMMIT;
+END;
+
+CREATE PROCEDURE `createRecoveryCode_2` (
+  IN `uidArg` BINARY(16),
+  IN `codeHashArg` BINARY(32),
+  IN `saltArg` BINARY(32)
+)
+BEGIN
+
+  INSERT INTO recoveryCodes (uid, codeHash, salt, createdAt) VALUES (uidArg, codeHashArg, saltArg, NOW());
+
+END;
+
+UPDATE dbMetadata SET value = '79' WHERE name = 'schema-patch-level';
+

lib/db/schema/patch-079-078.sql

@@ -0,0 +1,12 @@
+-- SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+-- ALTER TABLE recoveryCodes MODIFY COLUMN codeHash BINARY(64),
+-- ALGORITHM = COPY, LOCK = SHARED;
+-- ALTER TABLE recoveryCodes DROP COLUMN salt BINARY(32),
+-- ALGORITHM = COPY, LOCK = SHARED;
+-- DROP recoveryCodes_1;
+-- DROP consumeRecoveryCode_2;
+-- DROP createRecoveryCode_2;
+
+-- UPDATE dbMetadata SET value = '78' WHERE name = 'schema-patch-level';
+

lib/db/util.js

@@ -7,6 +7,7 @@
 const crypto = require('crypto')
 const P = require('../promise')
 const randomBytes = P.promisify(require('crypto').randomBytes)
+const scryptHash = P.promisify(require('scrypt-hash'))
 
 const BOUNCE_TYPES = new Map([
   ['__fxa__unmapped', 0], // a bounce type we don't yet recognize
@@ -93,25 +94,44 @@ module.exports = {
     return hash.digest()
   },
 
-  createHashSha512 () {
-    const hash = crypto.createHash('sha512')
-    const args = [...arguments]
-    args.forEach((arg) => {
-      hash.update(arg)
-    })
-    return hash.digest()
+  createHashScrypt(input) {
+    // Creates an scrypt hash from string input with a randomly generated
+    // salt. This matches process on auth-server.
+    let salt
+    return randomBytes(32)
+      .then((result) => {
+        salt = result
+        const inputBuffer = Buffer.from(input)
+        return scryptHash(inputBuffer, salt, 65536, 8, 1, 32)
+          .then((hash) => {
+            return {hash, salt}
+          })
+      })
+  },
+
+  compareHashScrypt(input, verifyHash, salt) {
+    const inputBuffer = Buffer.from(input)
+    return scryptHash(inputBuffer, salt, 65536, 8, 1, 32)
+      .then((hash) => crypto.timingSafeEqual(hash, verifyHash))
   },
 
-  generateRecoveryCodes(count) {
+  generateRecoveryCodes(count, keyspace, length) {
     const randomByteCodes = []
     for (let i = 0; i < count; i++) {
-      randomByteCodes.push(randomBytes(4))
+      randomByteCodes.push(randomBytes(length))
     }
 
     return P.all(randomByteCodes)
       .then((result) => {
         return result.map((randomCode) => {
-          return randomCode.toString('hex')
+          const charsLength = keyspace.length
+          const result = []
+          let currentIndex = 0
+          for (let i = 0; i < 10; i++) {
+            currentIndex += randomCode[i]
+            result[i] = keyspace[currentIndex % charsLength]
+          }
+          return result.join('')
         })
       })
   }