diff --git a/backend/migrations/019_recover_ip_rate.down.sql b/backend/migrations/019_recover_ip_rate.down.sql new file mode 100644 index 0000000..d8b3139 --- /dev/null +++ b/backend/migrations/019_recover_ip_rate.down.sql @@ -0,0 +1 @@ +DELETE FROM config WHERE key = 'recover_ip_rate_per_min'; diff --git a/backend/migrations/019_recover_ip_rate.up.sql b/backend/migrations/019_recover_ip_rate.up.sql new file mode 100644 index 0000000..38841be --- /dev/null +++ b/backend/migrations/019_recover_ip_rate.up.sql @@ -0,0 +1,19 @@ +-- Per-IP flood ceiling for /recover, mirroring the one migration 017 added for /join. +-- +-- Rationale: /recover is keyed `recover:{ip}:{name}` at 5 per 15 minutes. That is the +-- right shape for its actual job — stopping someone who knows a display name (they are +-- visible on the feed) from burning the victim's 3-strike PIN counter and locking them +-- out repeatedly. But the name is attacker-chosen, so cycling names mints a fresh bucket +-- every time and the per-IP cost is unbounded. +-- +-- Behind that limiter sits a cost-12 bcrypt verify, including an UNCONDITIONAL throwaway +-- verify for names that don't exist — deliberately, to close a timing oracle. So an +-- unknown name is the cheapest possible way to make the server do ~200ms of hashing. +-- Without a ceiling, one client can saturate the box's CPU with a name generator. +-- +-- 30/min is far above any real recovery attempt (a guest tries their PIN a handful of +-- times) while capping a name-cycling flood. The per-(ip, name) bucket is unchanged and +-- remains the anti-guessing control. +INSERT INTO config (key, value) VALUES + ('recover_ip_rate_per_min', '30') +ON CONFLICT (key) DO NOTHING; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index 74f54f0..f7d495d 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -111,7 +111,7 @@ pub async fn join( // Generate a 4-digit PIN let pin: String = format!("{:04}", rand::rng().random_range(0..10000u32)); - let pin_hash = bcrypt::hash(&pin, 12).map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; + let pin_hash = hash_password(pin.clone(), 12).await?; // The pre-check above is racy: two simultaneous joins with the same name can both // pass it, and the DB's unique index then rejects the loser. Map that unique @@ -175,6 +175,28 @@ fn dummy_pin_hash() -> &'static str { }) } +/// Run a bcrypt verify on the blocking pool. +/// +/// bcrypt at cost 12 is ~200ms of deliberate CPU. Called inline on an async task it pins a +/// tokio WORKER thread for that whole time, and the runtime only has one per core — so a +/// flood of `/recover` or `/admin/login` attempts stalls every other request on the box, +/// including the feed. Offloading moves that cost to the blocking pool, which is sized for +/// exactly this and whose saturation degrades logins rather than the whole app. +async fn verify_password(candidate: String, hash: String) -> bool { + tokio::task::spawn_blocking(move || bcrypt::verify(&candidate, &hash).unwrap_or(false)) + .await + .unwrap_or(false) +} + +/// Hash a secret on the blocking pool. Same reasoning as [`verify_password`] — and this one +/// runs on the busiest auth path there is, since every guest who joins gets a PIN hashed. +pub async fn hash_password(secret: String, cost: u32) -> Result { + tokio::task::spawn_blocking(move || bcrypt::hash(&secret, cost)) + .await + .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))? + .map_err(|e| AppError::Internal(anyhow::anyhow!(e))) +} + pub async fn recover( State(state): State, ConnectInfo(peer): ConnectInfo, @@ -192,6 +214,25 @@ pub async fn recover( let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let recover_rate_on = config::get_bool(&state.config_cache, "recover_rate_enabled", true).await; if rate_limits_on && recover_rate_on { + // Coarse per-IP ceiling FIRST. The per-(ip, name) bucket below is the anti-guessing + // control, but the name is attacker-chosen, so cycling names mints a fresh bucket + // every time and leaves the per-IP cost unbounded. That matters more here than + // anywhere else: every call runs a cost-12 bcrypt verify, including an + // unconditional throwaway one for names that don't exist (see below), so an unknown + // name is the CHEAPEST way to make the server do ~200ms of hashing. Checked before + // the per-name bucket so a name generator can't walk past it. + let ip_ceiling = config::get_usize(&state.config_cache, "recover_ip_rate_per_min", 30).await; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("recover_ip:{ip}"), + ip_ceiling, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Versuche. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } + let name_key = display_name.to_lowercase(); if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), @@ -217,7 +258,7 @@ pub async fn recover( // PIN — so "no such name" and "wrong PIN" are indistinguishable by response or // timing. Display names are already public on the feed, but this still closes // the /recover enumeration + timing oracle. - let _ = bcrypt::verify(&body.pin, dummy_pin_hash()); + let _ = verify_password(body.pin.clone(), dummy_pin_hash().to_string()).await; return Err(AppError::Unauthorized("PIN ist falsch.".into())); } @@ -241,7 +282,7 @@ pub async fn recover( User::reset_pin_attempts(&state.pool, user.id).await?; } - let pin_matches = bcrypt::verify(&body.pin, &user.recovery_pin_hash).unwrap_or(false); + let pin_matches = verify_password(body.pin.clone(), user.recovery_pin_hash.clone()).await; if pin_matches { // Reset failed attempts on success @@ -340,7 +381,11 @@ pub async fn admin_login( )); } - let valid = bcrypt::verify(&body.password, &state.config.admin_password_hash).unwrap_or(false); + let valid = verify_password( + body.password.clone(), + state.config.admin_password_hash.clone(), + ) + .await; if !valid { tracing::warn!(ip = %ip, "admin_login: wrong password"); @@ -367,7 +412,7 @@ pub async fn admin_login( .map(|_| rand::rng().random_range(b'a'..=b'z') as char) .collect(); let dummy_hash = - bcrypt::hash(&dummy_pin, 4).map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; + hash_password(dummy_pin.clone(), 4).await?; let user = User::create(&state.pool, event.id, admin_name, &dummy_hash).await?; sqlx::query("UPDATE \"user\" SET role = 'admin' WHERE id = $1") .bind(user.id) diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index f29fe36..cb8fb96 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -124,6 +124,9 @@ pub async fn patch_config( // only bounds raw volume from one source, so it must stay well above the size of a // party arriving at once (see migration 017). ("join_ip_rate_per_min", true, 1.0, 100_000.0), + // Same shape for /recover: the per-(ip, name) bucket is the anti-guessing control, + // this only bounds a name-cycling flood in front of a cost-12 bcrypt (migration 019). + ("recover_ip_rate_per_min", true, 1.0, 100_000.0), ("quota_tolerance", false, 0.0, 1.0), ("estimated_guest_count", true, 1.0, 1_000_000.0), ]; diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index f47f474..a4a73c5 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -433,7 +433,7 @@ pub async fn reset_user_pin( } let pin: String = format!("{:04}", rand::rng().random_range(0..10000u32)); - let pin_hash = bcrypt::hash(&pin, 12).map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; + let pin_hash = crate::auth::handlers::hash_password(pin.clone(), 12).await?; sqlx::query( "UPDATE \"user\" diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index c6f4202..362c61a 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -41,7 +41,7 @@ pub async fn truncate_all( .execute(&state.pool) .await?; - // Reseed config. The NUMERIC values mirror migrations 005/015/016; the BOOLEAN + // Reseed config. The NUMERIC values mirror migrations 005/015/016/017/019; the BOOLEAN // toggles deliberately do NOT — migration 009 seeds every one of them `true` // (production), and this forces them `false` so the suite isn't fighting rate limits // and quotas it isn't testing. @@ -62,6 +62,7 @@ pub async fn truncate_all( ('feed_rate_per_min', '60'), ('export_rate_per_day', '3'), ('join_ip_rate_per_min', '60'), + ('recover_ip_rate_per_min', '30'), ('quota_tolerance', '0.75'), ('estimated_guest_count', '100'), ('compression_concurrency', '2'), diff --git a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts index f025b30..744188b 100644 --- a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts +++ b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts @@ -142,3 +142,65 @@ test.describe('Rate limits — guests behind a shared NAT', () => { expect((await mintAndFetch(host.jwt)).status).not.toBe(429); }); }); + +test.describe('Rate limits — /recover name cycling', () => { + test('cycling names from one IP hits the ceiling, while one name is still throttled', async ({ + api, + adminToken, + }) => { + // /recover is keyed `recover:{ip}:{name}` — right for its job (stopping someone who + // knows a display name from burning the victim's 3-strike PIN counter), but the name is + // ATTACKER-CHOSEN, so cycling names minted a fresh bucket every time. Behind it sits a + // cost-12 bcrypt verify, including an unconditional throwaway one for unknown names, so + // a name generator was the cheapest way to make the server hash forever. + // + // Squeeze the ceiling so the flood is reproducible without firing 30+ requests. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + recover_rate_enabled: 'true', + recover_ip_rate_per_min: '5', + }); + + const attempt = (name: string) => + fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: name, pin: '0000' }), + }); + + // Every name is distinct, so the per-name bucket can never fire — only the ceiling can. + const codes: number[] = []; + for (let i = 0; i < 12; i++) codes.push((await attempt(`Unbekannt${i}_${Date.now()}`)).status); + + expect( + codes.filter((c) => c === 429).length, + 'name cycling must be capped by the per-IP ceiling' + ).toBeGreaterThan(0); + + const throttled = await attempt(`Unbekannt99_${Date.now()}`); + expect(throttled.status).toBe(429); + expect(Number(throttled.headers.get('retry-after'))).toBeGreaterThan(0); + }); + + test('the per-name bucket still protects a real account', async ({ api, adminToken, guest }) => { + // The ceiling must not have REPLACED the anti-guessing control. With a generous ceiling, + // repeated wrong PINs against ONE name must still be shut down by the per-name bucket. + const victim = await guest('PinVictim'); + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + recover_rate_enabled: 'true', + recover_ip_rate_per_min: '1000', + }); + + const attempt = () => + fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: victim.displayName, pin: '9999' }), + }); + + const codes: number[] = []; + for (let i = 0; i < 7; i++) codes.push((await attempt()).status); + expect(codes.at(-1), 'guessing one name must still be throttled').toBe(429); + }); +});