From 4f2083bfbdc680285270c900008667aee35d7832 Mon Sep 17 00:00:00 2001 From: Mads Lorentzen <50207393+MadsLorentzen@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:11:08 +0100 Subject: [PATCH] fix(robots_check): match wildcard rules in linear time and hand curl only http(s) URLs (#779) Two findings on tools/robots_check.py, reported privately by Martin Vlach (@Huge, BudgetScan). GHSA-xhqf-8q8x-4jwm: _match turned every '*' into a regex '.*', so a rule with repeated wildcard segments that does not match the path ("Disallow: /*a*a*a...*b$") sent Python's backtracking engine into exponential time - minutes at 16 segments - in the main process, where no curl timeout applies, on a file the site being checked controls. The matcher is now the reporter's linear table walk in O(len(pattern) * len(path)) with no regex and no backtracking; consecutive '*' match as one. A 200,000-case differential fuzz against the old regex matcher found no change in answers, and three tests pin the prefix, anchored and slash-spanning semantics. GHSA-h8vc-753p-wpmh: gate() rebuilt the target as scheme://host/robots.txt without checking the scheme, so a direct call with file:, gopher: or ftp: passed that scheme to curl -L, which could also follow a redirect to any protocol it was built with. gate() now fails closed with UNCONFIRMED on any scheme but http(s) and on a URL without a host, before curl runs, and curl gets --proto =http,https --proto-redir =http,https. Seven new tests, all failing before this change. Co-authored-by: Claude Fable 5.1 --- CHANGELOG.md | 19 ++++++ tests/test_robots_check.py | 116 +++++++++++++++++++++++++++++++++++++ tools/robots_check.py | 48 +++++++++++++-- 3 files changed, 178 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fd54db1..5aea42c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -479,6 +479,25 @@ per-file diff commands. the `texlive/texlive:latest` leg exists to track current TeX Live, and a digest pin without automated updates would freeze both legs. The test reads the workflow text with the stdlib, so it needs neither zizmor nor PyYAML. +- **`robots_check.py` matches wildcard rules in linear time** (`tools/robots_check.py` `_match`, + `tests/test_robots_check.py`) - every `*` in a robots.txt rule became a regex `.*`, so a rule with + repeated wildcard segments that does not match the path (`Disallow: /*a*a*a...*b$`) sent Python's + backtracking engine into exponential time: 16 segments took minutes of CPU, on a file the site + being checked controls, in the main process where no curl timeout applies. The matcher is now a + table walk in `O(len(pattern) * len(path))` with no regex and no backtracking; consecutive `*` + match as one, and three tests pin the old matcher's answers for prefix, anchored and + slash-spanning rules. Reported privately by @Huge (Martin Vlach), whose linear matcher this is. + +- **`robots_check.py` only ever hands http(s) URLs to curl** (`tools/robots_check.py` `gate`, + `_fetch`, `tests/test_robots_check.py`) - `gate()` rebuilt the target as + `scheme://host/robots.txt` without checking the scheme, so a direct call with `file:`, `gopher:` + or `ftp:` passed that scheme to `curl -L`, which was also free to follow a redirect to any + protocol it was built with. Reach was small (the response is never printed, and the main fetch + path in `fetch_page.py` already allowed only http(s) per hop), but the tool is pre-approved. + `gate()` now fails closed with `UNCONFIRMED` on any scheme but http(s) and on a URL without a + host, before curl runs, and curl itself gets `--proto =http,https --proto-redir =http,https`. + Reported privately by @Huge (Martin Vlach). + - **The ask list now covers the files the allow list runs, and the guard reads load-time commands** (`.claude/settings.json`, `tools/security_guards.py` checks 4 and 5, `SECURITY.md`, diff --git a/tests/test_robots_check.py b/tests/test_robots_check.py index 799fb2cf..94912eeb 100644 --- a/tests/test_robots_check.py +++ b/tests/test_robots_check.py @@ -73,6 +73,47 @@ class TestPathRules(unittest.TestCase): self.assertFalse(allowed(body, "*", "/tag/$var")) self.assertFalse(allowed(body, "*", "/tag/$var/sub")) + def test_repeated_wildcards_do_not_catastrophically_backtrack(self): + # GHSA-xhqf-8q8x-4jwm: every '*' became a regex '.*', so a rule with + # repeated wildcard segments that does not match took exponential time + # in Python's backtracking engine (16 segments: minutes). The matcher + # must run in O(len(pattern) * len(path)) whatever the rule says. + import time + + pat = "/" + "*a" * 13 + "*b$" + body = f"User-agent: *\nDisallow: {pat}\n" + path = "/" + "a" * 28 + "X" + start = time.perf_counter() + res = allowed(body, "*", path) + elapsed = time.perf_counter() - start + self.assertTrue(res) + self.assertLess(elapsed, 0.5, f"_match took {elapsed:.2f}s (catastrophic backtracking)") + + def test_consecutive_wildcards_match_identically_to_single_wildcard(self): + body = "User-agent: *\nDisallow: /docs/*******.pdf$\n" + self.assertFalse(allowed(body, "*", "/docs/cv.pdf")) + self.assertFalse(allowed(body, "*", "/docs/sub/folder/cv.pdf")) + self.assertTrue(allowed(body, "*", "/docs/sub/folder/cv.html")) + + def test_wildcard_semantics_survive_the_matcher_change(self): + # Pinned against the regex matcher's answers: prefix match without '$', + # full match with it, a wildcard spanning slashes and the empty string, + # and a trailing wildcard before the anchor. + body = ( + "User-agent: *\n" + "Disallow: /a*b\n" + "Disallow: /x/*/y$\n" + "Disallow: /end*$\n" + ) + self.assertFalse(allowed(body, "*", "/ab")) + self.assertFalse(allowed(body, "*", "/a/long/path/b/and-more")) + self.assertTrue(allowed(body, "*", "/a")) + self.assertFalse(allowed(body, "*", "/x/anything/at/all/y")) + self.assertTrue(allowed(body, "*", "/x/anything/y/more")) + self.assertFalse(allowed(body, "*", "/end")) + self.assertFalse(allowed(body, "*", "/ending/here")) + self.assertTrue(allowed(body, "*", "/en")) + def test_percent_encoded_query_string_matches_rule_in_gate(self): import robots_check @@ -462,3 +503,78 @@ class TestNonAsciiCurlOutput(unittest.TestCase): if __name__ == "__main__": unittest.main() + + +class TestSchemeHardening(unittest.TestCase): + """GHSA-h8vc-753p-wpmh: only http(s) reaches curl, on the first hop and on + every redirect. + + gate() rebuilt the target as scheme://host/robots.txt but never checked the + scheme, so a direct call with file:, gopher: or ftp: handed that scheme to + curl, and curl -L was free to follow a redirect to any protocol it supports. + """ + + def _gate_without_curl(self, url): + # Record rather than fail inside the spy: gate() swallows every + # exception from _fetch into an UNCONFIRMED verdict, which would pass. + import robots_check + + fetched = [] + original = robots_check._fetch + robots_check._fetch = lambda url, ua: fetched.append(url) or ("", 404) + try: + result = robots_check.gate(url) + finally: + robots_check._fetch = original + self.assertEqual(fetched, [], f"curl was asked for {fetched}") + return result + + def test_non_http_scheme_is_unconfirmed_without_a_fetch(self): + for url in ("file:///etc/passwd", "gopher://127.0.0.1:6379/_", "ftp://x.example/robots.txt", "x.example/path"): + with self.subTest(url=url): + rc, msg = self._gate_without_curl(url) + self.assertEqual(rc, 1) + self.assertTrue(msg.startswith("UNCONFIRMED"), msg) + self.assertIn("scheme", msg) + + def test_missing_host_is_unconfirmed_without_a_fetch(self): + rc, msg = self._gate_without_curl("http:///robots.txt") + self.assertEqual(rc, 1) + self.assertTrue(msg.startswith("UNCONFIRMED"), msg) + + def test_curl_is_pinned_to_http_and_https_on_every_hop(self): + import robots_check + + seen = [] + + class Done: + returncode, stdout, stderr = 0, "\n404", "" + + def fake_run(argv, **kwargs): + seen.append(argv) + return Done() + + original = robots_check.subprocess.run + robots_check.subprocess.run = fake_run + try: + body, code = robots_check._fetch("https://x.example/robots.txt", "Claude-User") + finally: + robots_check.subprocess.run = original + self.assertEqual(code, 404) + argv = seen[0] + self.assertIn("--proto", argv) + self.assertEqual(argv[argv.index("--proto") + 1], "=http,https") + self.assertIn("--proto-redir", argv) + self.assertEqual(argv[argv.index("--proto-redir") + 1], "=http,https") + self.assertLess(argv.index("--proto-redir"), argv.index("--")) + + def test_real_curl_refuses_a_file_url_through_fetch(self): + import shutil + + import robots_check + + if not shutil.which("curl"): + self.skipTest("curl is not installed") + target = (REPO_ROOT / "tools" / "robots_check.py").resolve().as_uri() + with self.assertRaises(RuntimeError): + robots_check._fetch(target, "Claude-User") diff --git a/tools/robots_check.py b/tools/robots_check.py index cf3daf27..45e28f2a 100644 --- a/tools/robots_check.py +++ b/tools/robots_check.py @@ -19,6 +19,9 @@ Rules implemented (RFC 9309), deliberately on the cautious side: * any other failure to read robots.txt leaves permission unconfirmed, and the retry does not happen - including a User-agent, Allow or Disallow line holding U+FFFD, i.e. a byte that is not valid UTF-8 + * only an http or https URL with a host is checked at all; anything else + is unconfirmed without a request, and curl itself is pinned to http(s) + on the first hop and on every redirect Usage: python3 tools/robots_check.py Exit 0 = the retry may proceed. Exit 1 = do not retry; go to escalation step 3. @@ -37,8 +40,12 @@ def _fetch(url, ua): # be read by curl as a flag. gate() rebuilds the target as # scheme://host/robots.txt before calling here, so this is hardening for # direct callers rather than a hole in the gate path itself. + # --proto pins the first hop to http(s) and --proto-redir every redirect + # -L follows, so neither the caller nor the site can steer curl to file:, + # gopher:, ftp: or any other protocol it was built with (GHSA-h8vc-753p-wpmh). r = subprocess.run( - ['curl', '-sS', '-L', '--max-redirs', '5', '--max-time', '12', '-A', ua, + ['curl', '-sS', '-L', '--proto', '=http,https', '--proto-redir', '=http,https', + '--max-redirs', '5', '--max-time', '12', '-A', ua, '-H', 'Accept: text/plain,*/*', '-w', '\n%{http_code}', '--', url], # robots.txt is UTF-8 (RFC 9309). Unpinned, Turkish Windows decoded it as # cp1254, where bytes like 0x81 are undefined: the pipe reader thread died, @@ -124,14 +131,39 @@ def _match(pattern, path): silently skipped - a fail-open on any site that encodes its own rules. Under RFC 9309 section 2.2.2, '$' anchors the end of the URL only when at the end of the pattern; mid-pattern '$' is matched literally. + + Matching is a table walk in O(len(pattern) * len(path)), never a regex. + Each '*' used to become '.*', and a non-matching rule with repeated + wildcard segments ("/*a*a*a...*b$") sent Python's backtracking engine into + exponential time - minutes at 16 segments - on a file the site controls + (GHSA-xhqf-8q8x-4jwm). Consecutive '*' match as one, as RFC 9309 implies. """ if pattern == '': return -1 pattern = unquote(pattern) end_anchor = pattern.endswith('$') - pat = pattern[:-1] if end_anchor else pattern - rx = '^' + ''.join('.*' if c == '*' else re.escape(c) for c in pat) + ('$' if end_anchor else '') - return len(pattern) if re.match(rx, path) else -1 + pat = re.sub(r'\*{2,}', '*', pattern[:-1] if end_anchor else pattern) + # reach[j] is True when the first j pattern characters match the path + # consumed so far. A '*' extends a match over any character (reach[j] + # carries forward) and also matches nothing (reach[j-1] on the same row). + reach = [True] + [False] * len(pat) + for j, c in enumerate(pat, 1): + if c != '*': + break + reach[j] = True + if reach[-1] and not end_anchor: + return len(pattern) + for ch in path: + step = [False] * (len(pat) + 1) + for j, c in enumerate(pat, 1): + if c == '*': + step[j] = step[j - 1] or reach[j] + elif c == ch: + step[j] = reach[j - 1] + reach = step + if reach[-1] and not end_anchor: + return len(pattern) # a prefix match is enough without '$' + return len(pattern) if reach[-1] else -1 def allowed(text, agent, path): g = _groups(text) @@ -144,7 +176,13 @@ def allowed(text, agent, path): return True if best_len < 0 else best_allow def gate(url): - parts = urlsplit(url) + parts = urlsplit(url) # urlsplit lower-cases the scheme + # Only http(s) with a host is a page this tool can have a policy for; any + # other input fails closed before curl is asked anything. + if parts.scheme not in ('http', 'https'): + return 1, f'UNCONFIRMED (unsupported URL scheme {parts.scheme!r}) - do not retry, go to step 3' + if not parts.netloc: + return 1, 'UNCONFIRMED (no host in the URL) - do not retry, go to step 3' path = unquote(parts.path) or '/' if parts.query: path += '?' + unquote(parts.query)