Skip to content

fix(api): Login timing still reveals whether a username exists #199

Description

@bitbiter-dev

Found while reviewing the #173 fix. #173 removed the hashing asymmetry on login; this is the residual leak one layer up, in AuthService.LoginAsync.

The leak

After #173, verifying a non-existent account costs the same Argon2 pass as verifying a real one, so the hasher itself reveals nothing. But the code path afterwards is not symmetric:

var passwordValid = passwordHasher.Verify(password, user?.PasswordHash);   // now equal cost

var now = clock.GetCurrentInstant();

if (user is not null && !passwordValid && !lockout.IsLockedOut(user, now))
    await lockout.RecordFailedAttemptAsync(user, now, ct);                 // KNOWN user only

A known username with a wrong password performs a database write. An unknown username skips it entirely.

Why it matters more than it looks

The delta is a Postgres round trip — milliseconds. That is orders of magnitude larger than the asymmetries considered negligible inside the hasher (a Base64 parse, a fixed-time compare: microseconds). So this is now the dominant timing signal on the login path, not a rounding error.

Scenario. An attacker posts /api/v1/auth/login with a deliberately wrong password against a list of candidate usernames and times the responses. Existing accounts come back consistently slower by one write. Username enumeration works, which is precisely what the dummy-hash defence was introduced to prevent.

⚠️ Pre-existing, not introduced by #173 — the old precomputed-dummy-hash version had the identical asymmetry. What changed is that it is now the only remaining one, so it is worth fixing rather than lost in noise.

Options, none free

  1. Write unconditionally. Record a failed attempt for a sentinel/non-existent user too, so both branches pay a write. Symmetric, but it means writing rows for usernames that do not exist — a storage and lock-contention channel an attacker controls.
  2. Move the write off the response path. Queue the failed-attempt record and respond without awaiting it. Removes the delta, but loses the ordering guarantee lockout currently relies on and introduces a durability gap.
  3. Pad the response to a fixed floor. Delay every login response to a constant target. Hides the write and any future asymmetry, at the cost of making every login as slow as the slowest branch.
  4. Accept it, and say so. Rate limiting is already configured; whether that reduces enumeration to an acceptable risk is a judgement call rather than something to discover later. If this is the answer, it belongs in an ADR, not an unstated assumption.

📌 Option 4 is a legitimate outcome. The point of this issue is that the choice gets made explicitly — the code currently reads as though the defence is complete.

Done when

  • A decision is recorded (ADR if it is "accept")
  • If fixed: a test that pins the chosen property, not a wall-clock ratio between two paths — that shape goes flaky on loaded CI
  • The comment at AuthService.LoginAsync updated; it currently names this leak and points here

Context: #173, and the comments added alongside it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingneeds-triageMaintainer needs to evaluate this issue

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions