Merge pull request #6023 from Alexis-Loskoutoff/fix-authentication-oracle

Fix authentication oracle

backend/internal/token.js

@@ -34,18 +34,20 @@ export default {
 			.andWhere("is_disabled", 0)
 			.first();
 
-		if (!user) {
-			throw new errs.AuthError(ERROR_MESSAGE_INVALID_AUTH);
-		}
+		// An unknown, disabled or passwordless account goes through the same queries and the same password check
+		// as a real one, and fails with the same error: answering any sooner or any differently would tell anyone
+		// trying addresses which ones have an account. There is no user 0, so its lookup finds nothing. An empty
+		// secret, which creating a user through the API can store, is no hash: bcrypt would reject it at once.
+		const auth = await authModel
+			.query()
+			.where("user_id", "=", user?.id ?? 0)
+			.where("type", "=", "password")
+			.first();
 
-		const auth = await authModel.query().where("user_id", "=", user.id).where("type", "=", "password").first();
-
-		if (!auth) {
-			throw new errs.AuthError(ERROR_MESSAGE_INVALID_AUTH);
-		}
-
-		const valid = await auth.verifyPassword(data.secret);
-		if (!valid) {
+		const valid = auth?.secret
+			? await auth.verifyPassword(data.secret)
+			: await authModel.verifyDummyPassword(data.secret);
+		if (!user || !valid) {
 			throw new errs.AuthError(ERROR_MESSAGE_INVALID_AUTH, ERROR_MESSAGE_INVALID_AUTH_I18N);
 		}
 

backend/models/auth.js

@@ -1,6 +1,7 @@
 // Objection Docs:
 // http://vincit.github.io/objection.js/
 
+import crypto from "node:crypto";
 import bcrypt from "bcrypt";
 import { Model } from "objection";
 import db from "../db.js";
@@ -11,10 +12,15 @@ import User from "./user.js";
 Model.knex(db());
 
 const boolFields = ["is_deleted"];
+const saltRounds = 13;
+
+// A hash of a random value, for a login that names an account with no password to check it against. Hashed
+// with the same cost as real passwords, and made at startup so the first such login takes no longer either.
+const dummyHash = bcrypt.hash(crypto.randomBytes(32).toString("hex"), saltRounds);
 
 function encryptPassword() {
 	if (this.type === "password" && this.secret) {
-		return bcrypt.hash(this.secret, 13).then((hash) => {
+		return bcrypt.hash(this.secret, saltRounds).then((hash) => {
 			this.secret = hash;
 		});
 	}
@@ -60,6 +66,18 @@ class Auth extends Model {
 		return bcrypt.compare(password, this.secret);
 	}
 
+	/**
+	 * Take as long as verifyPassword does, then fail. bcrypt is nearly all of the time a login takes, so a login
+	 * that skipped it for an unknown account would answer far sooner, and say which accounts exist.
+	 *
+	 * @param {String} password
+	 * @returns {Promise<false>}
+	 */
+	static async verifyDummyPassword(password) {
+		await bcrypt.compare(password, await dummyHash);
+		return false;
+	}
+
 	static get name() {
 		return "Auth";
 	}

test/cypress/e2e/api/Tokens.cy.js

@@ -0,0 +1,61 @@
+/// <reference types="cypress" />
+
+describe('Tokens endpoints', () => {
+	const user = {
+		email:  'cypress@example.com',
+		secret: 'changeme',
+	};
+	const disabledUser = {
+		email:  'disabled@example.com',
+		secret: 'changeme',
+	};
+	const unknownEmail = 'nobody@example.com';
+
+	const login = (identity, secret) => {
+		return cy.request({
+			method:           'POST',
+			url:              '/api/tokens',
+			body:             {identity, secret},
+			failOnStatusCode: false,
+		});
+	};
+
+	before(() => {
+		cy.resetUsers();
+		cy.getToken().then((token) => {
+			cy.task('backendApiPost', {
+				token: token,
+				path:  '/api/users',
+				data:  {
+					name:        'Disabled User',
+					nickname:    'Disabled',
+					email:       disabledUser.email,
+					is_disabled: true,
+					auth:        {
+						type:   'password',
+						secret: disabledUser.secret,
+					},
+				},
+			});
+		});
+	});
+
+	it('Should give the same answer for an unknown email, a disabled account and a wrong password', () => {
+		// Only the error is compared: CI runs the backend in debug mode, which adds a stack trace that a real
+		// client never gets.
+		login(user.email, 'not-the-password').then((wrongPassword) => {
+			expect(wrongPassword.status).to.equal(400);
+			expect(wrongPassword.body.error.message_i18n).to.equal('error.invalid-auth');
+
+			login(unknownEmail, user.secret).then((unknown) => {
+				expect(unknown.status).to.equal(wrongPassword.status);
+				expect(unknown.body.error).to.deep.equal(wrongPassword.body.error);
+			});
+
+			login(disabledUser.email, disabledUser.secret).then((disabled) => {
+				expect(disabled.status).to.equal(wrongPassword.status);
+				expect(disabled.body.error).to.deep.equal(wrongPassword.body.error);
+			});
+		});
+	});
+});