fix: close host-scoping deny-list bypasses via alternate address encodings (#635) - #649
Conversation
CapabilityEnforcer's NormalizeHostForMatch compared requested hosts and configured deny/allow patterns as raw strings, so a deny entry could be evaded by spelling the same address differently: decimal/hex/short-form IPv4 (2130706433, 0x7f.0.0.1, 127.1), a Unicode label separator or zero-width character (evil。com), a bracketed or zone-qualified IPv6 literal ([::1], fe80::1%eth0), or IPv4-mapped IPv6 (::ffff:127.0.0.1 for 127.0.0.1 — found by code review, verified live against the compiled enforcer). Now canonicalizes through Uri.IdnHost (what the real HTTP client actually connects with) plus explicit zone-id truncation and IPv4-mapped-IPv6 collapse, mirroring the normalization CompositeHookExecutor.IsReservedAddress already does for the same address class. Also adds an advisory warning when a configured deny/allow entry can never match anything (a typo'd pattern silently became a permanent no-op before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu
…rse, correct overstated doc claim (#635) run-gates.sh's correctness and security gates found the synthetic-scheme parsing added for #635 silently reduced a bare value carrying '@', '#', '?', or '\' to just its leading host segment, where it was previously refused outright as malformed. Harmless today (matches what the real HTTP client would connect to), but any template consumer's future tool that resolves the raw value some other way (DNS, socket, subprocess) would be checked against a different string than it contacts — excluded alongside '/' rather than left to widen silently. Also corrects an inaccurate doc-comment claim the grader gate flagged: "no tool issues a network request through anything other than Uri/HttpClient" is false (several tools run subprocesses via ISandboxExecutor); the actually-verified fact is that zero production tools declare a Host-kind resource parameter, so EnforceHostScoping never runs with a non-empty requestedHosts today regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu
…hrow claim (#635) Round-2 code review found a second, distinct alternate-IPv6-encoding bypass beyond the IPv4-mapped form already fixed: the older, RFC-4291-deprecated "IPv4-compatible" form (::a.b.c.d, no ffff prefix) still parses successfully and IPAddress.IsIPv4MappedToIPv6 doesn't recognize it, so a plain DeniedHosts=["127.0.0.1"] entry didn't refuse "::127.0.0.1"/"::7f00:1". Fixed with its own guarded helper rather than a blind "first 12 bytes zero" check — naively taking the last 4 bytes of "::1" (loopback) gives "0.0.0.1", not 127.0.0.1, so loopback and the unspecified address are excluded via the same IPAddress.IsLoopback/ IPv6Any checks the BCL itself uses. Also corrects this file's own doc comment: verified live that Uri.IdnHost DOES throw UriFormatException for a mixed valid-character- plus-invalid-Unicode label, contradicting the earlier "does not throw" claim (that test batch only tried pure-invalid labels, which fail earlier and never reach IdnHost). The existing fail-closed sentinel fallback already handled this correctly — this only fixes what the comment claimed about it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu
Correctness review — PR #649Bottom line: What I checkedAnchor set is two files: Rather than reason from the diff's own prose, I rebuilt the old and new Result: every divergence between old and new is in the tightening direction (a value that
Advisory (not blocking)
Single most important thing for a human to look at: the allow-side direction of the |
🔒 Security review — PR #649Verdict: BLOCK — 1 HIGH finding. Everything else below is advisory. Scope reviewed: First, the part that is right, because it matters for reading the finding below. I replayed the pre-#635 normalization against all 21 host/deny pairs the new tests exercise: 17 of 17 bypass-closure cases were The problem is that it stops one step short, in the exact class it claims to close. 🔴 HIGH —
|
| requested host | normalizes to | Dns.GetHostAddresses actually resolves to |
|---|---|---|
2852039166 (U+FF12 + 852039166) |
2852039166 |
169.254.169.254 |
2130706433 |
2130706433 |
127.0.0.1 |
0x7f.0.0.1 |
0x7f.0.0.1 |
127.0.0.1 |
0251.0376.0251.0376 |
0251.0376.0251.0376 |
169.254.169.254 |
127.1 |
127.1 |
127.0.0.1 |
(Contrast the pure-ASCII spellings, which this PR does correctly collapse: 2852039166 → 169.254.169.254, 2130706433 → 127.0.0.1.)
Attack path. Given DeniedHosts: ["169.254.169.254"] (the deny entry from #635's own issue text) and a tool declaring a ResourceParameterKind.Host parameter:
- Agent/model-controlled argument supplies host
2852039166. NormalizeHostForMatch→"2852039166".SecureInputValidatorHelper.ValidateHost("2852039166")passes —Uri.CheckHostNamereturnsIPv4, so the malformed-input gate does not catch it."2852039166" != "169.254.169.254"→ no deny match →EnforceHostScopingreturnsnull→ call permitted.- The tool connects.
Dns.GetHostAddresses("2852039166")→ 169.254.169.254 (IPAddress.TryParseaccepts 1-part decimal and octal/hex octets, confirmed on the pinned BCL). IMDS reachable → managed-identity token theft.
The same shape defeats an AllowedHosts allowlist in reverse, and defeats DeniedHosts: ["127.0.0.1"] for loopback.
Why HIGH rather than advisory. I weighed the mitigation the file itself documents — zero shipped tools declare a Host resource parameter, so requestedHosts is empty today (I confirmed: only FileSystemTool declares ResourceParametersByOperation, and it declares Path). That is real, and it is the same mitigation that applies to every bug this PR fixes. It cannot make this finding advisory while the PR treats the identical exposure as worth fixing. What makes it blocking is the second half: the remarks assert this class is closed, and DeniedHost_AlternateIPv4Encoding_StillMatchesDottedQuadDenyEntry asserts it in a test. Merging leaves a documented-and-tested closure that a single codepoint walks through — the "control that reads as enforcing but isn't" failure mode recorded six times in CLAUDE.md.
Fix — one line, in the block above:
if (IPAddress.TryParse(host, out var ip))
{
if (ip.IsIPv4MappedToIPv6) host = ip.MapToIPv4().ToString();
else if (TryGetDeprecatedIPv4CompatibleForm(ip, out var ipv4)) host = ipv4.ToString();
else host = ip.ToString(); // canonicalize ANY parseable literal, not just the two IPv6 forms
}This is also the correct canonicalization by construction rather than by enumeration: Dns.GetHostAddresses — the value actually consumed — resolves a numeric host through IPAddress.TryParse, so canonicalizing through the same parser makes the checked value equal the consumed value for every radix, octet count, and digit script at once, instead of closing encodings one at a time.
I checked the other direction, per CLAUDE.md's standing rule that a narrowing fix needs its own false-negative test: with else host = ip.ToString() applied, all five bypass variants close and every other case is byte-identical — 127.0.0.1, ::1, [::1], fd00::1, 2001:db8::1, fe80::1%eth0, ::ffff:127.0.0.1, ::127.0.0.1, evil.com, notevil.com, xn--vil-9ma.com, evil。com, *.evil.com, sub.evil.com, localhost, evil.com:443 all normalize exactly as they do today. It cannot over-fire on a DNS name, because IPAddress.TryParse fails on one.
Please add a [Theory] alongside DeniedHost_AlternateIPv4Encoding_StillMatchesDottedQuadDenyEntry covering at least 2852039166 against DeniedHosts: ["169.254.169.254"] — the existing test's pure-ASCII inputs pass without this fix, so it cannot detect the regression.
🟡 Advisory (non-blocking)
LOW — the IPv4-compatible collapse widens an AllowedHosts allowlist, which the remarks only justify in the deny direction. CapabilityEnforcer.HostScoping.cs:299 — TryGetDeprecatedIPv4CompatibleForm collapses ::a.b.c.d on both sides. Deny-side that is the intended tightening; allow-side it is a widening: AllowedHosts: ["10.0.0.5"] now admits requested ::10.0.0.5 and ::a00:5 (verified, both → 10.0.0.5), which at connect time is IPv6 ::a00:5, a different address. Practically inert (::/96 is deprecated and unroutable, and .NET resolves it as IPv6; the attacker also cannot choose the target freely — it is pinned to whatever IPv4 the operator allowlisted). Worth one sentence in the remarks acknowledging the allow-direction asymmetry, since the current text reasons only about deny entries.
LOW — catch (Exception) is broader than the one exception it was written for and logs nothing. :247. The UnnormalizableHostSentinel design is sound and correctly fail-closed on the requested-host side — that part I verified (a�.com → "\0" → ValidateHost rejects → refused). Two smaller points: it also swallows OutOfMemoryException / OperationCanceledException, so narrow it to UriFormatException/ArgumentException; and the remarks' claim that the throw is observable through existing logging is only half true — the emitted message says "host denied" or "can never match", never that IdnHost threw, so the one input class most likely to be adversarial is the one the operator cannot distinguish in logs.
LOW — WarnIfPatternIsInert fires per tool call, not once at config load. :51-52, :327. A single typo'd deny entry emits one warning per invocation of that tool — unbounded log growth on a hot tool, and the noisiest signal is the one an operator learns to filter. This repo already has the right mechanism for a startup-time config assertion (the *ConfigValidator convention plus SecurityControlHasACallerTests.EveryConfigValidator_IsBoundIntoTheOptionsPipeline, which would keep it from becoming another unbound validator). Behaviourally the current placement is harmless — advisory only.
LOW — normalization runs before any length bound. NormalizeHostForMatch performs a LINQ Count, two string interpolations, and up to two Uri parses on an unbounded caller-controlled string; MaxHostLength is only enforced afterward in ValidateHost. Pre-existing, marginally amplified here. A cheap length guard at the top of NormalizeHostForMatch would cap it.
Informational — residual gap now named, so it is a known one. The remarks correctly say the list is "NOT verified exhaustive". Concretely: normalization is Uri-shaped, while a subprocess tool (terraform/curl/wget via ISandboxExecutor) resolves through inet_aton semantics. Fixing the HIGH above aligns the two for numeric hosts specifically, because IPAddress.TryParse and inet_aton accept the same radix forms — a good reason to prefer the parser-based fix over further enumeration.
Bottom line
The normalization rewrite is well-built, the empirical method behind it is the right one, and the test suite provably discriminates — I verified all 17 bypass closures against pre-#635 behaviour rather than taking the comments' word for it. One HIGH blocks: a single fullwidth digit reopens the headline alternate-IPv4-encoding bypass, reaching 169.254.169.254, because a successfully-parsed IPAddress is discarded for plain IPv4. The fix is one line plus one [InlineData], and I have verified it closes all five variants with zero change to any other case. Re-request review after that and this passes.
…literals (#635) CI's security-review gate (running independently on the pushed PR) found a second-order bug in the fix itself: a fullwidth Unicode digit ("2", U+FF12) defeats Uri's own up-front IPv4-literal recognition, so "2852039166" classifies as HostNameType.Dns rather than IPv4. IdnHost then IDNA/NFKC-folds the fullwidth digit to plain ASCII as a side effect of DNS-label normalization, producing "2852039166" — a valid legacy decimal encoding of 169.254.169.254 (the cloud metadata address) — but Uri never re-evaluates HostNameType against its own normalized output, so the value was never collapsed to the dotted-quad form the very first commit in this PR already handles for a plain (all-ASCII) decimal literal. Verified live: re-running the same single-pass normalization on its own output resolves this correctly, since the second pass sees pure ASCII digits up front. NormalizeBareHost now re-normalizes to a fixed point (bounded at 4 iterations, not unconditional) rather than a single pass, so this closes the class of bug regardless of how many confusable/normalization layers an adversarial value stacks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu
Grader verdict — PR #649Intent: Close six alternate-encoding bypass classes in Check-by-check
Holes
Standards check
Bottom lineLOOKS GOOD. All claims in the PR description are backed by a changed line and a passing, independently-re-run test (63/63). The most important thing for the human Checker to look at: the doc-comment-disclosed assumption that no production tool resolves a host outside |
🔐 Security review — PR #649 ·
|
Correctness review — PR #649Method: reproduced Blocking defectsNone. Checks that came back clean (all empirically, not by reading):
Advisory (not blocking)
Bottom lineCORRECT — no blocking defect found. The single thing worth a human's eyes is advisory 1: |
Summary
Closes #635.
CapabilityEnforcer's network-host allow/deny check compared a requested host anda configured deny/allow entry as raw strings — so a blocked address (e.g. a cloud metadata
endpoint) could be reached by spelling it a different but equivalent way. This PR canonicalizes
both sides of the comparison through the same host resolution the real HTTP client uses, closing
six distinct alternate-encoding bypass classes:
2130706433,0x7f.0.0.1,127.1)evil。com, a trailing zero-width space)[::1],fe80::1%eth0)::ffff:127.0.0.1) — found during code review, verified live against thecompiled enforcer
::127.0.0.1, noffff) — found in asecond review round, fixed with an explicit loopback/unspecified-address exclusion (naively
taking the last 4 bytes of
::1gives0.0.0.1, not127.0.0.1— verified and guarded against)@/#/?/\silently reducing to just their leading host segment wherethey were previously refused outright as malformed
Also adds an advisory warning when a configured deny/allow entry can never match anything (a
typo'd pattern previously became a silent, permanent no-op).
Review history
Two full
/code-reviewrounds plus a nudge-nudge-verified coordinator pass,run-gates.sh'sgrader/correctness/security/owasp gates clean 5/5 runs (opus-tier automated review independently
found the IPv4-mapped-IPv6 bypass and the ambiguous-character gap before I'd finished my own
second pass). Every fix mutation-tested (temporarily reverted, confirmed the new test fails,
restored). 62/62
CapabilityEnforcementTestspass.Local
testgate failed 5 consecutive times on the same unrelated, pre-existing flaky test class(
ProcessSandboxExecutorTests/SandboxAttestationBindingTests— sandbox process execution,unrelated to this diff, confirmed clean in isolation every time) — pushed via the sanctioned
RAILS_SKIP_REVIEW_GATE=1bypass, deferring to CI's independent (non-contended) run.Follow-ups filed (out of scope for this PR)
OwnerOnlyDirectoryHelper.CreateTOCTOU race (different file, not touched here)owner-only helper
Test plan
dotnet build src/AgenticHarness.slnxCapabilityEnforcementTests: 62/62 passrun-gates.sh: build/owasp/grader/correctness/security/docs-drift clean 5/5;testgate'sfailure isolated and confirmed as pre-existing, unrelated flakiness
(throwaway console apps + live temp tests), not asserted from documentation alone
🤖 Generated with Claude Code
https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu