Skip to content

fix(auth): equalise login response time across unknown-email and bad-password branches - #867

Open
rollroyces wants to merge 1 commit into
deeplethe:devfrom
rollroyces:fix/login-timing-attack
Open

rollroyces wants to merge 1 commit into
deeplethe:devfrom
rollroyces:fix/login-timing-attack

Conversation

@rollroyces

Copy link
Copy Markdown
Contributor

Fix login timing oracle that leaks which emails are registered

/auth/login had a real but easy-to-fix timing oracle:

let Some(user) = find_user_by_email(...).await? else {
    return Err(AppError::Unauthorized);   // 早返回,0ms argon2
};
if !verify_password(&req.password, &user.password_hash) {
    return Err(AppError::Unauthorized);   // 一次 argon2,~50ms
}

Two branches returning the same error, but one runs argon2 (m=19456, t=2, p=1) and the other doesn't. Network jitter smooths some of that out, but at the tails the gap is measurable, and on a quiet box it's reliably tens of ms. That's enough for a credential-stuffing pass to enumerate which addresses are registered, then target the survivors.

Fix

Always run verify_password before returning Unauthorized. When the user lookup returned None, verify against a fixed dummy argon2 hash (auth::DUMMY_PASSWORD_HASH) and discard the result:

let password_valid = match &user {
    Some(u) => auth::verify_password(&req.password, &u.password_hash),
    None => {
        let _ = auth::verify_password(&req.password, auth::DUMMY_PASSWORD_HASH);
        false
    }
};

Both branches now pay one argon2 verify. The record_login_failure reason (unknown_email vs bad_password) stays in the audit log so admins can still distinguish the two attack shapes.

Safety of the dummy hash

The dummy plaintext is 00-utopia-fixed-timing-attack-mitigation-only-x9f3k-2026-09-23 — a string that looks test-only by construction. The hash for it is hardcoded in auth.rs. Two unit-test assertions register the safety properties:

  1. verify_password against DUMMY_PASSWORD_HASH with the paired plaintext does return true — otherwise the unknown-email branch falls through PasswordHash::new(...).unwrap_or(false) and the timing gap re-appears.
  2. verify_password against DUMMY_PASSWORD_HASH with the obvious candidates (empty, "password", "hunter2", "letmein", "utopia", near-misses one day off in either direction) returns false — i.e. the dummy plaintext is guaranteed not to collide with a real user's password.

If anyone ever changes the dummy plaintext, the second assertion will fire and the change will get caught in CI.

Out of scope

  • change_password already has the user from AuthUser so no timing leak there.
  • The 4xx response body for both error reasons is intentionally identical (Unauthorized).
  • General rate-limiting per-IP/email is a separate piece of work — this PR only closes the side channel.

Closes the timing oracle on /auth/login. Filing follow-up issues for any related rate-limiting work.

Signed-off-by: rollroyces royce@rollroyces.com

…password branches

The login handler at crates/utopia-server/src/api/auth_routes.rs:121
returned early when the supplied email was not found, before any
argon2 verification ran. The bad-password branch did run verify_password
(~50ms on default params m=19456,t=2,p=1). An attacker could therefore
distinguish registered emails from unregistered ones by timing the
response, and roll a credential-stuffing pass against the survivors.

Fix: keep the user lookup, but always run verify_password before
returning Unauthorized. When the user is missing, verify against a
fixed dummy argon2 hash (DUMMY_PASSWORD_HASH in auth.rs); the result is
discarded, but the call still pays the argon2 cost.

The dummy plaintext (`00-utopia-fixed-timing-attack-mitigation-only-x9f3k-2026-09-23`)
is a clearly-test-only string and the unit test asserts that no common
password or near-miss matches the dummy hash — i.e. the dummy hash's
plaintext is guaranteed never to collide with a real user's password.
The test also asserts the dummy *does* match its own plaintext, which
is the property the timing mitigation depends on (otherwise the
unknown-email branch would fall through PasswordHash::new's
`unwrap_or(false)` and the timing gap would re-appear).

Audit log reason stays accurate (unknown_email vs bad_password) so
admins can still see the attack shape.

Signed-off-by: rollroyces <royce@rollroyces.com>

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mitigation is right and I'd like to land it: the unknown-email branch now pays
the same argon2 verify as the bad-password branch, find_user_by_email already
filters deactivated_at, so deactivated accounts fall into the same branch as
unknown ones, and every user row carries a real argon2 hash (SSO only binds to an
existing password account; admin creation hashes too), so there's no third
"password_hash is a placeholder → parse fails → 0 ms" branch hiding behind this.
Verified locally: cargo test -p utopia-server auth:: 3/3, clippy clean, and
hash_password uses Argon2::default() whose 0.5.3 defaults are exactly the
m=19456,t=2,p=1 in the dummy string.

Two things to change before merging, both small, and one of them matters because
it's a security-sensitive file where the comment is the threat model:

1. The "paired plaintext must verify" property is not what makes this work, and
the comment says the opposite of what's true.

verify_password is PasswordHash::new(hash).map(|p| Argon2::default().verify_password(pw, &p).is_ok()). The early unwrap_or(false) only fires if the
PHC string fails to parse. Once it parses, the full argon2 computation runs
regardless of whether the password matches — that's the whole point of a
constant-cost verifier. So a dummy whose plaintext nobody knows equalises timing
exactly as well as one with a published plaintext; the first assertion in
dummy_password_hash_is_a_real_argon2id_hash pins a property the mitigation
doesn't depend on.

The second assertion (common passwords don't verify against the dummy) is
vacuously true for any hash and doesn't establish what its message claims. And the
doc comment's warning — 「不要让任何真用户用这条当密码,否则 argon2 真的算到相同常
数,那条分支和正常分支就过的是同一个 hash」 — is a misunderstanding: a real user
who picked that password would have their own salt and their own hash;
DUMMY_PASSWORD_HASH is never consulted on the Some branch. There's no
collision to avoid. It's harmless at runtime because the None branch discards
the result, but the next person to read this will carry a wrong model of why the
branch is safe.

2. Parameter parity is a coincidence, not a guarantee. If hash_password ever
moves off Argon2::default() — a bump in the crate's defaults, or someone tuning
Params::new(...) — the dummy silently keeps the old cost and the gap comes back,
and nothing in the test suite would notice.

Both are fixed by the same change: don't hardcode a hash at all. Derive it once
from the real hash_password so it can't drift, and drop the plaintext story:

/// 「邮箱不存在」分支用的常量时间伙伴:一个用真实参数算出来的 argon2 哈希,
/// 明文是随机字节、算完即弃。要点只有一个——它必须能被 `PasswordHash::new`
/// 解析并带着与 `hash_password` 相同的参数,这样那条分支和「邮箱存在、密码错」
/// 走的是同一段计算。它不匹配任何口令;结果本来就被丢弃。
pub fn dummy_password_hash() -> &'static str {
    static HASH: OnceLock<String> = OnceLock::new();
    HASH.get_or_init(|| {
        let mut bytes = [0u8; 32];
        OsRng.fill_bytes(&mut bytes);
        hash_password(&hex::encode(bytes)).expect("hashing random bytes cannot fail")
    })
}

and replace the test with the one property that actually matters:

#[test]
fn dummy_hash_costs_the_same_as_a_real_one() {
    let dummy = PasswordHash::new(dummy_password_hash()).unwrap();
    let real = PasswordHash::new(&hash_password("anything").unwrap()).unwrap();
    assert_eq!(dummy.algorithm, real.algorithm);
    assert_eq!(dummy.params, real.params);
    assert_eq!(dummy.salt.map(|s| s.len()), real.salt.map(|s| s.len()));
}

(If you'd rather keep a constant, that's fine too — keep the parity test, delete
the plaintext assertions, and rewrite the doc comment to say the string only
needs to parse with the same params.)

One tiny thing: let user = user.expect("user is Some when password verified");
is sound, but let Some(user) = user else { unreachable!(...) } reads the
invariant rather than asserting it; take it or leave it.

Not for this PR, just noting since you're in the area: register answers
"email already registered" explicitly, which is an enumeration oracle by design
under open registration. If enumeration is the concern being closed here, that's
the other half.

The branch is 4 behind dev; happy to update it and merge as soon as the above is
in.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants