ccd8221adf
client_ip() read the first X-Forwarded-For hop unconditionally. The
login rate limit and the account lockout are keyed on that value and the
audit log records it, so rotating the header per request bought an
unbounded budget and an untraceable trail.
This walked around TRUSTED_PROXY_COUNT, which exists to decide exactly
this. The app leaves ProxyFix off by default and says why, ten lines of
comment away: ProxyFix trusts X-Forwarded-*, a client can send those
itself, and the login limiter is keyed on the address. Then client_ip
read the header anyway, so the stated protection never applied.
Resolution is now from remote_addr, which ProxyFix has already filled
from the header wherever a proxy is genuinely configured. Both cases end
at the same trustworthy source, and the port still comes off because ARR
forwards clientip:port and ProxyFix passes it through verbatim.
geenforce and tools had each hand-rolled the same mistake for their own
limiters. Rather than duplicate security-relevant logic a third time,
client_ip and strip_port join the shopdb.api surface (contract 0.28.0).
geenforce's _trusted_client_ip, which already refused the raw header for
its IP allowlist, is untouched and was the model for this.
The stability table was also missing rows for 0.26.0 and 0.27.0.
INSTALL-WINDOWS-IIS.md never mentioned TRUSTED_PROXY_COUNT, so a site
installed from it runs behind IIS with the setting at 0: every request
looks like it came from IIS, the whole site shares one rate-limit
bucket, and the audit log records the proxy for every action. It now
says so, and says not to set it when nothing is in front.
Three test modules simulated separate callers by setting
X-Forwarded-For, which is the header a real attacker rotates to escape
the limit. One of them, test_client_ip_prefers_forwarded_first_hop_
stripped, asserted the defect outright. The suite could not have caught
this. They now drive REMOTE_ADDR, with a regression guard that forging
the header buys nothing.
Scope: product
(cherry picked from commit 7260244b7e)
54 lines
2.4 KiB
Python
54 lines
2.4 KiB
Python
"""Client IP extraction behind a reverse proxy.
|
|
|
|
Prod runs behind IIS ARR, which sets X-Forwarded-For to "clientip:port" (and
|
|
appends a hop per proxy). The trailing source port is ephemeral - it changes
|
|
every connection - so it must be stripped before the value is stored (audit),
|
|
matched (dashboard IP fallback), or used as a rate-limit key (login). Left in,
|
|
it makes every request look like a different client.
|
|
|
|
X-Forwarded-For is NOT read here. A client can send that header itself, so
|
|
trusting it unconditionally hands any caller a free identity: the login rate
|
|
limit and the account lockout are keyed on this value, and the audit log
|
|
records it. Reading it directly also walked around TRUSTED_PROXY_COUNT, which
|
|
exists precisely to decide whether a proxy is really in front (see the ProxyFix
|
|
block in shopdb/__init__.py).
|
|
|
|
The trusted case is already handled upstream. When TRUSTED_PROXY_COUNT is set,
|
|
ProxyFix resolves X-Forwarded-For into remote_addr before the request reaches a
|
|
view, so remote_addr is the real client. When it is unset, remote_addr is the
|
|
peer that actually connected. Either way remote_addr is the trustworthy source,
|
|
which is why this module only strips the port from it.
|
|
|
|
A site behind IIS or nginx MUST set TRUSTED_PROXY_COUNT (1 for a single proxy),
|
|
or every request will key on the proxy's own address and the per-client rate
|
|
limit becomes one shared bucket.
|
|
|
|
Port stripping is IPv6-aware: a bracketed [2001:db8::1]:443 loses its port; a
|
|
bare 2001:db8::1 (many colons, no brackets) is returned untouched.
|
|
"""
|
|
|
|
|
|
def strip_port(ipaddress):
|
|
"""Return ipaddress without a trailing :port. IPv6-safe. None/empty-safe."""
|
|
if not ipaddress:
|
|
return ipaddress
|
|
ipaddress = ipaddress.strip()
|
|
if ipaddress.startswith('['):
|
|
# [IPv6] or [IPv6]:port -> the address inside the brackets
|
|
return ipaddress[1:].split(']', 1)[0]
|
|
if ipaddress.count(':') == 1:
|
|
# exactly one colon = IPv4:port (a bare IPv6 has several colons)
|
|
return ipaddress.split(':', 1)[0]
|
|
return ipaddress
|
|
|
|
|
|
def client_ip(request_obj):
|
|
"""Client IP, port stripped, from the connecting peer.
|
|
|
|
Resolved from remote_addr, which ProxyFix has already rewritten from
|
|
X-Forwarded-For when TRUSTED_PROXY_COUNT says a proxy is really in front.
|
|
Returns None only when remote_addr is absent (callers that need a non-null
|
|
key coerce it).
|
|
"""
|
|
return strip_port(request_obj.remote_addr)
|