fix(openapi_plugin): block Azure WireServer and IPv6-embedded private addresses in server URL validation - #14267
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Just verifying this is still in the review queue. All checks green, branch is mergeable. Happy to address any feedback. |
cf75934 to
6b77c57
Compare
`IPv6Address.sixtofour` and `.teredo` already do this, and they return None outside their own prefixes, so they carry the membership test with them. That drops two network constants, one offset tuple and the hand-written XOR for the Teredo client address, which is the piece most likely to be restated wrongly. Checked against the previous hand-rolled version on 40000 random addresses per form: no disagreement. The approach is taken from microsoft#14267, which covers the same ground.
|
Heads up on one thing here, because I hit exactly the same bug in #14242 and only found it by measuring.
The tempting fix, only decoding when the address really is The reason none of it works is RFC 6052 section 3.3: the prefix length is configuration held by the translator and is not carried in the address, so there is nothing to infer it from. What I ended up with, if it is useful to you: keep the decode for Separately, thank you for There is real overlap between this and #14242, which I opened on 29 July. I am not asking you to close anything, that is for the maintainers to sort out, but the NAT64 detail is worth fixing in whichever one moves. |
|
Thanks for the careful read, you're right, and I've applied exactly the approach you suggested.
Added regression tests for all of it: the local-use range blocks (including And thanks for the pointer on #14242 - happy to defer to the maintainers on how to handle the overlap; this is the behavior I'd keep in whichever PR moves forward. |
|
That is the right split, and putting the RFC reasoning in the comment rather than only in the PR body is what will stop someone "simplifying" the two branches back into one later. Measured the behaviour at Two things worth having on the record. The prefixes are disjoint, so neither check can shadow the other and the order of the two branches is not load-bearing. That is worth knowing because it is the kind of thing a later refactor might reorder without thinking, and here it is safe. More importantly, and this is the one I would put in a comment if it is not already: the decoded WireServer address is not caught by any generic rule. So the explicit WireServer entry is not redundant with the private-range check, and anyone reviewing this who assumes "the private-address branch already covers Azure metadata" would be wrong. 169.254.169.254 decodes to link-local and would be caught either way; 168.63.129.16 would sail through. That asymmetry is the whole reason the WireServer rule has to exist separately, and it is invisible from reading the diff.
No further concerns from me on the NAT64 half. |
|
Thanks for measuring it at the head commit - that table is exactly the evidence this kind of change needs. Both points are now on the record in the code: the WireServer comment block spells out that Comment-only commit: 96d71d7 (+6 lines). |
|
That comment is better than what I asked for. Naming the wrong assumption outright, "do not remove it on the assumption that Nothing further from me on the NAT64 or WireServer half. |
|
Thanks for the close read-through - glad the comment lands the way you wanted. I will leave the PR as is then, and watch for the workflow-approval gate on your side. Happy to rebase if main moves before then. |
96d71d7 to
eb61cee
Compare
|
Taking you up on the rebase offer: main has moved since we last spoke. GitHub now On the approval gate, that is not something I can move. I have no write access The offer that is worth something from my side: if you rebase and the workflow |
eb61cee to
e2be1e1
Compare
|
Rebased onto latest main (e2be1e1). Ready for the test matrix run. Thank you! |
19e16ca to
e2be8ec
Compare
ErenAta16 (ErenAta16)
left a comment
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Taking up the offer I made: CI still has not run here, so here are the numbers
from running it locally instead.
First, confirming why you are still waiting. On e2be8eca three GitHub Actions
suites are action_required with zero runs each, and the only check that
executed is the Microsoft policy bot. That is the first-time-contributor workflow
gate, not a failure, and nothing you push will clear it. The branch has also gone
behind again since 8 September.
I ran a wrapper matrix against validate_server_url on your head, four addresses
by nine spellings, in both policy modes, with DNS stubbed out so nothing left the
machine.
Default mode is clean. All 36 combinations are refused, including every
obfuscated spelling of every address. Nothing to do there.
allow_private_network_access=True is where the interesting rows are, and the
plain WireServer case is exactly what you set out to fix:
168.63.129.16 plain blocked <- your change, working
168.63.129.16 ipv4-mapped v6 blocked <- your change, working
168.63.129.16 6to4 blocked <- your change, working
168.63.129.16 decimal ALLOWED
168.63.129.16 hex ALLOWED
168.63.129.16 octal ALLOWED
168.63.129.16 trailing dot ALLOWED
The four that get through do so before any of your logic runs.
_reject_cloud_metadata_host starts with ipaddress.ip_address(host) and returns
early on ValueError, and ip_address("2820128016") is a ValueError, as are the
hex, octal and trailing-dot forms. So the spelling never reaches
try_categorize_non_public_address to be categorised at all.
Whether that is reachable in practice depends on the client that later opens the
URL. On this Windows box getaddrinfo("2820128016") raises gaierror, so the
request would fail anyway; glibc's resolver generally accepts decimal, octal and
hex through inet_aton, so I would expect Linux to behave differently. I have not
tested that, and it is the part I would want a maintainer to decide on rather than
me.
The larger one is a gap between a comment and its implementation, and it is
older than your PR rather than caused by it. The call site says:
# Allowing access to a private network is not the same as allowing access to
# the host agent's cloud metadata endpoint, which is always blocked.
_reject_cloud_metadata_host(parsed_url)but the function blocks one category only:
if blocked and category == _AZURE_WIRE_SERVER_CATEGORY:Categorising the usual suspects shows what that leaves out:
168.63.129.16 blocked=True category="Azure metadata (WireServer)" <- rejected here
169.254.169.254 blocked=True category="link-local" <- not rejected
100.100.100.200 blocked=True category="carrier-grade NAT" <- not rejected
169.254.169.254 is the IMDS address for AWS, GCP and Azure alike, and in this
mode it is allowed in all nine spellings including the plain one. It is correctly
categorised as non-public; it simply is not the category the gate compares
against. So the comment's "always blocked" is true of WireServer and not of the
endpoint most SSRF write-ups mean.
None of this makes your PR worse than what it replaces, and the default posture
is unaffected. It does mean the comment now promises more than the code delivers,
so either the gate should widen to every category
try_categorize_non_public_address marks as metadata, or the comment should say
WireServer specifically.
I have kept this on the thread because the default configuration is not affected,
this path is opt-in and experimental, and the PR is already a public discussion of
these exact addresses. If the maintainers read it differently, MSRC is the right
channel and I am happy for it to move there.
… addresses in server URL validation The server URL validator could be bypassed to reach cloud metadata endpoints: the Azure WireServer IP 168.63.129.16 is publicly routable so it passed the private-address checks, and IPv6 addresses embedding an IPv4 (NAT64, 6to4, Teredo) were classified purely as IPv6, letting link-local and other private IPv4 addresses through. - Denylist the Azure WireServer metadata endpoint in _try_classify_ipv4 and enforce it even when allow_private_network_access is enabled. - Decode IPv4 addresses embedded in IPv6 (6to4 via sixtofour, Teredo via teredo, NAT64 64:ff9b::/96 and 64:ff9b:1::/48) before classification so embedded private addresses are blocked. - Add tests covering the WireServer endpoint, all three IPv6 embedding forms, and the private-network-access override.
…k mode
The call site documents that the host agent's cloud metadata endpoint "is
always blocked", but _reject_cloud_metadata_host only rejected the WireServer
category, so every other metadata endpoint passed when
allow_private_network_access=True:
168.63.129.16 Azure metadata (WireServer) rejected
169.254.169.254 link-local allowed
169.254.170.2 link-local allowed
100.100.100.200 carrier-grade NAT allowed
fd00:ec2::254 private (IPv6 ULA) allowed
169.254.169.254 is the IMDS address shared by AWS, GCP and Azure, so the gap
covered the endpoint most SSRF write-ups mean.
The category is not a usable signal here: "link-local", "carrier-grade NAT"
and "private (IPv6 ULA)" also cover ordinary private addresses that the
option exists to permit, and widening the gate to those categories would
remove the feature. Match the named metadata addresses instead, resolving
IPv4-mapped and NAT64-embedded IPv6 forms to the address they carry so
::ffff:169.254.169.254 and 64:ff9b::169.254.169.254 are caught as well.
Ordinary private addresses are unaffected: 10.0.0.5, 172.16.0.1, 192.168.1.50
and 100.64.0.1 (same category as the Alibaba Cloud address) still pass.
Adds 10 tests. Verified: 72 passed with this change; 6 failed, 66 passed
without it, the failures being exactly the six metadata cases above.
AI-assisted change, reviewed against the test output above.
e2be8ec to
0c24fba
Compare
|
Rebased onto While rebasing I went back through your metadata finding and acted on it, because you were right that the comment promised more than the code delivered.
So I did not widen the gate to every category. Those categories also cover ordinary private addresses the option exists to permit — Controls, so the change is pinned to addresses rather than to categories:
One judgement call that is worth your eye rather than mine: The change is AI-assisted; the output above is from the run, not a description of one. |
Description
The OpenAPI plugin server URL validator could be bypassed to reach cloud metadata endpoints in two ways:
168.63.129.16) is publicly routable. Unlike the AWS (169.254.169.254) and GCP (metadata.google.internal) endpoints, the Azure WireServer IP falls in no private range, sotry_categorize_non_public_addresstreated it as public andvalidate_server_urlallowed it.64:ff9b::/96,64:ff9b:1::/48), 6to4 (2002::/16), or Teredo (2001::/32) address looked like a benign public IPv6 address and passed validation.Changes
152071fe— WireServer and embedded IPv4_try_classify_ipv4, and enforce it even whenallow_private_network_access=True— being allowed to reach your own network is not the same as being allowed to reach the host agent's credential endpoint. The private-mode check only inspects literal IP hosts (private mode deliberately does not resolve hostnames).sixtofour,teredo, and the RFC 6052 NAT64 prefixes) intry_categorize_non_public_addressso the embedded address is classified with the IPv4 rules.0c24fba5— the remaining metadata endpointsThe private-mode gate documents that the host agent's cloud metadata endpoint "is always blocked", but it compared the category against
_AZURE_WIRE_SERVER_CATEGORYonly, so every other metadata endpoint passed:168.63.129.16169.254.169.254169.254.170.2100.100.100.200fd00:ec2::254169.254.169.254is the IMDS address AWS, GCP and Azure share — the endpoint most SSRF write-ups mean — andfd00:ec2::254is its documented IPv6 form.The category is not a usable signal here. "link-local", "carrier-grade NAT" and "private (IPv6 ULA)" also cover ordinary private addresses that the option exists to permit, so widening the gate to those categories would remove the feature rather than narrow it. The named addresses are matched instead, with IPv4-mapped and NAT64-embedded IPv6 forms resolved to the address they carry first, so
::ffff:169.254.169.254and64:ff9b::169.254.169.254are caught too.Ordinary private addresses are unaffected:
Test plan
pytest tests/unit/connectors/openapi_plugin/test_server_url_validator.py— 72 passed.The 10 tests added in
0c24fba5fail on152071fe:Fixes #14240
AI-assisted change, built and tested locally; the output above is from actual runs.