fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(audit-2026-06-11/H-B1 follow-up): use last X-Forwarded-For hop for login rate-limit key

Browse Source 
extract_remote_ip took the FIRST X-Forwarded-For entry, but Caddy (the
single trusted proxy) appends the real peer as the LAST hop — earlier
entries are client-supplied. An attacker could prepend a rotating
X-Forwarded-For value to get a fresh per-IP bucket every request and
evade the per-IP login limiter (the per-username bucket + global Argon2
semaphore still bounded it, but the per-IP layer was defeated). Take the
last hop so the key reflects the real client. Added unit tests covering
the spoof, the single-hop case, and the missing-header fallback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-12 18:37:34 +02:00
parent bdcc9a606d
commit e8cc3afa07
1 changed files with 60 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
crates/manager-core/src/auth_api.rs
View File

@@ -83,8 +83,9 @@ async fn login(
) -> Response {
// Audit 2026-06-11 H-B1 — rate limit BEFORE any DB read or
// Argon2 verify so an attacker can't even queue. Keying is
// (X-Forwarded-For first hop, username) + (username), so a single
// hot user and a credential-stuffing flurry both saturate.
// (real client IP, username) + (username), so a single hot user and
// a credential-stuffing flurry both saturate. The IP is the last
// X-Forwarded-For hop (Caddy-appended); see `extract_remote_ip`.
let remote_ip = extract_remote_ip(&headers);
if let LoginRateOutcome::Denied { retry_after } = state
.login_rate_limiter
@@ -245,16 +246,27 @@ fn extract_token_for_logout(req: &Request<Body>) -> Option<String> {
Some(token.to_string())
}
/// Pull the originating client IP from the request. Caddy is the only
/// trusted proxy in front of picloud; X-Forwarded-For carries the
/// client. We take the first entry (closest to the original client).
/// Returns `"unknown"` if the header is absent/malformed so the
/// per-user bucket still gates without an IP qualifier.
/// Pull the originating client IP from the request for rate-limiting.
///
/// Audit 2026-06-11 (H-B1 follow-up) — Caddy is the single trusted
/// proxy in front of picloud and *appends* the immediate peer to
/// `X-Forwarded-For`, so the **last** entry is the address Caddy
/// actually saw. Every earlier entry is client-supplied: an attacker
/// can send `X-Forwarded-For: <random>` and Caddy forwards
/// `<random>, <real-client>`. Taking the first entry (the previous
/// behavior) therefore let an attacker rotate the per-IP bucket key on
/// every request and evade the per-IP limit. Take the last entry so the
/// qualifier reflects the real client. Returns `"unknown"` when the
/// header is absent/malformed so the per-username bucket still gates.
///
/// (This assumes exactly one trusted proxy hop, which is the documented
/// single-node topology. A multi-proxy deployment would need a
/// configured trusted-hop count; tracked for the cluster work.)
fn extract_remote_ip(headers: &HeaderMap) -> String {
if let Some(xff) = headers.get("x-forwarded-for") {
if let Ok(s) = xff.to_str() {
if let Some(first) = s.split(',').next() {
let trimmed = first.trim();
if let Some(last) = s.split(',').next_back() {
let trimmed = last.trim();
if !trimmed.is_empty() {
return trimmed.to_string();
}
@@ -298,3 +310,42 @@ fn internal_error() -> Response {
)
.into_response()
}
#[cfg(test)]
mod tests {
use super::extract_remote_ip;
use axum::http::{HeaderMap, HeaderValue};
fn xff(value: &str) -> HeaderMap {
let mut h = HeaderMap::new();
h.insert("x-forwarded-for", HeaderValue::from_str(value).unwrap());
h
}
#[test]
fn takes_last_xff_entry_caddy_appended_peer() {
// Audit 2026-06-11 (H-B1 follow-up) — the real client is the
// last hop Caddy appended; earlier entries are client-spoofable.
assert_eq!(extract_remote_ip(&xff("203.0.113.9")), "203.0.113.9");
assert_eq!(
extract_remote_ip(&xff("9.9.9.9, 203.0.113.9")),
"203.0.113.9"
);
}
#[test]
fn spoofed_leading_entry_does_not_change_the_key() {
// Attacker prepends junk; Caddy still appends the real peer last,
// so the rate-limit key is stable regardless of the spoof.
let a = extract_remote_ip(&xff("evil-1, 203.0.113.9"));
let b = extract_remote_ip(&xff("evil-2, 203.0.113.9"));
assert_eq!(a, b);
assert_eq!(a, "203.0.113.9");
}
#[test]
fn missing_or_empty_header_is_unknown() {
assert_eq!(extract_remote_ip(&HeaderMap::new()), "unknown");
assert_eq!(extract_remote_ip(&xff(" ")), "unknown");
}
}