From 42093969b5436ec881e3d309210c2e0eca024ecd Mon Sep 17 00:00:00 2001 From: Johann150 Date: Sun, 23 Oct 2022 23:03:36 +0200 Subject: [PATCH] server: improve security of password reset flow - make the rate limiting for requesting password resets much more sensitive NB: This rate limiting is applied per IP address. - only allow 1 reset request at a time NB: This is effectively a rate limit that is applied per user. - try to mitigate timing attacks that may reveal a user's email address. - Rewrite email body for password reset request to contain the username. The username will be known to the person that requested the reset. This should serve to reassure users that this is not a phishing mail. - refactor to use time constants and proper errors Changelog: Security --- .../api/endpoints/request-reset-password.ts | 49 +++++++++---------- .../server/api/endpoints/reset-password.ts | 24 ++++++--- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/packages/backend/src/server/api/endpoints/request-reset-password.ts b/packages/backend/src/server/api/endpoints/request-reset-password.ts index 3aae59e33..402df9ffa 100644 --- a/packages/backend/src/server/api/endpoints/request-reset-password.ts +++ b/packages/backend/src/server/api/endpoints/request-reset-password.ts @@ -4,7 +4,7 @@ import config from '@/config/index.js'; import { Users, UserProfiles, PasswordResetRequests } from '@/models/index.js'; import { sendEmail } from '@/services/send-email.js'; import { genId } from '@/misc/gen-id.js'; -import { HOUR } from '@/const.js'; +import { DAY } from '@/const.js'; import define from '../define.js'; export const meta = { @@ -15,12 +15,8 @@ export const meta = { description: 'Request a users password to be reset.', limit: { - duration: HOUR, - max: 3, - }, - - errors: { - + duration: DAY, + max: 1, }, } as const; @@ -35,25 +31,26 @@ export const paramDef = { // eslint-disable-next-line import/no-default-export export default define(meta, paramDef, async (ps) => { - const user = await Users.findOneBy({ - usernameLower: ps.username.toLowerCase(), - host: IsNull(), + const [user, profile] = await Promise.all([ + Users.findOneBy({ + usernameLower: ps.username.toLowerCase(), + host: IsNull(), + }), + UserProfiles.findOneByOrFail({ userId: user.id }), + ]); + + const resetRequestExists = await PasswordResetRequests.countBy({ + userId: user.id, }); - // 合致するユーザーが登録されていなかったら無視 - if (user == null) { - return; - } - - const profile = await UserProfiles.findOneByOrFail({ userId: user.id }); - - // 合致するメアドが登録されていなかったら無視 - if (profile.email !== ps.email) { - return; - } - - // メアドが認証されていなかったら無視 - if (!profile.emailVerified) { + // This is an "arithmetical or" to try to prevent timing attacks which might reveal + // a users email address. + if ( + user == null + | profile.email !== ps.email + | !profile.emailVerified + | resetRequestExists > 0 + ) { return; } @@ -69,6 +66,6 @@ export default define(meta, paramDef, async (ps) => { const link = `${config.url}/reset-password/${token}`; sendEmail(ps.email, 'Password reset requested', - `To reset password, please click this link:
${link}`, - `To reset password, please click this link: ${link}`); + `Hello ${user.username}!

You requested to reset your password. To do so, please follow this link:

${link}

The link is valid for 30 minutes.

`, + `Hello ${user.username}!\nYou requested to reset your password. To do so, please follow this link: ${link}\nThe link is valid for 30 minutes.`); }); diff --git a/packages/backend/src/server/api/endpoints/reset-password.ts b/packages/backend/src/server/api/endpoints/reset-password.ts index ea7df52c0..ba32f475c 100644 --- a/packages/backend/src/server/api/endpoints/reset-password.ts +++ b/packages/backend/src/server/api/endpoints/reset-password.ts @@ -1,5 +1,6 @@ import bcrypt from 'bcryptjs'; import { UserProfiles, PasswordResetRequests } from '@/models/index.js'; +import { MINUTE } from '@/const.js'; import define from '../define.js'; export const meta = { @@ -10,7 +11,11 @@ export const meta = { description: 'Complete the password reset that was previously requested.', errors: { - + noSuchResetRequest: { + message: 'No such password reset request.', + code: 'NO_SUCH_RESET_REQUEST', + id: '2c455231-675b-41be-b313-1181646a3ad3', + }, }, } as const; @@ -25,13 +30,20 @@ export const paramDef = { // eslint-disable-next-line import/no-default-export export default define(meta, paramDef, async (ps, user) => { - const req = await PasswordResetRequests.findOneByOrFail({ + const req = await PasswordResetRequests.findOneBy({ token: ps.token, }); - // 発行してから30分以上経過していたら無効 - if (Date.now() - req.createdAt.getTime() > 1000 * 60 * 30) { - throw new Error(); // TODO + if (req == null) throw new ApiError(meta.errors.noSuchResetRequest); + + // Check that the has not expired yet after 30 minutes. + // + // This is a secondary check just in case the expiry task is broken, + // the expiry task schedule is not aligned to the timing, or something + // else strange is going on. After all this is security critical. + if (Date.now() - req.createdAt.getTime() > 30 * MINUTE) { + await PasswordResetRequests.delete(req.id); + throw new ApiError(meta.errors.noSuchResetRequest); } // Generate hash of password @@ -42,5 +54,5 @@ export default define(meta, paramDef, async (ps, user) => { password: hash, }); - PasswordResetRequests.delete(req.id); + await PasswordResetRequests.delete(req.id); }); -- 2.43.0