From 29bb4856abe9b8c79fba6983eec1068be9695f9f Mon Sep 17 00:00:00 2001 From: Thomas Lynch Date: Thu, 10 Nov 2022 00:07:40 +1100 Subject: [PATCH] 2fa improvements - Don't allow code re-use, successfully used codes will be invalid on repeated use for the window time - Don't attach the full twofactor secret to user object in session for security. Only store a boolean if it's enabled for rendering, checks, etc. The full account should be fetched first before doTwoFactor() - Better names for some keys of twofactor redis stuff --- controllers/forms/twofactor.js | 2 +- lib/middleware/permission/sessionrefresh.js | 4 +++- lib/misc/dotwofactor.js | 15 ++++++++++++--- lib/redis/redis.js | 8 ++++++++ models/forms/changepassword.js | 2 +- models/forms/login.js | 2 +- models/forms/twofactor.js | 6 +++--- models/pages/twofactor.js | 2 +- 8 files changed, 30 insertions(+), 11 deletions(-) diff --git a/controllers/forms/twofactor.js b/controllers/forms/twofactor.js index 4030d78b..5237ffbb 100644 --- a/controllers/forms/twofactor.js +++ b/controllers/forms/twofactor.js @@ -14,7 +14,7 @@ module.exports = { controller: async (req, res, next) => { const errors = await checkSchema([ - { result: existsBody(res.locals.user.twofactor), expected: false, error: 'You already have 2FA setup' }, + { result: res.locals.user.twofactor === false, expected: true, error: 'You already have 2FA setup' }, { result: existsBody(req.body.twofactor), expected: true, error: 'Missing 2FA code' }, { result: lengthBody(req.body.twofactor, 6, 6), expected: false, error: '2FA code must be 6 characters' }, ]); diff --git a/lib/middleware/permission/sessionrefresh.js b/lib/middleware/permission/sessionrefresh.js index 86f19812..88437ef9 100644 --- a/lib/middleware/permission/sessionrefresh.js +++ b/lib/middleware/permission/sessionrefresh.js @@ -21,7 +21,9 @@ module.exports = async (req, res, next) => { 'permissions': account.permissions.toString('base64'), 'staffBoards': account.staffBoards, 'ownedBoards': account.ownedBoards, - 'twofactor': account.twofactor, + /* For security, only storing a boolean used for checks, + we dont need/want to store the twofactor secret in session */ + 'twofactor': account.twofactor != null, }; req.session.expires = new Date(Date.now() + (3 * DAY)); cache.set(`users:${req.session.user}`, res.locals.user, 3600); diff --git a/lib/misc/dotwofactor.js b/lib/misc/dotwofactor.js index e5df63b0..bd25df1d 100644 --- a/lib/misc/dotwofactor.js +++ b/lib/misc/dotwofactor.js @@ -1,14 +1,23 @@ -const OTPAuth = require('otpauth'); +const OTPAuth = require('otpauth') + , redis = require(__dirname+'/../redis/redis.js'); -module.exports = (totpSecret, userInput) => { +module.exports = async (username, totpSecret, userInput) => { const totp = new OTPAuth.TOTP({ secret: totpSecret, algorithm: 'SHA256', }); - const delta = totp.validate({ + let delta = totp.validate({ token: userInput, algorithm: 'SHA256', window: 1, }); + if (delta !== null) { + const key = `twofactor_success:${username}`; + const uses = await redis.incr(key); + redis.expire(key, 30); + if (uses && uses > 1) { + delta = null; + } + } return { totp, delta }; }; diff --git a/lib/redis/redis.js b/lib/redis/redis.js index b7505572..ecd43b5e 100644 --- a/lib/redis/redis.js +++ b/lib/redis/redis.js @@ -62,6 +62,14 @@ module.exports = { } }, + incr: (key) => { + return sharedClient.incr(key); + }, + + expire: (key, ttl) => { + return sharedClient.expire(key, ttl); + }, + //set a value on key if not exist setnx: (key, value) => { return sharedClient.setnx(key, JSON.stringify(value)); diff --git a/models/forms/changepassword.js b/models/forms/changepassword.js index d1702f05..6d37a02b 100644 --- a/models/forms/changepassword.js +++ b/models/forms/changepassword.js @@ -37,7 +37,7 @@ module.exports = async (req, res) => { } if (account.twofactor) { - const { delta } = doTwoFactor(account.twofactor, req.body.twofactor); + const { delta } = await doTwoFactor(username, account.twofactor, req.body.twofactor); if (delta === null) { return dynamicResponse(req, res, 403, 'message', { 'title': 'Forbidden', diff --git a/models/forms/login.js b/models/forms/login.js index 51346174..47a1031f 100644 --- a/models/forms/login.js +++ b/models/forms/login.js @@ -41,7 +41,7 @@ module.exports = async (req, res) => { } if (account.twofactor) { - const { delta } = doTwoFactor(account.twofactor, req.body.twofactor); + const { delta } = await doTwoFactor(username, account.twofactor, req.body.twofactor); if (delta === null) { return dynamicResponse(req, res, 403, 'message', { 'title': 'Forbidden', diff --git a/models/forms/twofactor.js b/models/forms/twofactor.js index 3dd9f1f1..de3306eb 100644 --- a/models/forms/twofactor.js +++ b/models/forms/twofactor.js @@ -10,7 +10,7 @@ module.exports = async (req, res) => { const username = res.locals.user.username.toLowerCase(); // Get the temporary secret from redis and check it exists - const tempSecret = await redis.get(`twofactor:${username}`); + const tempSecret = await redis.get(`twofactor_tempsecret:${username}`); if (!tempSecret || !username) { return dynamicResponse(req, res, 403, 'message', { 'title': 'Forbidden', @@ -20,7 +20,7 @@ module.exports = async (req, res) => { } // Validate totp - const { delta } = doTwoFactor(tempSecret, req.body.twofactor); + const { delta } = await doTwoFactor(username, tempSecret, req.body.twofactor); // Check if code was valid if (delta === null) { @@ -30,7 +30,7 @@ module.exports = async (req, res) => { 'redirect': '/twofactor.html', }); } - redis.del(`twofactor:${username}`); + redis.del(`twofactor_tempsecret:${username}`); // Successfully enabled 2FA await Accounts.updateTwofactor(username, tempSecret); diff --git a/models/pages/twofactor.js b/models/pages/twofactor.js index 85bfc2d8..5a665dd9 100644 --- a/models/pages/twofactor.js +++ b/models/pages/twofactor.js @@ -36,7 +36,7 @@ module.exports = async (req, res, next) => { }); const secret = totp.secret; secretBase32 = secret.base32; - await redis.set(`twofactor:${username}`, secretBase32, 300); //store validation secret temporarily in redis + await redis.set(`twofactor_tempsecret:${username}`, secretBase32, 300); //store validation secret temporarily in redis const qrCodeURL = totp.toString(); qrCodeText = await QRCode.toString(qrCodeURL, { type: 'utf8' }); } catch (err) {