mirror of https://github.com/openwrt/luci.git
bddda7b4f6
get_client_ip() fell back to the leftmost X-Forwarded-For element
whenever REMOTE_ADDR was loopback, and the value it returned decides two
security outcomes in check(): whether the IP is whitelisted, which skips
2FA entirely, and which bucket the login rate limiter charges.
The fallback only fires behind a reverse proxy, which is what it was
written for - but that is exactly where the leftmost element is the one
the client sent, since a proxy appends rather than prepends. A request
carrying `X-Forwarded-For: <whitelisted address>` therefore returns
`{ required: false, whitelisted: true }` and never asks for an OTP, and
a fresh header value per request gives every attempt its own rate-limit
bucket, so the max_attempts lockout never fires and the six digits can
be brute forced. The same header pointed at someone else's address locks
that address out instead.
Nothing else in LuCI reads X-Forwarded-For, and the plugin has no
trusted-proxy configuration that could tell the header apart from a
forged one, so drop the fallback and use REMOTE_ADDR alone.
Its loopback gate means only an on-box proxy is affected by the removal,
and there every client collapses onto 127.0.0.1. The whitelist then
simply stops matching, and strict_mode stops taking the
local_subnet_bypass on an uncalibrated clock - both fail closed, which
is the point. The rate limiter does not: its gate runs ahead of
is_2fa_enabled() and returns blocked rather than an OTP prompt, so five
bad codes from any one client behind the proxy would lock every account
on the box out of LuCI for the lockout period, including accounts with
no OTP configured.
So key the bucket on user and address together, the way luci-base
already keys its own verify limiter. Brute forcing one account still
trips the lockout after max_attempts, which is what the limiter is for,
while a shared address no longer makes one client's failures anyone
else's. The address-only key was also the reason the three call sites
skipped the limiter entirely when REMOTE_ADDR was absent; the key is
well formed either way now, so they no longer need to.
A proxy on a separate host is a different deployment and this commit
does not change it: REMOTE_ADDR is the proxy's own LAN address there,
the fallback never fired, and both gates read that address rather than
the client's - so a whitelist entry or LAN subnet covering the proxy
exempts everyone behind it, before and after. Distinguishing those
clients needs a trusted-proxy setting the plugin does not have.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
|
||
|---|---|---|
| .. | ||
| luci-plugin-2fa | ||
| luci-plugin-auth-example | ||
| luci-plugin-csp | ||
| luci-plugin-examples | ||