From fb4a7a55331f87d849b7ddeb921bc41a99824959 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Thu, 20 Aug 2026 14:33:33 +0200 Subject: [PATCH] GHSA-56fq-63vj-9992-improved-URL-limitations --- .../blueprint/add_watch_ui/__init__.py | 14 ++- .../blueprint/browser_steps/__init__.py | 8 ++ .../browser_steps/browser_steps.py | 11 +- changedetectionio/content_fetchers/base.py | 6 +- .../content_fetchers/requests.py | 15 ++- changedetectionio/processors/base.py | 42 +++----- changedetectionio/validate_url.py | 100 ++++++++++++++++++ 7 files changed, 157 insertions(+), 39 deletions(-) diff --git a/changedetectionio/blueprint/add_watch_ui/__init__.py b/changedetectionio/blueprint/add_watch_ui/__init__.py index e6ee0bbb..f59645f2 100644 --- a/changedetectionio/blueprint/add_watch_ui/__init__.py +++ b/changedetectionio/blueprint/add_watch_ui/__init__.py @@ -4,6 +4,7 @@ from loguru import logger from changedetectionio import forms from changedetectionio.auth_decorator import login_optionally_required from changedetectionio.store import ChangeDetectionStore +from changedetectionio.validate_url import is_fetch_url_allowed def construct_blueprint(datastore: ChangeDetectionStore): @@ -47,9 +48,18 @@ def construct_blueprint(datastore: ChangeDetectionStore): # Opportunistically sweep snapshots that were fetched but never saved. datastore.cleanup_temporary_watches() + # This endpoint makes the server-side browser fetch an arbitrary URL and returns the + # rendered result (screenshot + xpath element data) straight back in the HTTP response, so + # it is a direct SSRF-with-exfiltration primitive if left unvalidated. It used to only + # check startswith('http://', 'https://'), which skipped the private-IP gate AND the + # backslash/parser-differential rejection of GHSA-rph4-96w6-q594 (GHSA-56fq-63vj-9992). + # Note this fetch never reaches difference_detection_processor.call_browser(), so it gets + # no gating from there - it has to validate for itself. url = (request.args.get('url') or '').strip() - if not url or not url.lower().startswith(('http://', 'https://')): - return make_response('Please enter a valid http(s):// URL', 400) + ok, reason = is_fetch_url_allowed(url) + if not ok: + logger.warning(f"Add-watch snapshot: refused '{url}' - {reason}") + return make_response(reason, 400) # Use whatever fetcher the application is configured to use by default # (e.g. CloakBrowser, Playwright/sockpuppet) so the preview matches real checks. diff --git a/changedetectionio/blueprint/browser_steps/__init__.py b/changedetectionio/blueprint/browser_steps/__init__.py index 58be9a95..07dbf589 100644 --- a/changedetectionio/blueprint/browser_steps/__init__.py +++ b/changedetectionio/blueprint/browser_steps/__init__.py @@ -18,6 +18,7 @@ import os from changedetectionio.store import ChangeDetectionStore from changedetectionio.flask_app import login_optionally_required +from changedetectionio.validate_url import validate_fetch_url_async from loguru import logger browsersteps_sessions = {} @@ -266,6 +267,13 @@ def construct_blueprint(datastore: ChangeDetectionStore): # Resolve the fetcher backend for this watch so we can ask it to launch its own browser # if it supports that (e.g. CloakBrowser, which runs locally rather than via CDP) watch = datastore.data['watching'][watch_uuid] + + # Live preview sessions also return rendered screenshots to the caller and never pass + # through difference_detection_processor.call_browser(), so validate before we even spend + # a browser on it - otherwise a watch pointed at a private address is refused at real check + # time but happily previewed (and exfiltrated) here. + await validate_fetch_url_async(watch.link) + fetcher_name = watch.get_fetch_backend or 'system' if fetcher_name == 'system': fetcher_name = datastore.data['settings']['application'].get('fetch_backend', 'html_requests') diff --git a/changedetectionio/browser_steps/browser_steps.py b/changedetectionio/browser_steps/browser_steps.py index 9b37a10e..062a7b88 100644 --- a/changedetectionio/browser_steps/browser_steps.py +++ b/changedetectionio/browser_steps/browser_steps.py @@ -7,6 +7,7 @@ from loguru import logger from changedetectionio.content_fetchers import SCREENSHOT_MAX_HEIGHT_DEFAULT from changedetectionio.content_fetchers.base import manage_user_agent from changedetectionio.jinja2_custom import render as jinja_render +from changedetectionio.validate_url import validate_fetch_url_async def browser_steps_get_valid_steps(browser_steps: list): if browser_steps is not None and len(browser_steps): @@ -135,7 +136,15 @@ class steppable_browser_interface(): if not value: logger.warning("No URL provided for goto_url action") return None - + + # Every browser navigation we initiate funnels through here - the "Goto URL" step, the + # "Goto site" step, the live Browser Steps UI and the Add Watch snapshot preview - so this + # is the one place that has to enforce the fetch rules. Step values are plain user-supplied + # strings (forms.SingleBrowserStep.optional_value, or the browser_steps[] API field) and are + # NOT covered by the watch URL validation, which is what made file:///etc/passwd readable + # and private-IP SSRF possible via a browser step (GHSA-hm22-wg2m-35v4). + await validate_fetch_url_async(value) + now = time.time() response = await self.page.goto(value, timeout=0, wait_until='load') logger.debug(f"Time to goto URL {time.time()-now:.2f}s") diff --git a/changedetectionio/content_fetchers/base.py b/changedetectionio/content_fetchers/base.py index 12a1c680..f4cd459f 100644 --- a/changedetectionio/content_fetchers/base.py +++ b/changedetectionio/content_fetchers/base.py @@ -219,7 +219,11 @@ class Fetcher(): optional_value=optional_value) await self.screenshot_step(step_n) await self.save_step_html(step_n) - except (Error, TimeoutError) as e: + except (Error, TimeoutError, ValueError) as e: + # ValueError is what validate_fetch_url_async() raises when a step's URL is + # refused (file://, private IP, bad scheme) - report it against the offending + # step number like any other step failure, rather than failing the whole watch + # with an opaque error. logger.debug(str(e)) # Stop processing here raise BrowserStepsStepException(step_n=step_n, original_e=e) diff --git a/changedetectionio/content_fetchers/requests.py b/changedetectionio/content_fetchers/requests.py index 08e8b327..533cdb19 100644 --- a/changedetectionio/content_fetchers/requests.py +++ b/changedetectionio/content_fetchers/requests.py @@ -9,7 +9,7 @@ import asyncio from changedetectionio import strtobool from changedetectionio.content_fetchers.exceptions import BrowserStepsInUnsupportedFetcher, EmptyReply, Non200ErrorCodeReceived from changedetectionio.content_fetchers.base import Fetcher -from changedetectionio.validate_url import is_private_hostname, is_url_private_or_parser_confused +from changedetectionio.validate_url import is_fetch_url_allowed, is_private_hostname, is_url_private_or_parser_confused # "html_requests" is listed as the default fetcher in store.py! @@ -87,13 +87,12 @@ class fetcher(Fetcher): try: # Fresh DNS check at fetch time — catches DNS rebinding regardless of add-time cache. - # Validates every hostname both urlparse and urllib3 see, so parser-differential - # payloads (GHSA-rph4-96w6-q594) cannot smuggle an internal target past the gate. - if not allow_iana_restricted: - if is_url_private_or_parser_confused(url): - raise Exception(f"Fetch blocked: '{url}' resolves to a private/reserved IP address " - f"or contains a parser-differential payload. " - f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow.") + # Shared with every other fetch entry point, so the scheme allowlist and the + # parser-differential rejection (GHSA-rph4-96w6-q594) stay in step here too. + # Per-redirect-hop re-validation is done separately in the loop below. + ok, reason = is_fetch_url_allowed(url) + if not ok: + raise Exception(reason) r = session.request(method=request_method, data=request_body.encode('utf-8') if type(request_body) is str else request_body, diff --git a/changedetectionio/processors/base.py b/changedetectionio/processors/base.py index 55f45bd9..cfbd7a6d 100644 --- a/changedetectionio/processors/base.py +++ b/changedetectionio/processors/base.py @@ -1,11 +1,8 @@ -import asyncio -import re import hashlib from changedetectionio.browser_steps.browser_steps import browser_steps_get_valid_steps from changedetectionio.content_fetchers.base import Fetcher -from changedetectionio.strtobool import strtobool -from changedetectionio.validate_url import is_private_hostname, is_url_private_or_parser_confused +from changedetectionio.validate_url import validate_fetch_url_async from copy import deepcopy from abc import abstractmethod import os @@ -97,22 +94,19 @@ class difference_detection_processor(): logger.warning(f"Failed to read checksum file for {self.watch_uuid}: {e}") self.last_raw_content_checksum = None - async def validate_iana_url(self): - """Pre-flight SSRF check — runs DNS lookup in executor to avoid blocking the event loop. - Covers all fetchers (requests, playwright, puppeteer, plugins) since every fetch goes - through call_browser(). + async def validate_url_is_fetchable(self): + """Pre-flight fetch gate for the regular check path (all fetchers, since they all come + through call_browser()). The scheme/file:///private-IP rules live in + validate_url.is_fetch_url_allowed() so that the fetch paths which do NOT come through + here - the live Browser Steps UI, the Add Watch snapshot preview, and individual + 'Goto URL' browser steps - enforce exactly the same rules. """ - if strtobool(os.getenv('ALLOW_IANA_RESTRICTED_ADDRESSES', 'false')): - return - loop = asyncio.get_running_loop() - # Use the parser-agnostic check so urlparse/urllib3 differentials (GHSA-rph4-96w6-q594) - # can't slip a private/internal hostname past this pre-flight gate. - if await loop.run_in_executor(None, is_url_private_or_parser_confused, self.watch.link): - raise Exception( - f"Fetch blocked: '{self.watch.link}' resolves to a private/reserved IP address " - f"or contains a parser-differential payload. " - f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow." - ) + try: + await validate_fetch_url_async(self.watch.link) + except ValueError as e: + # Re-raised as a plain Exception so it lands in the watch's last_error like every other + # fetch failure, instead of looking like an internal type error. + raise Exception(str(e)) from e def _consume_preloaded_fetch(self): """One-shot: if the Add Watch page parked a freshly-fetched snapshot for this @@ -178,14 +172,8 @@ class difference_detection_processor(): url = self.watch.link - # Protect against file:, file:/, file:// access, check the real "link" without any meta "source:" etc prepended. - if re.search(r'^file:', url.strip(), re.IGNORECASE): - if not strtobool(os.getenv('ALLOW_FILE_URI', 'false')): - raise Exception( - "file:// type access is denied for security reasons." - ) - - await self.validate_iana_url() + # Scheme allowlist (file:// etc), parser-differential and private/reserved IP checks. + await self.validate_url_is_fetchable() # Proxy ID "key" preferred_proxy_id = preferred_proxy_id if preferred_proxy_id else self.datastore.get_preferred_proxy_for_watch( diff --git a/changedetectionio/validate_url.py b/changedetectionio/validate_url.py index d8e7b51f..47d46323 100644 --- a/changedetectionio/validate_url.py +++ b/changedetectionio/validate_url.py @@ -133,6 +133,106 @@ def is_url_private_or_parser_confused(url): return False +def is_fetch_url_allowed(url): + """THE single gate for "is the server allowed to fetch this URL?". + + Returns (ok: bool, reason: str) — `reason` is safe to show the user. + + Call this from EVERY entry point that causes a server-side fetch. The checks used to live + inline in difference_detection_processor.call_browser(), on the documented assumption that + "every fetch goes through call_browser()". That stopped being true once the live Browser + Steps UI and the Add Watch snapshot preview grew their own fetch paths — each silently + skipped both the file:// and the private-IP gate (GHSA-hm22-wg2m-35v4, GHSA-56fq-63vj-9992). + Rather than re-assert that invariant, every fetch path now calls this function. + + Layers, in order: + 1. Render Jinja2 and strip the 'source:' meta prefix, so what gets checked is what the + browser/requests library will actually be handed. Stripping is load-bearing, not + cosmetic: urlparse('source:http://127.0.0.1/') reports NO hostname at all, so an + unstripped value sails straight past the private-IP check in step 5. + 2. file:// refused unless ALLOW_FILE_URI=true. Checked explicitly rather than leaning on + is_safe_valid_url()'s scheme allowlist, because an operator who loosened + SAFE_PROTOCOL_REGEX for some other scheme should not silently get local file reads too. + 3. Backslash rejection (GHSA-rph4-96w6-q594) — unconditional, including when the operator + has opted into private addresses. + 4. is_safe_valid_url() — scheme allowlist, '<>' rejection, validators.url(). + 5. Private / loopback / link-local / reserved IP rejection, unless + ALLOW_IANA_RESTRICTED_ADDRESSES=true. + + Step 5 performs DNS resolution and therefore blocks. From async code call + validate_fetch_url_async() instead so the event loop keeps turning. + + Note this validates one URL, not a redirect chain. content_fetchers/requests.py follows + redirects manually and re-checks each hop; the Chromium-based fetchers cannot do that yet, + so an open redirect on a public host remains a known gap for those backends. + """ + import os + import re + from changedetectionio.strtobool import strtobool + from changedetectionio.jinja2_custom import render as jinja_render + + if not url or not isinstance(url, str) or not url.strip(): + return False, "No URL specified." + + url = url.strip() + + # Jinja2 first — the fetch uses the rendered value, so the rendered value is what must pass. + if '{%' in url or '{{' in url: + try: + url = jinja_render(template_str=url).strip() + except Exception as e: + logger.error(f"URL '{url}' is not valid Jinja2? {str(e)}") + return False, "The URL contains invalid Jinja2 template syntax." + + # 'source:' is our own meta prefix meaning "return the raw source"; it is not part of the + # URL that gets fetched. Must be removed before any hostname parsing happens - see step 1 above. + url = re.sub(r'^source:', '', url, flags=re.IGNORECASE).strip() + + if re.match(r'^file:', url, re.IGNORECASE) and not strtobool(os.getenv('ALLOW_FILE_URI', 'false')): + logger.warning(f"Fetch blocked: file:// access is disabled (ALLOW_FILE_URI) - '{url}'") + return False, "file:// type access is denied for security reasons." + + # Checked here in its own right, not left to is_safe_valid_url()/is_url_private_or_parser_confused(): + # a backslash is never legitimate in a URL, so it must be refused even when the operator has + # opted into private addresses with ALLOW_IANA_RESTRICTED_ADDRESSES (GHSA-rph4-96w6-q594). + if '\\' in url: + logger.warning(f"Fetch blocked: '{url}' contains a backslash (parser-differential SSRF vector).") + return False, f"Fetch blocked: '{url}' contains a parser-differential payload (backslash)." + + if not is_safe_valid_url(url): + return False, "The URL is invalid or uses an unsupported protocol." + + if not strtobool(os.getenv('ALLOW_IANA_RESTRICTED_ADDRESSES', 'false')): + if is_url_private_or_parser_confused(url): + return False, ( + f"Fetch blocked: '{url}' resolves to a private/reserved IP address " + f"or contains a parser-differential payload. " + f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow." + ) + + return True, '' + + +def validate_fetch_url(url): + """is_fetch_url_allowed() as an assertion - raises ValueError with the reason. + + Use at fetch entry points that should abort loudly (the message surfaces to the user as a + watch error or an HTTP 400). Blocks on DNS; from async code use validate_fetch_url_async(). + """ + ok, reason = is_fetch_url_allowed(url) + if not ok: + raise ValueError(reason) + + +async def validate_fetch_url_async(url): + """validate_fetch_url() with the DNS lookup pushed to a thread so the event loop isn't blocked.""" + import asyncio + loop = asyncio.get_running_loop() + ok, reason = await loop.run_in_executor(None, is_fetch_url_allowed, url) + if not ok: + raise ValueError(reason) + + def is_llm_api_base_safe(api_base): """SSRF guard for the LLM `api_base` setting (GHSA-jrxm-qjfh-g54f).