GHSA-29hq-23m2-2j47

Summary

validateUser() in backend/src/authentication/providers/mysql/auth-provider-mysql.service.ts returns immediately when the supplied login/email does not match any account, without ever calling comparePassword():

async validateUser(loginOrEmail: string, password: string, ip?: string, scope?: AUTH_SCOPE): Promise<UserModel> {
  let user: UserModel
  try {
    user = await this.usersManager.findUser(loginOrEmail, false)
  } catch (e) { ... }
  if (!user) {
    this.logger.warn(...)
    return null   // <-- comparePassword() is never reached here
  }
  return await this.usersManager.logUser(user, password, ip, scope)
}

comparePassword() (backend/src/common/functions.ts) already contains a dummy-hash branch that was clearly added to defend against exactly this class of attack:

export async function comparePassword(password: string, hash?: string | null): Promise<boolean> {
  if (!hash) {
    // No hash, waste time for time-based attacks
    await bcrypt.compare(password, DUMMY_PASSWORD_HASH)
    return false
  }
  return await bcrypt.compare(password, hash)
}

The problem is that this protection only runs when comparePassword() is actually invoked with a falsy hash. Because validateUser() short-circuits with return null as soon as findUser() comes back empty, the "account doesn't exist" path skips all cryptographic work entirely, while the "account exists, wrong password" path always performs a real bcrypt comparison (cost factor 10, ~100ms+). The two outcomes are trivially distinguishable by response time.

There's already a published advisory in this repo for "Username Enumeration via Timing Attack" - this looks like the same underlying issue surfacing through a different call path (the early return in validateUser()) that the existing fix (the dummy-hash branch in comparePassword()) doesn't actually reach, rather than a brand new vulnerability class.

Impact

Any unauthenticated client can determine whether a given username/email is a valid account on the instance by timing POST /api/auth/login:

• Non-existent login: near-instant rejection (no bcrypt call).
• Existing login (regardless of password correctness): consistently slower due to a real bcrypt comparison.

This enables efficient enumeration of valid accounts, which can then be used to focus credential-stuffing, password-spraying, or phishing against confirmed-valid targets.

Proof of Concept

Verified with the actual comparePassword() logic and the real DUMMY_PASSWORD_HASH constant copied verbatim from backend/src/common/functions.ts, using the project's own bcryptjs dependency (no mocking of bcrypt itself):

Avg time for "login does not exist" path (validateUser returns null, no bcrypt call): 0.00 ms
Avg time for "login exists, wrong password" path (real bcrypt.compare runs):          114.90 ms
Difference: 114.90 ms (ratio: ~58923x)

The "not found" path reproduces validateUser()'s exact early return (no call into comparePassword); the "wrong password" path reproduces logUser()'s real call into comparePassword(password, user.password). The gap is large enough to be trivially observable over a real network, even accounting for jitter.

Reachable endpoint: POST /api/auth/login, guarded only by AuthLocalGuard (Passport local strategy invoking validateUser()), no authentication required.

Suggested fix

Make validateUser() always pass through comparePassword()'s timing-equalized path, even when no user is found, e.g.:

if (!user) {
  await comparePassword(password, null) // burns the same time as a real comparison
  return null
}

so the "account not found" and "account found, wrong password" branches take statistically indistinguishable time.