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

@@ -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' },
 		]);

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

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

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

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',

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',

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

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