From 512f6de6deba7e82ae07066a693e84e64b2246d8 Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 6 Nov 2020 08:40:00 -0500 Subject: [PATCH] feat: allow passwords with length > 73 characters (#8818) * feat: allow passwords longer than 73 characters Context: A bcrypt/blowfish limitation means that password length is capped at 72 characters. We can get around this without compromising on security by hashing all incoming passwords with SHA512, and then sending that to bcrypt. https://dropbox.tech/security/how-dropbox-securely-stores-your-passwords * feat: add additional test for passwords > 73 chars * fix: remove 'password-too-long' error message and all invocations * test: added test to show that a super long password won't bring down NodeBB * fix: remove debug log * Revert "fix: remove 'password-too-long' error message and all invocations" This reverts commit 1e312bf7ef7e119fa0f1bd3517d756ca013d5e79. * fix: added back password length checks, but at 512 chars As processing a large string still uses a lot of memory --- src/controllers/authentication.js | 4 +-- src/password.js | 12 +++++-- src/user/create.js | 5 ++- src/user/password.js | 4 +-- src/user/profile.js | 1 + src/user/reset.js | 2 +- test/password.js | 52 +++++++++++++++++++++++++++++++ test/user.js | 2 +- 8 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 test/password.js diff --git a/src/controllers/authentication.js b/src/controllers/authentication.js index 27b3d63cf6..2b59a835a0 100644 --- a/src/controllers/authentication.js +++ b/src/controllers/authentication.js @@ -94,7 +94,7 @@ authenticationController.register = async function (req, res) { throw new Error('[[user:change_password_error_match]]'); } - if (userData.password.length > 4096) { + if (userData.password.length > 512) { throw new Error('[[error:password-too-long]]'); } @@ -357,7 +357,7 @@ authenticationController.localLogin = async function (req, username, password, n return next(new Error('[[error:invalid-password]]')); } - if (password.length > 4096) { + if (password.length > 512) { return next(new Error('[[error:password-too-long]]')); } diff --git a/src/password.js b/src/password.js index fdca33f53f..d3fa3dae65 100644 --- a/src/password.js +++ b/src/password.js @@ -1,9 +1,11 @@ 'use strict'; const path = require('path'); -const bcrypt = require('bcryptjs'); +const crypto = require('crypto'); const util = require('util'); +const bcrypt = require('bcryptjs'); + const fork = require('./meta/debugFork'); function forkChild(message, callback) { @@ -19,11 +21,17 @@ function forkChild(message, callback) { const forkChildAsync = util.promisify(forkChild); exports.hash = async function (rounds, password) { + password = crypto.createHash('sha512').update(password).digest('hex'); return await forkChildAsync({ type: 'hash', rounds: rounds, password: password }); }; -exports.compare = async function (password, hash) { +exports.compare = async function (password, hash, shaWrapped) { const fakeHash = await getFakeHash(); + + if (shaWrapped) { + password = crypto.createHash('sha512').update(password).digest('hex'); + } + return await forkChildAsync({ type: 'compare', password: password, hash: hash || fakeHash }); }; diff --git a/src/user/create.js b/src/user/create.js index 69caf97877..5fb1c56ad8 100644 --- a/src/user/create.js +++ b/src/user/create.js @@ -128,7 +128,10 @@ module.exports = function (User) { } const hash = await User.hashPassword(password); await Promise.all([ - User.setUserField(uid, 'password', hash), + User.setUserFields(uid, { + password: hash, + 'password:shaWrapped': 1, + }), User.reset.updateExpiry(uid), ]); } diff --git a/src/user/password.js b/src/user/password.js index 352b73e062..acb89a6c60 100644 --- a/src/user/password.js +++ b/src/user/password.js @@ -17,7 +17,7 @@ module.exports = function (User) { User.isPasswordCorrect = async function (uid, password, ip) { password = password || ''; - var hashedPassword = await db.getObjectField('user:' + uid, 'password'); + var { password: hashedPassword, 'password:shaWrapped': shaWrapped } = await db.getObjectFields('user:' + uid, ['password', 'password:shaWrapped']); if (!hashedPassword) { // Non-existant user, submit fake hash for comparison hashedPassword = ''; @@ -25,7 +25,7 @@ module.exports = function (User) { User.isPasswordValid(password, 0); await User.auth.logAttempt(uid, ip); - const ok = await Password.compare(password, hashedPassword); + const ok = await Password.compare(password, hashedPassword, !!parseInt(shaWrapped, 10)); if (ok) { await User.auth.clearLoginAttempts(uid); } diff --git a/src/user/profile.js b/src/user/profile.js index e8f6e5e600..e2ca5f03a0 100644 --- a/src/user/profile.js +++ b/src/user/profile.js @@ -324,6 +324,7 @@ module.exports = function (User) { await Promise.all([ User.setUserFields(data.uid, { password: hashedPassword, + 'password:shaWrapped': 1, rss_token: utils.generateUUID(), }), User.reset.cleanByUid(data.uid), diff --git a/src/user/reset.js b/src/user/reset.js index a6aa113e8a..1e152e6909 100644 --- a/src/user/reset.js +++ b/src/user/reset.js @@ -70,7 +70,7 @@ UserReset.commit = async function (code, password) { const hash = await user.hashPassword(password); - await user.setUserFields(uid, { password: hash, 'email:confirmed': 1 }); + await user.setUserFields(uid, { password: hash, 'email:confirmed': 1, 'password:shaWrapped': 1 }); await groups.join('verified-users', uid); await groups.leave('unverified-users', uid); await db.deleteObjectField('reset:uid', code); diff --git a/test/password.js b/test/password.js new file mode 100644 index 0000000000..4ad6d5a7e1 --- /dev/null +++ b/test/password.js @@ -0,0 +1,52 @@ +'use strict'; + +const assert = require('assert'); +const bcrypt = require('bcryptjs'); + +const password = require('../src/password'); + +describe('Password', () => { + describe('.hash()', () => { + it('should return a password hash when called', async () => { + const hash = await password.hash(12, 'test'); + assert(hash.startsWith('$2a$')); + }); + }); + + describe('.compare()', async () => { + const salt = await bcrypt.genSalt(12); + + it('should correctly compare a password and a hash', async () => { + const hash = await password.hash(12, 'test'); + const match = await password.compare('test', hash, true); + assert(match); + }); + + it('should correctly handle comparison with no sha wrapping of the input (backwards compatibility)', async () => { + const hash = await bcrypt.hash('test', salt); + const match = await password.compare('test', hash, false); + assert(match); + }); + + it('should continue to function even with passwords > 73 characters', async () => { + const arr = []; + arr.length = 100; + const hash = await password.hash(12, arr.join('a')); + + arr.length = 150; + const match = await password.compare(arr.join('a'), hash, true); + assert.strictEqual(match, false); + }); + + it('should process a million-character long password quickly', async () => { + // ... because sha512 reduces it to a constant size + const arr = []; + const start = Date.now(); + arr.length = 1000000; + await password.hash(12, arr.join('a')); + const end = Date.now(); + + assert(end - start < 5000); + }); + }); +}); diff --git a/test/user.js b/test/user.js index c17a0a288a..101373316c 100644 --- a/test/user.js +++ b/test/user.js @@ -584,7 +584,7 @@ describe('User', function () { }, }, function (err, results) { assert.ifError(err); - Password.compare('newpassword', results.password, function (err, match) { + Password.compare('newpassword', results.password, true, function (err, match) { assert.ifError(err); assert(match); assert.strictEqual(results.userData['email:confirmed'], 1);