Fix 3 pentest findings: lockout status disclosure, timing side-channel, XFF trust scope
F-01 (passkeys.py): Add constant-time DB no-op on login/begin when username not found. Without it the absent credential-fetch query makes the "no user" path measurably faster, leaking username existence via timing. F-02 (session.py, auth.py, passkeys.py, totp.py): Change check_account_lockout from HTTP 423 to 401 — status-code analysis can no longer distinguish a locked account from an invalid credential. record_failed_login now returns remaining attempt count; callers use it for progressive UX warnings (<=3 attempts left, and on the locking attempt) without changing the 401 status code visible to attackers. Session-lock 423 path in get_current_user is unaffected. F-03 (nginx.conf): Replace set_real_ip_from 0.0.0.0/0 with RFC 1918 ranges (172.16.0.0/12, 10.0.0.0/8) to prevent external clients from spoofing X-Forwarded-For to bypass rate limiting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
863e9e2c45
commit
44e6c8e3e5
@ -285,13 +285,19 @@ async def login(
|
|||||||
await check_account_lockout(user)
|
await check_account_lockout(user)
|
||||||
|
|
||||||
if not valid:
|
if not valid:
|
||||||
await record_failed_login(db, user)
|
remaining = await record_failed_login(db, user)
|
||||||
await log_audit_event(
|
await log_audit_event(
|
||||||
db, action="auth.login_failed", actor_id=user.id,
|
db, action="auth.login_failed", actor_id=user.id,
|
||||||
detail={"reason": "invalid_password"}, ip=client_ip,
|
detail={"reason": "invalid_password", "attempts_remaining": remaining}, ip=client_ip,
|
||||||
)
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
raise HTTPException(status_code=401, detail="Invalid username or password")
|
if remaining == 0:
|
||||||
|
detail = "Account temporarily locked. Try again in 30 minutes."
|
||||||
|
elif remaining <= 3:
|
||||||
|
detail = f"Invalid username or password. {remaining} attempt(s) remaining before account locks."
|
||||||
|
else:
|
||||||
|
detail = "Invalid username or password"
|
||||||
|
raise HTTPException(status_code=401, detail=detail)
|
||||||
|
|
||||||
# Block passwordless-only accounts from using the password login path.
|
# Block passwordless-only accounts from using the password login path.
|
||||||
# Checked after password verification to avoid leaking account existence via timing.
|
# Checked after password verification to avoid leaking account existence via timing.
|
||||||
|
|||||||
@ -267,6 +267,16 @@ async def passkey_login_begin(
|
|||||||
cid_bytes = base64url_to_bytes(row[0])
|
cid_bytes = base64url_to_bytes(row[0])
|
||||||
transports = json.loads(row[1]) if row[1] else None
|
transports = json.loads(row[1]) if row[1] else None
|
||||||
credential_data.append((cid_bytes, transports))
|
credential_data.append((cid_bytes, transports))
|
||||||
|
else:
|
||||||
|
# F-01: User not found — run a no-op DB query to equalize timing with
|
||||||
|
# the credential fetch that executes for existing users. Without this,
|
||||||
|
# the absence of the second query makes the "no user" path measurably
|
||||||
|
# faster, leaking whether the username exists.
|
||||||
|
await db.execute(
|
||||||
|
select(PasskeyCredential.credential_id).where(
|
||||||
|
PasskeyCredential.user_id == 0
|
||||||
|
).limit(1)
|
||||||
|
)
|
||||||
|
|
||||||
# V-03: Generate options regardless of whether user exists or has passkeys.
|
# V-03: Generate options regardless of whether user exists or has passkeys.
|
||||||
# Identical response shape prevents timing enumeration.
|
# Identical response shape prevents timing enumeration.
|
||||||
@ -330,13 +340,15 @@ async def passkey_login_complete(
|
|||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.warning("Passkey authentication verification failed for user %s: %s", user.id, e)
|
logger.warning("Passkey authentication verification failed for user %s: %s", user.id, e)
|
||||||
# Increment failed login counter (shared with password auth)
|
# Increment failed login counter (shared with password auth)
|
||||||
await record_failed_login(db, user)
|
remaining = await record_failed_login(db, user)
|
||||||
await log_audit_event(
|
await log_audit_event(
|
||||||
db, action="passkey.login_failed", actor_id=user.id,
|
db, action="passkey.login_failed", actor_id=user.id,
|
||||||
detail={"reason": "verification_failed"},
|
detail={"reason": "verification_failed", "attempts_remaining": remaining},
|
||||||
ip=get_client_ip(request),
|
ip=get_client_ip(request),
|
||||||
)
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
if remaining == 0:
|
||||||
|
raise HTTPException(status_code=401, detail="Account temporarily locked. Try again in 30 minutes.")
|
||||||
raise HTTPException(status_code=401, detail="Authentication failed")
|
raise HTTPException(status_code=401, detail="Authentication failed")
|
||||||
|
|
||||||
# Update sign count (log anomaly but don't fail — S-05)
|
# Update sign count (log anomaly but don't fail — S-05)
|
||||||
|
|||||||
@ -276,8 +276,10 @@ async def totp_verify(
|
|||||||
normalized = data.backup_code.strip().upper()
|
normalized = data.backup_code.strip().upper()
|
||||||
valid = await _verify_backup_code(db, user.id, normalized)
|
valid = await _verify_backup_code(db, user.id, normalized)
|
||||||
if not valid:
|
if not valid:
|
||||||
await record_failed_login(db, user)
|
remaining = await record_failed_login(db, user)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
if remaining == 0:
|
||||||
|
raise HTTPException(status_code=401, detail="Account temporarily locked. Try again in 30 minutes.")
|
||||||
raise HTTPException(status_code=401, detail="Invalid backup code")
|
raise HTTPException(status_code=401, detail="Invalid backup code")
|
||||||
|
|
||||||
# Backup code accepted — reset lockout counter and issue session
|
# Backup code accepted — reset lockout counter and issue session
|
||||||
@ -293,8 +295,10 @@ async def totp_verify(
|
|||||||
# --- TOTP code path ---
|
# --- TOTP code path ---
|
||||||
matched_window = verify_totp_code(user.totp_secret, data.code)
|
matched_window = verify_totp_code(user.totp_secret, data.code)
|
||||||
if matched_window is None:
|
if matched_window is None:
|
||||||
await record_failed_login(db, user)
|
remaining = await record_failed_login(db, user)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
if remaining == 0:
|
||||||
|
raise HTTPException(status_code=401, detail="Account temporarily locked. Try again in 30 minutes.")
|
||||||
raise HTTPException(status_code=401, detail="Invalid code")
|
raise HTTPException(status_code=401, detail="Invalid code")
|
||||||
|
|
||||||
# Replay prevention — record (user_id, code, actual_matching_window)
|
# Replay prevention — record (user_id, code, actual_matching_window)
|
||||||
|
|||||||
@ -33,24 +33,31 @@ def set_session_cookie(response: Response, token: str) -> None:
|
|||||||
|
|
||||||
|
|
||||||
async def check_account_lockout(user: User) -> None:
|
async def check_account_lockout(user: User) -> None:
|
||||||
"""Raise HTTP 423 if the account is currently locked."""
|
"""Raise HTTP 401 if the account is currently locked.
|
||||||
|
|
||||||
|
Uses 401 (same status as wrong-password) so that status-code analysis
|
||||||
|
cannot distinguish a locked account from an invalid credential (F-02).
|
||||||
|
"""
|
||||||
if user.locked_until and datetime.now() < user.locked_until:
|
if user.locked_until and datetime.now() < user.locked_until:
|
||||||
remaining = int((user.locked_until - datetime.now()).total_seconds() / 60) + 1
|
remaining = int((user.locked_until - datetime.now()).total_seconds() / 60) + 1
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=423,
|
status_code=401,
|
||||||
detail=f"Account locked. Try again in {remaining} minutes.",
|
detail=f"Account temporarily locked. Try again in {remaining} minutes.",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
async def record_failed_login(db: AsyncSession, user: User) -> None:
|
async def record_failed_login(db: AsyncSession, user: User) -> int:
|
||||||
"""Increment failure counter; lock account after 10 failures.
|
"""Increment failure counter; lock account after 10 failures.
|
||||||
|
|
||||||
|
Returns the number of attempts remaining before lockout (0 = just locked).
|
||||||
Does NOT commit — caller owns the transaction boundary.
|
Does NOT commit — caller owns the transaction boundary.
|
||||||
"""
|
"""
|
||||||
user.failed_login_count += 1
|
user.failed_login_count += 1
|
||||||
|
remaining = max(0, 10 - user.failed_login_count)
|
||||||
if user.failed_login_count >= 10:
|
if user.failed_login_count >= 10:
|
||||||
user.locked_until = datetime.now() + timedelta(minutes=30)
|
user.locked_until = datetime.now() + timedelta(minutes=30)
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
return remaining
|
||||||
|
|
||||||
|
|
||||||
async def record_successful_login(db: AsyncSession, user: User) -> None:
|
async def record_successful_login(db: AsyncSession, user: User) -> None:
|
||||||
|
|||||||
@ -29,13 +29,14 @@ server {
|
|||||||
# Suppress nginx version in Server header
|
# Suppress nginx version in Server header
|
||||||
server_tokens off;
|
server_tokens off;
|
||||||
|
|
||||||
# ── Real client IP restoration (PT-01) ────────────────────────────
|
# ── Real client IP restoration (PT-01 / F-03) ─────────────────────
|
||||||
# Pangolin (TLS-terminating reverse proxy) connects via Docker bridge.
|
# Pangolin (TLS-terminating reverse proxy) connects via Docker bridge.
|
||||||
# Restore the real client IP from X-Forwarded-For so that limit_req_zone
|
# Restore the real client IP from X-Forwarded-For so that limit_req_zone
|
||||||
# (which keys on $binary_remote_addr) throttles per-client, not per-proxy.
|
# (which keys on $binary_remote_addr) throttles per-client, not per-proxy.
|
||||||
# Safe to trust all sources: nginx is only reachable via Docker networking,
|
# Restricted to RFC 1918 ranges only — trusting 0.0.0.0/0 would allow an
|
||||||
# never directly internet-facing. Tighten if deployment model changes.
|
# external client to spoof X-Forwarded-For and bypass rate limiting (F-03).
|
||||||
set_real_ip_from 0.0.0.0/0;
|
set_real_ip_from 172.16.0.0/12;
|
||||||
|
set_real_ip_from 10.0.0.0/8;
|
||||||
real_ip_header X-Forwarded-For;
|
real_ip_header X-Forwarded-For;
|
||||||
real_ip_recursive on;
|
real_ip_recursive on;
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user