diff --git a/CHANGELOG.md b/CHANGELOG.md index 64cd8cba..5a27f81f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,19 @@ ADR-007 and ADR-002. refresh token that minted new ones for thirty days. `/api/auth/refresh` and the API-token path already checked `isactive`; the loader now does too. +- **The login rate limit could be reset by a header the caller writes.** + `client_ip()` read the first `X-Forwarded-For` hop unconditionally, so + rotating it per request bought an unbounded budget, and every audit row + recorded the forged address. This walked around `TRUSTED_PROXY_COUNT`, which + exists to decide whether a proxy is really in front: the app deliberately + leaves ProxyFix off by default for exactly this reason, and then read the + header anyway. Resolution is now from `remote_addr`, which ProxyFix has + already filled from the header where a proxy is configured. Two plugins had + hand-rolled the same mistake for their own limiters and now share the helper, + which is on `shopdb.api` so the third one does not. **A site behind IIS must + set `TRUSTED_PROXY_COUNT`** or its whole floor shares one rate-limit bucket; + INSTALL-WINDOWS-IIS.md now says so. + - **The published site's API documentation page had no spec to show.** `shopdb/core/api/docs.py` reads `docs/openapi.json` at runtime to serve `/api/docs`, and publication excluded the whole of `docs/` because the prose diff --git a/docs/CONTRACT-STABILITY.md b/docs/CONTRACT-STABILITY.md index 899ee32d..c2cdcb62 100644 --- a/docs/CONTRACT-STABILITY.md +++ b/docs/CONTRACT-STABILITY.md @@ -45,6 +45,9 @@ Recorded in the comment block in `shopdb/__init__.py`: | 0.23.0 | Added `mint_service_token`, `list_service_tokens` and `revoke_service_tokens` so a plugin can provision, list and revoke a single-scope managed service token without importing core token internals. The endpoints plugin uses them for a UI-managed, regenerable agent key scoped to `collector.ingest.endpoints` alone - narrow on purpose, so rotating or revoking it cannot disturb the computers or GE-Enforce collectors, and revocation is an instant kill switch on agent ingest | additive surface (minor) | | 0.24.0 | Added `collector_authorized(pluginname)` so a plugin can gate an agent-facing route that lives OUTSIDE the generic collector blueprint on the same per-plugin key and kill switch the ingest route uses. The endpoints command channel needs it: the agent long-polls `/api/endpoints/commands/next` and posts results back, and those are agent callbacks rather than JWT routes. Without it a plugin would reach into core collector internals to authorize them | additive surface (minor) | | 0.25.0 | Added `caller_holds(permission)`, which answers whether the current caller holds a permission WITHOUT refusing the request - the complement to `require_permission` for a route that serves everyone but should only offer what the caller can act on. The endpoints services panel is the case: it emitted Start/Stop row actions to every reader, because the emission never consulted the caller. Clicking them was always refused server-side, so nothing was exploitable, but offering a read-only viewer controls over a shop-floor PC is the wrong signal, and it becomes a real problem once a site grants that panel to guests. Guests are included in the answer, since a signed-out visitor holds whatever the guest role was granted | additive surface (minor) | +| 0.26.0 | Dashboard cards may declare `shortname`. The dashboard merged its per-source cards into one worst-first queue, and a card's `title` is written as a heading ("Endpoints with crashes or shutdowns") which as a row badge ran wider than the hostname it qualified. Optional, defaults to the title, so a plugin that does not declare one is unaffected | additive field (minor) | +| 0.27.0 | Added `Script` so a plugin can read the ShopDB-hosted script library that backs endpoint `run_script` delivery without importing core model internals. The endpoints plugin resolves a delivered script by name and serves its body by digest to the agent | additive surface (minor) | +| 0.28.0 | Added `client_ip` and `strip_port` so a plugin does not hand-roll caller-IP extraction. Two plugins had, and both read the first `X-Forwarded-For` hop, which the caller supplies - so a rate limit keyed on it was defeated by rotating the header. The helper resolves from `remote_addr`, which ProxyFix has already filled from the header when `TRUSTED_PROXY_COUNT` says a proxy is really in front | additive surface (minor) | The source comment block documents 0.3.0, 0.4.0, 0.6.0, 0.7.0, 0.9.0, 0.10.0, 0.11.0, 0.16.0, 0.18.0 and 0.19.0; 0.12.0 through 0.15.0 and 0.17.0 are recorded diff --git a/docs/INSTALL-WINDOWS-IIS.md b/docs/INSTALL-WINDOWS-IIS.md index 34a7354c..83290757 100644 --- a/docs/INSTALL-WINDOWS-IIS.md +++ b/docs/INSTALL-WINDOWS-IIS.md @@ -88,12 +88,28 @@ SECRET_KEY=<64+ random chars> JWT_SECRET_KEY= DATABASE_URL=mysql+pymysql://shopdb:CHANGE_ME@:3306/shopdb_flask?charset=utf8mb4 CORS_ORIGINS=http:// +TRUSTED_PROXY_COUNT=1 ``` Generate a key: `venv\Scripts\python -c "import secrets;print(secrets.token_urlsafe(64))"`. Production **refuses to boot** if any of `SECRET_KEY`, `JWT_SECRET_KEY`, `DATABASE_URL`, `CORS_ORIGINS` is missing or a dev default. +**`TRUSTED_PROXY_COUNT` matters on this install and nowhere else.** IIS is a +reverse proxy, so the app sees IIS's connection rather than the browser's. Set +it to the number of proxies actually in front (1 for a single IIS), and the app +enables ProxyFix, which resolves the real client address, scheme and host from +the `X-Forwarded-*` headers IIS sets. + +Leave it at 0 behind IIS and the app is not wrong about who the client is so +much as blind to it: every request looks like it came from IIS itself. The login +rate limit is keyed on the client address, so the whole site shares one bucket, +and the audit log records the proxy for every action. + +Do NOT set it when nothing is in front of the app. ProxyFix trusts those headers, +and a caller can send them - which is the reason it is off by default rather +than always on. + --- ## 5. Preflight (catch problems before installing) diff --git a/plugins/geenforce/api/routes.py b/plugins/geenforce/api/routes.py index f3d6217a..c0ee5df4 100644 --- a/plugins/geenforce/api/routes.py +++ b/plugins/geenforce/api/routes.py @@ -21,7 +21,7 @@ from sqlalchemy.exc import IntegrityError from shopdb.api import ( db, cache, success_response, error_response, ErrorCodes, require_permission, require_any_permission, - authorized_service_token, Setting, Application, + authorized_service_token, Setting, Application, client_ip, ) SHAREROOT_SETTING = 'geenforce_share_root' @@ -207,12 +207,14 @@ PAYLOAD_DOWNLOAD_RATELIMIT_WINDOW_SECONDS = 60 def _client_ip(): - """Caller IP for rate limiting, honoring the first X-Forwarded-For hop - (mirrors shopdb.core.api.auth._login_ip).""" - forwarded = request.headers.get('X-Forwarded-For') - if forwarded: - return forwarded.split(',')[0].strip() - return request.remote_addr or 'unknown' + """Caller IP for rate limiting. + + Uses the shared helper, which resolves from remote_addr. The raw + X-Forwarded-For hop is client-supplied, so keying a rate limit on it lets a + caller rotate the header and get an unbounded budget - the same reasoning + _trusted_client_ip() above already applies to the allowlist. + """ + return client_ip(request) or 'unknown' def _payload_max_bytes(): diff --git a/plugins/tools/api/routes.py b/plugins/tools/api/routes.py index 9dd4231d..8c31acbe 100644 --- a/plugins/tools/api/routes.py +++ b/plugins/tools/api/routes.py @@ -37,7 +37,7 @@ from flask import current_app from flask_jwt_extended import jwt_required, get_jwt_identity, current_user from shopdb.api import ( - db, cache, Setting, + db, cache, Setting, client_ip, success_response, error_response, ErrorCodes, ) @@ -117,8 +117,9 @@ def _guest_ratelimited(): person has more addresses - but it stops an open endpoint being an easy place to park data or fill a disk. """ - address = (request.headers.get('X-Forwarded-For', '').split(',')[0].strip() - or request.remote_addr or 'unknown') + # Not the raw X-Forwarded-For hop: the caller sends that, so rotating it + # gives an unbounded budget and the limit stops being a limit. + address = client_ip(request) or 'unknown' bucket = int(_utcnow().timestamp()) // GUEST_CREATE_WINDOW_SECONDS key = f'securesendguest:{address}:{bucket}' used = cache.get(key) or 0 diff --git a/shopdb/api/__init__.py b/shopdb/api/__init__.py index 6393ab64..19d7626e 100644 --- a/shopdb/api/__init__.py +++ b/shopdb/api/__init__.py @@ -104,6 +104,12 @@ def collector_authorized(pluginname: str): # as script in this origin - see shopdb/utils/uploads.py. from shopdb.utils.uploads import send_upload, harden_upload_response +# Caller IP for rate limiting and audit. On the surface so a plugin does not +# reimplement it: the obvious hand-rolled version reads X-Forwarded-For, which +# the caller supplies, so a rate limit keyed on it can be rotated away to +# nothing. See shopdb/utils/clientip.py. +from shopdb.utils.clientip import client_ip, strip_port + # Import-mode helpers: preserve legacy timestamps during a bulk data import from shopdb.utils.import_mode import ( apply_import_timestamps, @@ -346,6 +352,8 @@ __all__ = [ # Serving uploads 'send_upload', 'harden_upload_response', + 'client_ip', + 'strip_port', # Import-mode helpers 'apply_import_timestamps', 'import_mode_active', diff --git a/shopdb/utils/clientip.py b/shopdb/utils/clientip.py index 4b22e826..c61eb7fd 100644 --- a/shopdb/utils/clientip.py +++ b/shopdb/utils/clientip.py @@ -6,6 +6,23 @@ 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. """ @@ -26,14 +43,11 @@ def strip_port(ipaddress): def client_ip(request_obj): - """First-hop client IP behind one proxy, port stripped. + """Client IP, port stripped, from the connecting peer. - Honors the first X-Forwarded-For hop, else remote_addr. Returns None only - when neither is present (callers that need a non-null key coerce it). + 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). """ - forwarded = request_obj.headers.get('X-Forwarded-For') - if forwarded: - raw = forwarded.split(',')[0].strip() - else: - raw = request_obj.remote_addr - return strip_port(raw) + return strip_port(request_obj.remote_addr) diff --git a/tests/test_core/test_auth_ratelimit.py b/tests/test_core/test_auth_ratelimit.py index 84ab1e71..1e70b1f2 100644 --- a/tests/test_core/test_auth_ratelimit.py +++ b/tests/test_core/test_auth_ratelimit.py @@ -32,11 +32,18 @@ def ratelimit_on(app): def _bad_login(client, ip): - """Attempt a login for a non-existent user from a given source IP.""" + """Attempt a login for a non-existent user from a given source IP. + + The caller is set through REMOTE_ADDR, the peer that actually connected, + because that is what the limiter keys on. It used to be sent as + X-Forwarded-For, which meant these tests changed identity using the very + header a real attacker would rotate to escape the limit - so the suite + could not have told the difference. + """ return client.post( '/api/auth/login', json={'username': 'ghost', 'password': 'wrong'}, - headers={'X-Forwarded-For': ip}, + environ_base={'REMOTE_ADDR': ip}, ) diff --git a/tests/test_core/test_clientip.py b/tests/test_core/test_clientip.py index b7d7dedd..a77da169 100644 --- a/tests/test_core/test_clientip.py +++ b/tests/test_core/test_clientip.py @@ -1,4 +1,12 @@ -"""strip_port / client_ip normalization (ARR forwards clientip:port).""" +"""client_ip: which address the app treats as the caller, and port normalization. + +The answer is remote_addr. X-Forwarded-For is written by the caller, and the +login rate limit, the account lockout and the audit log all key on this value, +so honoring the header let a caller rotate it for an unbounded budget and an +untraceable trail. Where a proxy really is in front, TRUSTED_PROXY_COUNT turns +on ProxyFix and the header arrives already resolved into remote_addr - ports +included, because ARR forwards clientip:port. +""" from shopdb.utils.clientip import strip_port, client_ip @@ -34,12 +42,94 @@ class _FakeRequest: self.remote_addr = remote_addr -def test_client_ip_prefers_forwarded_first_hop_stripped(): - request_obj = _FakeRequest(headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'}, - remote_addr='127.0.0.1') +def test_client_ip_ignores_the_forwarded_header(): + """The caller sends X-Forwarded-For, so it cannot decide who the caller is. + + This test used to assert the opposite. The login rate limit, the account + lockout and the audit log all key on client_ip, so honoring the header let + a caller rotate it for an unbounded budget and an untraceable trail. Where + a proxy really is in front, TRUSTED_PROXY_COUNT turns on ProxyFix and the + header reaches us already resolved into remote_addr. + """ + request_obj = _FakeRequest( + headers={'X-Forwarded-For': '10.0.0.5:51000, 10.0.0.1'}, + remote_addr='127.0.0.1') + assert client_ip(request_obj) == '127.0.0.1' + + +def test_client_ip_strips_a_port_proxyfix_passed_through(): + """ARR writes clientip:port into the header and ProxyFix copies it verbatim + into remote_addr, so the port still has to come off.""" + request_obj = _FakeRequest(headers={}, remote_addr='10.0.0.5:51000') assert client_ip(request_obj) == '10.0.0.5' def test_client_ip_falls_back_to_remote_addr_stripped(): request_obj = _FakeRequest(headers={}, remote_addr='192.168.1.9:60000') assert client_ip(request_obj) == '192.168.1.9' + + +# --------------------------------------------------------------------------- +# Through a real request, and through the login limiter that keys on this. +# --------------------------------------------------------------------------- +def test_the_forwarded_header_does_not_decide_the_client_ip(app): + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', headers={'X-Forwarded-For': '10.0.0.99'}, + environ_base={'REMOTE_ADDR': '203.0.113.7'}): + from flask import request + assert client_ip(request) == '203.0.113.7' + + +def test_the_peer_address_decides_the_client_ip(app): + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', environ_base={'REMOTE_ADDR': '203.0.113.8'}): + from flask import request + assert client_ip(request) == '203.0.113.8' + + +def test_a_forwarded_port_is_still_stripped(app): + """IIS ARR writes "clientip:port" and ProxyFix passes it through verbatim, + so the port has to come off remote_addr or every request looks like a new + client.""" + from shopdb.utils.clientip import client_ip + + with app.test_request_context( + '/', environ_base={'REMOTE_ADDR': '203.0.113.9:54321'}): + from flask import request + assert client_ip(request) == '203.0.113.9' + + +def test_rotating_the_forwarded_header_does_not_reset_the_login_limiter(app, + client, + db): + """The budget follows the connection, not a header the caller writes. + + Sprayed across many usernames on purpose: the per-account lockout bounds + guessing at ONE account, so the IP limiter is the only thing standing + between a caller and unlimited attempts spread across the directory. That + is what rotating the header used to defeat. + """ + caller = {'REMOTE_ADDR': '203.0.113.50'} + previous = dict(app.config) + app.config['AUTH_RATELIMIT_ENABLED'] = True + app.config['AUTH_RATELIMIT_MAX'] = 3 + try: + seen = set() + for hop in range(10): + response = client.post( + '/api/auth/login', + headers={'X-Forwarded-For': '198.51.100.{}'.format(hop)}, + environ_base=caller, + json={'username': 'nobody{}'.format(hop), 'password': 'wrong'}) + seen.add(response.status_code) + if response.status_code == 429: + break + finally: + app.config['AUTH_RATELIMIT_ENABLED'] = previous.get( + 'AUTH_RATELIMIT_ENABLED', False) + app.config['AUTH_RATELIMIT_MAX'] = previous.get('AUTH_RATELIMIT_MAX', 30) + assert 429 in seen, 'rotating X-Forwarded-For bought an unbounded budget' diff --git a/tests/test_plugins/test_geenforce_payload_hardening.py b/tests/test_plugins/test_geenforce_payload_hardening.py index 1d7bc508..c25f0a3c 100644 --- a/tests/test_plugins/test_geenforce_payload_hardening.py +++ b/tests/test_plugins/test_geenforce_payload_hardening.py @@ -107,21 +107,47 @@ def test_payload_download_is_rate_limited(client, app, auth_headers): def test_rate_limit_is_per_ip(client, app, auth_headers): - """A second caller (different X-Forwarded-For) has its own budget: one IP - being throttled must not throttle everyone.""" + """A second caller has its own budget: one IP being throttled must not + throttle everyone. + + Callers are distinguished by REMOTE_ADDR, the peer that actually connected, + which is what ProxyFix fills from X-Forwarded-For when a proxy is really in + front of this instance. + """ sha = _store(app, b'per-ip blob') key = _fetch_key(client, auth_headers) - hot = {**key, 'X-Forwarded-For': '203.0.113.202'} - cool = {**key, 'X-Forwarded-For': '203.0.113.203'} + hot = {'REMOTE_ADDR': '203.0.113.202'} + cool = {'REMOTE_ADDR': '203.0.113.203'} with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1, GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300): assert client.get(f'/api/geenforce/payload/{sha}', - headers=hot).status_code == 200 + headers=key, environ_base=hot).status_code == 200 assert client.get(f'/api/geenforce/payload/{sha}', - headers=hot).status_code == 429 + headers=key, environ_base=hot).status_code == 429 # Fresh IP still gets its first request through. assert client.get(f'/api/geenforce/payload/{sha}', - headers=cool).status_code == 200 + headers=key, environ_base=cool).status_code == 200 + + +def test_rate_limit_cannot_be_reset_by_forging_a_forwarded_header( + client, app, auth_headers): + """The budget follows the connection, not a header the caller writes. + + The limiter used to key on the first X-Forwarded-For hop, so rotating that + header per request bought an unbounded budget and the limit stopped being + a limit. + """ + sha = _store(app, b'forged header blob') + key = _fetch_key(client, auth_headers) + caller = {'REMOTE_ADDR': '203.0.113.210'} + with _config(app, GEENFORCE_PAYLOAD_RATELIMIT_MAX=1, + GEENFORCE_PAYLOAD_RATELIMIT_WINDOW_SECONDS=300): + assert client.get(f'/api/geenforce/payload/{sha}', headers=key, + environ_base=caller).status_code == 200 + for hop in ('198.51.100.1', '198.51.100.2', '198.51.100.3'): + spoofed = {**key, 'X-Forwarded-For': hop} + assert client.get(f'/api/geenforce/payload/{sha}', headers=spoofed, + environ_base=caller).status_code == 429 def test_rate_limit_can_be_disabled(client, app, auth_headers):