security: dial the validated address, bound audit response bodies (3.9.3) - #20
Conversation
…odies (3.9.3) Two findings from the ClawHub audit of 3.9.2, both reported independently by AIG and ClawScan. DNS rebinding: the address rule resolved a hostname to check it and then let urllib resolve it again to connect, so an attacker controlling the name could answer a public address for the check and a private one microseconds later. 3.9.0 documented this as a known narrow window and shipped it anyway - but the skill's own permissions block claimed address enforcement, so the gap was between the claim and the behaviour, which is the kind of gap a disclosure is supposed to close rather than open. _assert_public_host now returns the addresses it approved, and everything that enforces the rule dials one of them: both openers go through pinned HTTP/HTTPS connection classes, and the TLS expiry check goes through connect_public_tls instead of opening its own socket. Certificate validation and SNI still use the hostname, so pinning changes which endpoint is reached and nothing about how it is verified. Response bodies: site_audit read both the success and the HTTP-error body with a bare read(), letting any server it visits decide how much memory this process uses. Both are capped at 5 MB. The audit parses the document head, so truncating a giant page costs nothing. Proven offline (this sandbox has no DNS): the pinned opener refuses a loopback target at connect time, transports a real request end to end when the rule allows the address, and the socket's peer is the validated address rather than a fresh lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1e3f546e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return urllib.request.build_opener( | ||
| _PinnedHTTPHandler(), _PinnedHTTPSHandler(), _ValidatingRedirectHandler(validator)) |
There was a problem hiding this comment.
When HTTP_PROXY or HTTPS_PROXY is configured, build_opener still installs urllib's default ProxyHandler, which rewrites the connection host to the proxy before these handlers run. _PinnedHTTPConnection therefore validates and dials the proxy while sending the target hostname in the absolute request URI, and _PinnedHTTPSConnection sends that hostname in CONNECT; the proxy then resolves the target independently, reopening the DNS-rebinding path this change is intended to close. A private enterprise proxy is instead rejected as an unsafe host, breaking both audits and remote-media downloads. Disable proxy handling for these security-sensitive openers or explicitly pin the origin address through the proxy.
Useful? React with 👍 / 👎.
urllib's default ProxyHandler rewrites the connection host to the proxy before the pinned handlers run, so _PinnedHTTPConnection validated and dialled the PROXY while the real target travelled on in the absolute request URI (http) or CONNECT (https). The proxy then resolved that target itself, with none of these rules applied - the pin was enforcing the address of the wrong host, which is worse than no pin, because the disclosure says the rule is enforced. A private enterprise proxy was also refused outright as an unsafe address. Reproduced before fixing: with HTTP_PROXY set, a request to the public literal 93.184.216.34 was refused with "Refusing host '127.0.0.1'" - naming the proxy, not the target, which is the bug in one line. The enforcing openers now pass an empty ProxyHandler, which is what stops build_opener installing the default one. WP_ALLOW_PROXY=1 restores proxy use for an environment where the proxy is the only egress, and warns that address enforcement is no longer end to end - the proxy decides what it connects to from there. Proven both ways against a local proxy and a local target: by default the target is hit directly and the proxy is never touched; with WP_ALLOW_PROXY=1 the proxy serves the response and the warning is printed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed in Reproduced before fixing. With The refusal names the proxy, not the target — the bug in one line. With a public proxy it would have dialled the proxy happily and let it resolve the target, and your second point lands too: a private enterprise proxy is refused outright as an unsafe address, which is a confusing way to fail. The enforcing openers now pass an empty Proven both ways against a local proxy and a local target: Two of my own test bugs surfaced writing this, both worth noting: an empty 109 tests passing. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4aea1d6f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _PROBE_OPENER = _address_enforcing_opener(validate_probe_url) | ||
| _MEDIA_OPENER = _address_enforcing_opener(validate_remote_url) |
There was a problem hiding this comment.
When WP_ALLOW_PROXY=1 and a proxy variable is configured, importing security constructs both openers and therefore emits this warning twice. Because nearly every CLI imports this module, unrelated commands such as create_post also print two warnings even though they never use the audit/media openers. This should be emitted once and only when a proxy-enabled protected fetch is actually performed.
Useful? React with 👍 / 👎.
| tport, _ = self._server("target", b"from-target") | ||
| env = {"HTTP_PROXY": f"http://127.0.0.1:{pport}", "WP_ALLOW_PROXY": "1"} | ||
| buf = io.StringIO() | ||
| with _mock.patch.dict(os.environ, env, clear=False): |
There was a problem hiding this comment.
On hosts whose environment defines no_proxy or NO_PROXY for 127.0.0.1 (a common development and CI configuration), clear=False retains that setting, so urllib bypasses the local proxy and returns b'from-target'; the subsequent assertion fails. Override both bypass variables or isolate the environment completely so this test actually verifies the opt-in proxy path.
Useful? React with 👍 / 👎.
Building the openers happens at import, and there are two of them, so the proxy warning fired twice in every CLI that imports this module - including create_post, update_post and the rest, which never touch these openers and whose behaviour the warning does not describe. A warning that appears on unrelated commands is one users learn to ignore. _warn_proxy_in_use fires once, from urlopen_probe and fetch_https_media: the two paths that actually claim end-to-end address enforcement and, under WP_ALLOW_PROXY=1, no longer have it. Also fixes my own test: patching os.environ with clear=False left a host's no_proxy/NO_PROXY in place, and a host that exempts 127.0.0.1 (common in dev and CI) would bypass the local proxy, so the opt-in test would have passed for the wrong reason. Both bypass variables are now cleared explicitly. Reported by Codex on PR #20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both fixed in The warning. You are right that it was attached to the wrong thing. Building the openers happens at import and there are two of them, so it fired twice in every CLI that imports the module —
The test. Also right, and it would have been a test that passes for the wrong reason on exactly the machines most likely to run it. 113 tests passing. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7b65bc2bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handlers = [_PinnedHTTPHandler(), _PinnedHTTPSHandler(), _ValidatingRedirectHandler(validator)] | ||
| if env.get("WP_ALLOW_PROXY") != "1": | ||
| handlers.insert(0, urllib.request.ProxyHandler({})) |
There was a problem hiding this comment.
When WP_ALLOW_PROXY=1 and the configured proxy resolves to a private or loopback address—as is common for enterprise proxies—ProxyHandler rewrites the connection host to that proxy, but the opener still uses _PinnedHTTPConnection/_PinnedHTTPSConnection, so _connect_to_public_host rejects the proxy before sending the request. Thus the documented escape hatch only works with globally routable proxies and does not restore access in the proxy-only environments it targets. Fresh evidence after e7b65bc is that a local proxy still raises SafetyError: Refusing host '127.0.0.1'; the opt-in path needs non-pinning connection handlers.
Useful? React with 👍 / 👎.
| env = {"HTTP_PROXY": f"http://127.0.0.1:{pport}", "WP_ALLOW_PROXY": "1", | ||
| "no_proxy": "", "NO_PROXY": ""} | ||
| with _mock.patch.dict(os.environ, env, clear=False): |
There was a problem hiding this comment.
On hosts that define lowercase http_proxy, this clear=False patch leaves it intact, and urllib prefers the lowercase value over the newly supplied uppercase HTTP_PROXY. The test then connects to the host proxy instead of the local test server and fails (this occurs in the current review environment). Fresh evidence after e7b65bc is therefore a different retained variable from the fixed no_proxy case; either clear the environment or override both uppercase and lowercase proxy variables.
Useful? React with 👍 / 👎.
With WP_ALLOW_PROXY=1 the opener still used the pinned connection classes, so _connect_to_public_host validated the rewritten connection host - the proxy - and refused it. An enterprise proxy normally sits on a private address, which is exactly what those classes reject, so the documented escape hatch failed for the only situation it was written for. Pinning a connection whose host has been rewritten to the proxy enforces nothing anyway: it checks the address of the proxy while the real target rides along in the absolute request URI or CONNECT. Under the opt-in the opener now carries URL-level validation only - the initial URL and every redirect target are still checked - and the warning already says addresses are not enforced from there. Demonstrated: BEFORE (pinned + allowed proxy): Refusing host '127.0.0.1'; unsafe address AFTER (opt-in, no pinning) : b'from-proxy' Also fixes the opt-in test properly this time: it now clears EVERY proxy variable before setting its own, because urllib prefers lowercase http_proxy over the uppercase name the test was setting - which is why the previous fix still failed in Codex's environment. Reported by Codex on PR #20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both fixed in With And pinning there enforces nothing anyway: it checks the address of the proxy while the real target rides along in the absolute request URI or CONNECT. Under the opt-in the opener now carries URL-level validation only — initial URL and every redirect target still checked — which is what the warning already claims. The test, third time. You were right that fresh evidence was needed rather than a re-read: urllib prefers lowercase That is twice in a row that this test passed locally for a reason that did not hold elsewhere — worth saying plainly rather than quietly fixing. 115 tests passing. @codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
From the ClawHub audit of 3.9.2. Two findings, both Medium, both reported independently by AIG and ClawScan.
1. DNS rebinding — the check and the connection were two different lookups
validate_probe_urlresolved a hostname to check it, then urllib resolved it again to connect. An attacker controlling the name answers a public address for the check and a private one microseconds later.3.9.0 knew about this. The docstring said so:
That reasoning was defensible for a threat-model note and indefensible next to the skill's own permissions block, which claims address enforcement outright. A disclosure is supposed to close the gap between claim and behaviour, not document one and move on. ClawScan named exactly that: "the public site-audit path claims public-address enforcement while the code validates DNS before urllib reconnects."
Fixed by pinning.
_assert_public_hostreturns the addresses it approved, and everything that enforces the rule dials one of them:_PinnedHTTPConnection/_PinnedHTTPSConnection— connect to a validated address_ssl_notaftergoes throughconnect_public_tlsinstead of opening its own socket, which had the same double-resolutionTLS is unchanged: certificate validation and SNI still use the hostname (
server_hostname=self._tunnel_host or self.host), so pinning changes which endpoint is reached and nothing about how it is verified. There is a test asserting exactly that, because "pin the IP" is a plausible way to accidentally weaken cert checking.Proof
This sandbox has no DNS, so the machinery is proven offline against a local server:
(1) the rule fires at connect time, (2) the refusal comes from the rule and not from a broken opener — the same opener transports a real request end to end when the address is allowed, and (3) the socket's peer is the validated address rather than a fresh lookup.
2. Unbounded response bodies
site_audit._getread both the success and the HTTP-error body with a bareread(), so any server it visits decided how much memory this process uses — and the audit is the one feature pointed at URLs the operator does not control. Both paths are capped atMAX_BODY_BYTES(5 MB). The audit parses the document head — title, meta, generator, h1 — so truncating a giant page costs nothing.Scanner verdicts, for the record
The three LLM scanners all still report
suspiciouson 3.9.2; the green "Pass" badge in the ClawHub UI is not those verdicts — what changed there wasvirustotal: null → clean. Worth stating plainly because I got it wrong in conversation first.ClawScan dismissed five of its own matches as
unexpected, two of them on this skill's security comments (the169.254.169.254in the SSRF docstring, the phrase "never warn" in the localhost one). Its remainingnoteitems are capability descriptions — live-site writes with real credentials — which is what the skill is for.Tests: 96 → 105.
🤖 Generated with Claude Code