From d20b93156bb8c8793a35d128bad32dd61a0eb598 Mon Sep 17 00:00:00 2001 From: Bernd Storath Date: Wed, 27 May 2026 08:42:14 +0200 Subject: [PATCH] nullable password, prevent timing attack this prevents timing attacks by always checking hash even if there is none prevents using basic auth if 2fa is enabled --- .../migrations/0005_quiet_sentinels.sql | 2 -- .../migrations/0005_supreme_groot.sql | 22 +++++++++++++++++++ .../migrations/meta/0005_snapshot.json | 4 ++-- .../database/migrations/meta/_journal.json | 4 ++-- .../database/repositories/user/schema.ts | 3 ++- .../database/repositories/user/service.ts | 12 +++++----- src/server/utils/password.ts | 15 ++++++++++--- src/server/utils/session.ts | 15 ++++--------- 8 files changed, 49 insertions(+), 28 deletions(-) delete mode 100644 src/server/database/migrations/0005_quiet_sentinels.sql create mode 100644 src/server/database/migrations/0005_supreme_groot.sql diff --git a/src/server/database/migrations/0005_quiet_sentinels.sql b/src/server/database/migrations/0005_quiet_sentinels.sql deleted file mode 100644 index 42e41e35..00000000 --- a/src/server/database/migrations/0005_quiet_sentinels.sql +++ /dev/null @@ -1,2 +0,0 @@ -ALTER TABLE `users_table` ADD `oauth_provider` text;--> statement-breakpoint -ALTER TABLE `users_table` ADD `oauth_id` text; \ No newline at end of file diff --git a/src/server/database/migrations/0005_supreme_groot.sql b/src/server/database/migrations/0005_supreme_groot.sql new file mode 100644 index 00000000..c5265489 --- /dev/null +++ b/src/server/database/migrations/0005_supreme_groot.sql @@ -0,0 +1,22 @@ +PRAGMA foreign_keys=OFF;--> statement-breakpoint +CREATE TABLE `__new_users_table` ( + `id` integer PRIMARY KEY AUTOINCREMENT NOT NULL, + `username` text NOT NULL, + `password` text, + `email` text, + `name` text NOT NULL, + `role` integer NOT NULL, + `totp_key` text, + `totp_verified` integer NOT NULL, + `enabled` integer NOT NULL, + `oauth_provider` text, + `oauth_id` text, + `created_at` text DEFAULT (CURRENT_TIMESTAMP) NOT NULL, + `updated_at` text DEFAULT (CURRENT_TIMESTAMP) NOT NULL +); +--> statement-breakpoint +INSERT INTO `__new_users_table`("id", "username", "password", "email", "name", "role", "totp_key", "totp_verified", "enabled", "oauth_provider", "oauth_id", "created_at", "updated_at") SELECT "id", "username", "password", "email", "name", "role", "totp_key", "totp_verified", "enabled", "oauth_provider", "oauth_id", "created_at", "updated_at" FROM `users_table`;--> statement-breakpoint +DROP TABLE `users_table`;--> statement-breakpoint +ALTER TABLE `__new_users_table` RENAME TO `users_table`;--> statement-breakpoint +PRAGMA foreign_keys=ON;--> statement-breakpoint +CREATE UNIQUE INDEX `users_table_username_unique` ON `users_table` (`username`); \ No newline at end of file diff --git a/src/server/database/migrations/meta/0005_snapshot.json b/src/server/database/migrations/meta/0005_snapshot.json index 9c799dab..639d8a8f 100644 --- a/src/server/database/migrations/meta/0005_snapshot.json +++ b/src/server/database/migrations/meta/0005_snapshot.json @@ -1,7 +1,7 @@ { "version": "6", "dialect": "sqlite", - "id": "1d6d806f-441e-4f18-b84b-3e9232e45359", + "id": "840b00a9-8e9d-4bc9-a0fa-6185ebb01a46", "prevId": "0f072f91-cd10-4702-ae7b-245255d69d1e", "tables": { "clients_table": { @@ -749,7 +749,7 @@ "name": "password", "type": "text", "primaryKey": false, - "notNull": true, + "notNull": false, "autoincrement": false }, "email": { diff --git a/src/server/database/migrations/meta/_journal.json b/src/server/database/migrations/meta/_journal.json index dd0f8170..a00425fc 100644 --- a/src/server/database/migrations/meta/_journal.json +++ b/src/server/database/migrations/meta/_journal.json @@ -40,8 +40,8 @@ { "idx": 5, "version": "6", - "when": 1779787680891, - "tag": "0005_quiet_sentinels", + "when": 1779862471761, + "tag": "0005_supreme_groot", "breakpoints": true } ] diff --git a/src/server/database/repositories/user/schema.ts b/src/server/database/repositories/user/schema.ts index aea4158e..5aba80cd 100644 --- a/src/server/database/repositories/user/schema.ts +++ b/src/server/database/repositories/user/schema.ts @@ -6,7 +6,8 @@ import { client } from '../../schema'; export const user = sqliteTable('users_table', { id: int().primaryKey({ autoIncrement: true }), username: text().notNull().unique(), - password: text().notNull(), + /** `password == null` means password login disabled */ + password: text(), email: text(), name: text().notNull(), role: int().$type().notNull(), diff --git a/src/server/database/repositories/user/service.ts b/src/server/database/repositories/user/service.ts index 37ea9bcc..ea4b4877 100644 --- a/src/server/database/repositories/user/service.ts +++ b/src/server/database/repositories/user/service.ts @@ -144,7 +144,7 @@ export class UserService { // Create new user await this.#db.insert(user).values({ username, - password: '--- no password ---', + password: null, email, name, role: roles.ADMIN, @@ -233,13 +233,11 @@ export class UserService { .findFirst({ where: eq(user.username, username) }) .execute(); - if (!txUser) { - return { success: false, error: 'INCORRECT_CREDENTIALS' }; - } + // always check to avoid timing attack + const userHashPassword = txUser?.password ?? null; + const passwordValid = await isPasswordValid(password, userHashPassword); - const passwordValid = await isPasswordValid(password, txUser.password); - - if (!passwordValid) { + if (!txUser || !passwordValid) { return { success: false, error: 'INCORRECT_CREDENTIALS' }; } diff --git a/src/server/utils/password.ts b/src/server/utils/password.ts index 915795c7..0a4e0c62 100644 --- a/src/server/utils/password.ts +++ b/src/server/utils/password.ts @@ -3,13 +3,22 @@ import argon2 from 'argon2'; import { deserialize } from '@phc/format'; +const DUMMY_HASH = + '$argon2id$v=19$m=65536,t=3,p=4$jsh6z1/SbZHYAiO/Ww9HZw$ikzkoXWqc2b0Pc4O8ZNJjp1xKZSb7SNM/3dPMNUPk9Y'; + /** - * Checks if `password` matches the hash. + * Checks if `password` matches the `hash`. + * + * Checks against `DUMMY_HASH` and returns false if `hash` is null */ -export function isPasswordValid( +export async function isPasswordValid( password: string, - hash: string + hash: string | null ): Promise { + if (hash === null) { + await argon2.verify(DUMMY_HASH, password); + return false; + } return argon2.verify(hash, password); } diff --git a/src/server/utils/session.ts b/src/server/utils/session.ts index c638a7f7..8d185467 100644 --- a/src/server/utils/session.ts +++ b/src/server/utils/session.ts @@ -73,21 +73,14 @@ export async function getCurrentUser(event: H3Event) { }); } - // TODO: timing can be used to enumerate usernames - const foundUser = await Database.users.getByUsername(username); - if (!foundUser) { - throw createError({ - statusCode: 401, - statusMessage: 'Session failed', - }); - } - - const userHashPassword = foundUser.password; + // always check to avoid timing attack + const userHashPassword = foundUser?.password ?? null; const passwordValid = await isPasswordValid(password, userHashPassword); - if (!passwordValid) { + // can't login through basic auth if 2fa enabled + if (!foundUser || !passwordValid || foundUser.totpVerified) { throw createError({ statusCode: 401, statusMessage: 'Session failed',