From a2189992437095fbb1606fc2a7d736edf353842e Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 6 Jul 2026 20:15:01 -0700 Subject: [PATCH] fix(server): login timing pad, 409 on register race, avatar magic-byte check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the three actionable server Low findings from the 2026-07-06 review: - #139: unknown-username logins now verify against a static bcrypt dummy hash so both failure paths pay the same cost — response timing no longer reveals which usernames exist. - #140: register maps a unique-constraint violation to UsernameTaken (409) — the SELECT pre-check stays as the friendly fast path, the constraint is the arbiter for the concurrent case. - #141: avatar uploads must start with the magic bytes of the declared image type; the stored extension (which decides how the file is re-served) is now backed by content, not the Content-Type header. Unit tests: real/spoofed/truncated signatures for all four formats, and the dummy hash's validity at BCRYPT_COST. Closes #139 Closes #140 Closes #141 Co-Authored-By: Claude Fable 5 --- solitaire_server/src/auth.rs | 94 +++++++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 2 deletions(-) diff --git a/solitaire_server/src/auth.rs b/solitaire_server/src/auth.rs index 5704c16..8b9d4f2 100644 --- a/solitaire_server/src/auth.rs +++ b/solitaire_server/src/auth.rs @@ -74,6 +74,15 @@ struct UserRow { /// bcrypt work factor. Cost 12 ≈ 300 ms on modern hardware — balances security against registration latency. pub const BCRYPT_COST: u32 = 12; +/// Static bcrypt hash used to equalise login timing when the username does +/// not exist (issue #139: user-enumeration timing oracle). Both login paths +/// must pay the same bcrypt cost; this hash is verified against when there +/// is no real one. Computed once at first use with the same [`BCRYPT_COST`] +/// as real hashes. `None` only if bcrypt itself fails on a constant input — +/// in that case the dummy verify is skipped rather than panicking. +static DUMMY_PASSWORD_HASH: std::sync::LazyLock> = + std::sync::LazyLock::new(|| hash("ferrous-dummy-timing-pad", BCRYPT_COST).ok()); + async fn hash_password(password: String) -> Result { tokio::task::spawn_blocking(move || hash(password, BCRYPT_COST)) .await @@ -208,6 +217,10 @@ pub async fn register( let password_hash = hash_password(body.password).await?; let now = Utc::now().to_rfc3339(); + // The SELECT above is a friendly fast path; the UNIQUE constraint is the + // real arbiter. A concurrent registration that slips between the two + // surfaces as a unique violation here — map it to the same 409 the + // fast path produces instead of a raw 500 (issue #140). sqlx::query!( "INSERT INTO users (id, username, password_hash, created_at) VALUES (?, ?, ?, ?)", user_id, @@ -216,7 +229,11 @@ pub async fn register( now ) .execute(&state.pool) - .await?; + .await + .map_err(|e| match &e { + sqlx::Error::Database(db) if db.is_unique_violation() => AppError::UsernameTaken, + _ => AppError::from(e), + })?; let access_token = make_access_token(&user_id, &state.jwt_secret)?; let (refresh_token, refresh_jti) = make_refresh_token(&user_id, &state.jwt_secret)?; @@ -242,7 +259,16 @@ pub async fn login( .fetch_optional(&state.pool) .await?; - let row = row.ok_or(AppError::InvalidCredentials)?; + let Some(row) = row else { + // Unknown username: burn a bcrypt verify against a static dummy hash + // so this path costs the same as a wrong-password attempt. Returning + // immediately here would let response timing reveal which usernames + // exist (issue #139). + if let Some(dummy) = DUMMY_PASSWORD_HASH.as_ref() { + let _ = verify_password(body.password, dummy.clone()).await; + } + return Err(AppError::InvalidCredentials); + }; let row_id = row .id .ok_or_else(|| AppError::Internal("user id missing".into()))?; @@ -362,6 +388,22 @@ const AVATAR_MAX_BYTES: usize = 1024 * 1024; /// /// The `Content-Type` header must be one of `image/jpeg`, `image/png`, /// `image/webp`, or `image/gif`. The previous avatar file is replaced in-place. +/// Returns `true` when `body` begins with the magic bytes of the image type +/// implied by `ext` (the extension derived from the declared Content-Type). +/// +/// Avatars are re-served under the stored extension, so the extension must be +/// derived from the content itself — a client header is not evidence of what +/// the bytes are (issue #141). +fn magic_bytes_match(ext: &str, body: &[u8]) -> bool { + match ext { + "png" => body.starts_with(&[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A]), + "jpg" => body.starts_with(&[0xFF, 0xD8, 0xFF]), + "gif" => body.starts_with(b"GIF87a") || body.starts_with(b"GIF89a"), + "webp" => body.len() >= 12 && body.starts_with(b"RIFF") && &body[8..12] == b"WEBP", + _ => false, + } +} + pub async fn upload_avatar( State(state): State, user: AuthenticatedUser, @@ -387,6 +429,14 @@ pub async fn upload_avatar( if body.len() > AVATAR_MAX_BYTES { return Err(AppError::BadRequest("avatar must be ≤ 1 MB".into())); } + // The stored extension decides how the file is re-served, so it must be + // backed by the bytes, not just the client's Content-Type header + // (issue #141). + if !magic_bytes_match(ext, &body) { + return Err(AppError::BadRequest( + "avatar bytes do not match the declared image type".into(), + )); + } // Write to avatars/ directory, replacing any previous file for this user. tokio::fs::create_dir_all("avatars") @@ -587,6 +637,46 @@ mod tests { assert!(username_chars_ok("")); } + #[test] + fn magic_bytes_match_accepts_real_signatures() { + assert!(magic_bytes_match( + "png", + &[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A, 0x00] + )); + assert!(magic_bytes_match("jpg", &[0xFF, 0xD8, 0xFF, 0xE0, 0x00])); + assert!(magic_bytes_match("gif", b"GIF89a\x00\x00")); + assert!(magic_bytes_match("gif", b"GIF87a\x00\x00")); + assert!(magic_bytes_match("webp", b"RIFF\x00\x00\x00\x00WEBPVP8 ")); + } + + #[test] + fn magic_bytes_match_rejects_mismatched_or_bogus_content() { + // HTML declared as PNG — the exact spoof the check exists to stop. + assert!(!magic_bytes_match("png", b"