diff --git a/internal/atproto/oauth/transport.go b/internal/atproto/oauth/transport.go index 21ab957..09af26d 100644 --- a/internal/atproto/oauth/transport.go +++ b/internal/atproto/oauth/transport.go @@ -26,43 +26,282 @@ func (t *ssrfSafeTransport) resolveHost(host string) ([]net.IP, error) { return net.LookupIP(host) } -// isPrivateIP checks if an IP is in a private/reserved range +// reservedNetworks are the ranges, in BOTH families, that no stdlib predicate +// names. +// +// Parsed ONCE, at package scope, because isPrivateIP runs on the per-request hot +// path — every resolved address of every outbound call walks this list. +// +// Each entry is a destination a caller-supplied URL has no business naming: +// +// - 0.0.0.0/8, "this network". The honest reason is NOT reachability: the rest +// of the /8 is not independently routable, and `ip route get 0.0.0.5` takes +// the default route rather than lo. It is that ::1 read as an +// IPv4-compatible address decodes to 0.0.0.1, so blocking the /8 makes that +// payload private on its own merits and removes isPrivateIP's dependence on +// testing loopback BEFORE it decodes a payload. An ordering invariant that +// nothing enforces is one a refactor deletes silently. +// - 100.64.0.0/10 is carrier-grade NAT, and in practice the operator's own +// mesh — Tailscale hands out addresses from this block. +// - 192.0.0.0/24 is reserved for protocol machinery (DS-Lite's 192.0.0.0/29 +// among it), never a destination a caller legitimately asks for. +// - 198.18.0.0/15 is the benchmarking range, routed internally where it is +// routed at all. +// - 240.0.0.0/4 is former class E, and it carries 255.255.255.255 with it: +// the all-hosts broadcast, which the stack handles unlike a unicast +// destination. +// - 2002::/16 is 6to4, banned outright rather than decoded. Its embedded IPv4 +// names the tunnel's gateway, not where the packet ends up — see +// embeddedIPv4, which explains why this one prefix is the exception to +// everything that file does. +// - fec0::/10 is IPv6 site-local: deprecated by RFC 3879 and superseded by +// fc00::/7, but a stack that still recognises it routes it as an internal +// network. It falls outside BOTH predicates that look like they cover it, +// since IsPrivate is fc00::/7, IsLinkLocalUnicast is fe80::/10, and +// fec0::/10's bit pattern is disjoint from each. +// +// NEVER add ::ffff:0:0/96 to this list to cover SIIT. It is not the SIIT prefix, +// and net.ParseCIDR degenerates it to 0.0.0.0/0 — the full reasoning is on the +// SIIT branch in embeddedIPv4, which is where that prefix is handled instead. +var reservedNetworks = []*net.IPNet{ + mustParseCIDR("0.0.0.0/8"), + mustParseCIDR("100.64.0.0/10"), + mustParseCIDR("192.0.0.0/24"), + mustParseCIDR("198.18.0.0/15"), + mustParseCIDR("240.0.0.0/4"), + mustParseCIDR("2002::/16"), + mustParseCIDR("fec0::/10"), +} + +// mustParseCIDR panics on a malformed prefix. Its arguments are compile-time +// constants in this file, so a failure is a typo caught at startup rather than a +// range that silently stops being checked. +func mustParseCIDR(cidr string) *net.IPNet { + _, network, err := net.ParseCIDR(cidr) + if err != nil { + panic(fmt.Sprintf("oauth: malformed reserved CIDR %q: %v", cidr, err)) + } + return network +} + +// isPrivateIP reports whether an address reaches this host, the operator's own +// network, or something the kernel treats specially. +// +// THE DEFAULT IS THE DANGEROUS DIRECTION. Anything this predicate does not +// recognise is treated as public and dialled, and the address space holds far +// more reserved territory than RFC1918. Every host that reaches this transport +// was chosen by a stranger — a DID document's PDS endpoint, an acceptance +// record's subject — so the attacker picks from the whole space, not from the +// part we happened to remember. func isPrivateIP(ip net.IP) bool { if ip == nil { return false } - // Check for loopback + // The unspecified address is loopback wearing a different number. 0.0.0.0 + // (and :: , and ::ffff:0.0.0.0) is a wildcard in bind() only; in connect() + // the kernel substitutes the local host, so http://0.0.0.0:5432/ reaches + // whatever is listening on 127.0.0.1:5432. + if ip.IsUnspecified() { + return true + } + if ip.IsLoopback() { return true } - // Check for link-local + // Link-local reaches the local segment without routing, and 169.254.169.254 + // is the cloud instance metadata service. if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { return true } - // Check for private ranges - privateRanges := []string{ - "10.0.0.0/8", - "172.16.0.0/12", - "192.168.0.0/16", - "169.254.0.0/16", - "::1/128", - "fc00::/7", - "fe80::/10", + // A group address touches every listener on the segment without naming one. + // This has to be the FAMILY-AGNOSTIC predicate rather than a 224.0.0.0/4 + // entry in the list below, which would cover IPv4 only: the scopes it would + // leave open are ff05:: (site-local) and ff0e:: (global). NOT ff02::, which + // IsLinkLocalMulticast already catches one check above — mutation testing + // confirmed the site-local and global scopes are the only two that + // discriminate here. + // + // Which is why IsLinkLocalMulticast must stay even though this line now makes + // it redundant FOR IPv6: for IPv4 it is not redundant at all, and it is the + // only cover for 224.0.0.x if IsMulticast were ever narrowed. + if ip.IsMulticast() { + return true + } + + // RFC1918 and IPv6 unique-local, via the stdlib so the masks cannot drift. + if ip.IsPrivate() { + return true } - for _, cidr := range privateRanges { - _, network, err := net.ParseCIDR(cidr) - if err == nil && network.Contains(ip) { + for _, network := range reservedNetworks { + if network.Contains(ip) { return true } } + // Last, because everything above answers "which block is this address in" and + // some IPv6 forms defeat that question rather than answering it wrongly: the + // destination is a four-byte field carried INSIDE the address. + // + // Position is no longer load-bearing, and that is deliberate. ::1 read as an + // IPv4-compatible address decodes to 0.0.0.1, so running the decode first + // used to reclassify IPv6 loopback on a payload nothing blocked; 0.0.0.0/8 in + // the list above now blocks that payload too, and the reorder is safe. + if embedded := embeddedIPv4(ip); embedded != nil { + return isPrivateIP(embedded) + } + return false } +// embeddedIPv4 returns the IPv4 address an IPv6 address is carrying, or nil. +// +// # WHY THESE PAYLOADS ARE DECODED RATHER THAN THEIR PREFIXES BANNED +// +// 64:ff9b::7f00:1 and 64:ff9b::808:808 differ only in the four bytes the prefix +// carries — one is 127.0.0.1 and the other is 8.8.8.8 — and no CIDR over the +// IPv6 space separates them. Banning the prefix to catch the first would be an +// outage, not a trade: 64:ff9b::/96 is a legitimate connect() destination, and +// an IPv6-only host with DNS64 reaches every IPv4-only server in the world +// through it. On such a deployment a wholesale ban does not block some outbound +// federation, it blocks all of it. +// +// # WHAT IS DECODED HERE +// +// - NAT64 well-known prefix, 64:ff9b::/96 (RFC 6052) — IPv4 in the last four +// bytes. Purpose-built to mean "this IPv4 host", so a translator on the path +// delivers it exactly there. +// - NAT64 local-use prefix, 64:ff9b:1::/48 (RFC 8215) — the same mechanism +// with an operator-chosen prefix, and the same consequence if banned. +// - SIIT IPv4-translated, ::ffff:0:0:0/96 (RFC 6052 §2.2) — IPv4 in the last +// four bytes, and likewise the destination itself. +// - IPv4-compatible IPv6, ::/96 — deprecated. It slips past every range check +// because of the standard library, not this package: To4 normalises ONLY the +// ::ffff: mapped form, so ::ffff:127.0.0.1 reaches the loopback test as a +// 4-byte 127.0.0.1 while ::7f00:1 reaches it as sixteen opaque bytes. +// +// # WHAT IS DELIBERATELY NOT DECODED +// +// This is not an exhaustive list of the encodings that embed IPv4, and it is not +// trying to be. Each exclusion has a precondition that does not hold here: +// +// - 6to4, 2002::/16 — BANNED WHOLESALE in reservedNetworks instead, and the +// asymmetry with NAT64 is the thing to remember about this function. Making +// the two symmetric is wrong in either direction. 6to4 embeds a GATEWAY: +// 2002:V4ADDR:SLA:iface names a tunnel endpoint in bytes 2..6 and then a +// subnet and a host BEHIND it, so a public payload says only who the tunnel +// belongs to and nothing about the far side, and decoding answers a question +// nobody asked. NAT64 embeds the DESTINATION, so its payload is precisely +// what to classify. 6to4 is banned; NAT64 is decoded. +// +// Do not shorten the justification to "RFC 7526 deprecated it", because that +// is false: RFC 7526 deprecates the ANYCAST RELAY prefix 192.88.99.0/24 and +// says of the rest, verbatim, that "the associated 6to4 IPv6 prefix +// 2002::/16 are not deprecated". The ban is our policy call. What makes it +// cheap is RFC 7526 §4 ("in host implementations, unicast 6to4 MUST also be +// disabled by default"), 2002::/16's place on the standard bogon lists, and +// 6to4 rounding to 0.00% of Google's measured IPv6 traffic. +// +// - Teredo — needs a Teredo tunnel on the host. If it is ever added the prefix +// is 2001:0000::/32 and NEVER 2001::/16, which is sixteen bits too wide: +// live atProto PDSes sit at 2001:19f0:7002:191:: and 2001:550:5a00:785b::1, +// and a /16 rule blocks both. They are pinned as allowed rows in the tests. +// +// - ISATAP — its 0000:5efe:V4ADDR interface identifier can appear under ANY +// unicast /64, so matching it means reading the low bytes of every IPv6 +// address rather than recognising a prefix, and it reaches nothing without +// an ISATAP interface configured on this host. +// +// - RFC 6052 network-specific prefixes — the operator chooses the prefix, it +// may be a /32, /40, /48, /56, /64 or /96, and the IPv4 sits at a different +// offset in each. There is no set of them to enumerate. +func embeddedIPv4(ip net.IP) net.IP { + // An IPv4 address carries no payload of its own — which is also what bounds + // isPrivateIP's recursion to a single step, since every return below is one. + // The ::ffff: mapped form lands here too, already normalised by To4. + if ip.To4() != nil { + return nil + } + + // nil for any length that is not an address, so the indexing below cannot + // panic on a malformed slice. A panic here would take down every outbound + // request, not just the odd one. + ip16 := ip.To16() + if ip16 == nil { + return nil + } + + // NAT64, well-known prefix: 64:ff9b:: followed by eight zero bytes. + if hasBytePrefix(ip16, 0x00, 0x64, 0xff, 0x9b) && isAllZero(ip16[4:12]) { + return net.IPv4(ip16[12], ip16[13], ip16[14], ip16[15]) + } + + // NAT64, local-use prefix: the same four bytes, then 0001, then zeroes. + // + // Only the /96-suffix shape is read. RFC 8215 hands the operator a /48 and + // RFC 6052 then puts the IPv4 at an offset that depends on the prefix length + // they actually deployed, so a 64:ff9b:1:: address with a non-zero middle is + // one whose payload offset this code cannot know — and guessing wrong would + // invent a destination rather than find one. + if hasBytePrefix(ip16, 0x00, 0x64, 0xff, 0x9b, 0x00, 0x01) && isAllZero(ip16[6:12]) { + return net.IPv4(ip16[12], ip16[13], ip16[14], ip16[15]) + } + + // SIIT IPv4-translated: eight zero bytes, ffff, two more zero bytes. + // + // A BYTE PATTERN AND NOT A CIDR ENTRY, deliberately, because the CIDR that + // looks right is a production outage. ::ffff:0:0/96 — one ":0" group short of + // the translated prefix, and the spelling anyone reaching for a range entry + // writes first — is the IPv4-MAPPED prefix, whose network number passes To4; + // net.IPNet.Contains then compares in 4-byte space against the last four + // bytes of a 16-byte /96 mask, which are zero. The result parses and prints + // as 0.0.0.0/0 and Contains(8.8.8.8) is true, so that single typo in + // reservedNetworks refuses every outbound request the AppView makes. + // + // The correctly spelled ::ffff:0:0:0/96 does not degenerate, and is still the + // wrong tool: a range entry bans the prefix, and the destination here has to + // be decoded and re-checked like NAT64's. + if isAllZero(ip16[:8]) && ip16[8] == 0xff && ip16[9] == 0xff && isAllZero(ip16[10:12]) { + return net.IPv4(ip16[12], ip16[13], ip16[14], ip16[15]) + } + + // IPv4-compatible: twelve zero bytes. Disjoint from SIIT above, which carries + // ffff where this form has zeroes. + if isAllZero(ip16[:12]) { + return net.IPv4(ip16[12], ip16[13], ip16[14], ip16[15]) + } + + return nil +} + +// hasBytePrefix reports whether b begins with the given bytes. The length test +// is what keeps the prefixes above from indexing past a short slice. +func hasBytePrefix(b []byte, prefix ...byte) bool { + if len(b) < len(prefix) { + return false + } + for i, want := range prefix { + if b[i] != want { + return false + } + } + return true +} + +// isAllZero reports whether every byte is zero, which is how the prefixes above +// are recognised without allocating a mask per call. +func isAllZero(b []byte) bool { + for _, v := range b { + if v != 0 { + return false + } + } + return true +} + // vettedAddrsKeyType keys the addresses RoundTrip approved, so the dialler can // read them off the request's own context. A private type, so nothing outside // this file can plant a value under the same key. diff --git a/internal/atproto/oauth/transport_embedded_ipv4_test.go b/internal/atproto/oauth/transport_embedded_ipv4_test.go new file mode 100644 index 0000000..67fe821 --- /dev/null +++ b/internal/atproto/oauth/transport_embedded_ipv4_test.go @@ -0,0 +1,263 @@ +package oauth + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestIsPrivateIP_IPv6FormsEmbeddingIPv4 pins the IPv6 addresses that carry an +// IPv4 address inside them. +// +// # WHY THIS IS A DIFFERENT MECHANISM, NOT MORE RANGES +// +// The reserved-range checks ask "which block is this address in". These forms +// defeat that question rather than answering it wrongly: the address the packet +// ends up at is not the address being classified, it is a four-byte field +// EMBEDDED in it. A classifier that never decodes the payload cannot tell a +// loopback from a public DNS server, because the two differ only in bytes no CIDR +// over the IPv6 space separates. +// +// # 6to4 IS THE EXCEPTION, AND THE REASON IS NOT "IT WAS TOO HARD" +// +// The rule for every other form here is decode the payload, classify the payload. +// 6to4 is banned wholesale instead, and `2002:808:808::` — 6to4 carrying the +// public 8.8.8.8 — is a BLOCKED row rather than an allowed one. This file +// asserted the opposite when it was first written. **The reversal is a +// requirements change, not a test relaxed to let an implementation pass**, and +// the argument is worth having in front of you before you change it back. +// +// 6to4's embedded IPv4 is not the destination. It is the TUNNEL ENDPOINT: +// `2002:V4ADDR:SLA:iface` names a gateway in bytes 2..6 and then a subnet and a +// host BEHIND that gateway in the remaining ten. So "the embedded v4 is public, +// therefore this address is safe" does not follow — a 6to4 address whose gateway +// is public can still name an internal IPv6 service on the far side of it, and +// decoding tells you only who the tunnel belongs to. The decode answers a +// different question from the one asked. +// +// The decision is to ban `2002::/16` wholesale and stop decoding 6to4 at all. +// The soundness argument above is the reason; what makes it cheap is that +// unicast 6to4 is effectively dead traffic. RFC 7526 §4 requires that "in host +// implementations, unicast 6to4 MUST also be disabled by default"; `2002::/16` +// sits on the standard bogon lists (Team Cymru, NLNOG); 6to4 and Teredo together +// round to 0.00% of Google's measured IPv6 traffic; and a `2002::`-only server is +// unreachable in practice anyway, because the return-relay ecosystem it depended +// on is gone. The operator has confirmed nothing in this infrastructure uses it. +// +// Be careful with the citation, because the obvious shorthand is wrong: RFC 7526 +// does NOT deprecate 6to4. It deprecates the ANYCAST RELAY prefix 192.88.99.0/24, +// and says of the rest, in terms — "The basic unicast 6to4 mechanism defined in +// [RFC3056] and the associated 6to4 IPv6 prefix 2002::/16 are not deprecated." +// The ban here is our policy choice, justified by the gateway-field argument and +// made cheap by the traffic numbers; it is not something an RFC did for us. +// +// # NAT64 KEEPS ITS DECODE, AND THAT IS NOT A NICETY +// +// The temptation for the next reader is to make NAT64 symmetric with the 6to4 +// ban. That would be an outage. `64:ff9b::/96` is a legitimate connect() +// destination: an IPv6-only host with DNS64 reaches every IPv4-only server in the +// world through it, so on an IPv6-only deployment a wholesale ban does not block +// some outbound federation, it blocks ALL of it. `64:ff9b:1::/48` (RFC 8215) is +// the same mechanism with a locally-chosen prefix and carries the same +// consequence. +// +// The asymmetry, stated plainly, is the thing to remember about this file: +// **6to4 embeds a gateway, so we ban it. NAT64 embeds the destination, so we +// decode it.** Same-shaped encoding, different semantics, different answer. +// +// # WHAT THE ALLOWED ROWS ARE FOR +// +// Every remaining allowed row embeds the public 8.8.8.8 under a prefix that is +// still decoded. They are what stops the 6to4 decision from being generalised +// into "ban every prefix": a wholesale ban of NAT64 or SIIT turns all their +// blocked rows green and these red. The blocked rows prove the extraction +// happens; the allowed rows prove it is a decode and not a ban. +// +// # DELIBERATE EXCLUSIONS +// +// Teredo is NOT handled here, and if anyone adds it, the prefix is +// `2001:0000::/32` — **never** `2001::/16`. That is not a style preference. The +// atProto ecosystem has live production PDSes at `2001:19f0:7002:191::` (socl.is) +// and `2001:550:5a00:785b::1` (pds.zzls.xyz), and a /16 rule blocks both: +// verified, `net.ParseCIDR("2001::/16")` contains each of them while +// `2001:0000::/32` contains neither. Both addresses are pinned as allowed rows +// below so the mistake fails a test rather than federation. +func TestIsPrivateIP_IPv6FormsEmbeddingIPv4(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ip string + blocked bool + }{ + // 6to4, 2002::/16 — banned wholesale, payload irrelevant. The public-payload + // row is the one that states the ban; without it, a decode-and-recheck + // implementation still passes the other three. + {"6to4 embedding loopback", "2002:7f00:1::", true}, + {"6to4 embedding RFC1918 10/8", "2002:a00:1::", true}, + {"6to4 embedding RFC1918 192.168/16", "2002:c0a8:1::", true}, + {"6to4 with a public tunnel endpoint", "2002:808:808::", true}, + + // The prefix ban must be exactly /16 and not wider. + {"Just above 6to4", "2003::1", false}, + + // NAT64 well-known prefix, 64:ff9b::/96 — the IPv4 is the last 4 bytes and + // it is the real destination, so this one is decoded. + {"NAT64 well-known embedding loopback", "64:ff9b::7f00:1", true}, + {"NAT64 well-known embedding RFC1918 10/8", "64:ff9b::a00:1", true}, + {"NAT64 well-known embedding a public address", "64:ff9b::808:808", false}, + + // NAT64 local-use prefix, 64:ff9b:1::/48 (RFC 8215). Same semantics as the + // well-known prefix and a separate range: an implementation matching only + // 64:ff9b::/96 lets every one of these through, and the metadata-service row + // is what that costs. + {"NAT64 local-use embedding loopback", "64:ff9b:1::7f00:1", true}, + {"NAT64 local-use embedding the metadata service", "64:ff9b:1::a9fe:a9fe", true}, + {"NAT64 local-use embedding a public address", "64:ff9b:1::808:808", false}, + + // SIIT IPv4-translated, ::ffff:0:0:0/96 — bytes 0-7 zero, 8-9 ffff, 10-11 + // zero, IPv4 in the last 4. Decoded, not banned: like NAT64, the embedded + // address is the destination. + // + // THIS ONE MUST BE A BYTE-PATTERN DECODE AND MUST NEVER BECOME A CIDR + // ENTRY. Two separate traps sit on top of each other here, and the second + // is a production outage: + // + // 1. It READS as already covered. ::ffff:0:127.0.0.1 is one group away + // from ::ffff:127.0.0.1, which is blocked — but they are different + // prefixes (mapped puts ffff at bytes 10-11, translated at bytes 8-9) + // and To4 normalises only the mapped one. + // + // 2. The near-miss spelling silently blocks the entire IPv4 internet. + // `::ffff:0:0/96` — one ":0" group short of the translated prefix, and + // the spelling anyone reaching for a CIDR entry is most likely to + // write — is not SIIT at all. It is ::ffff:0.0.0.0, the IPv4-MAPPED + // prefix, whose network number passes To4; net.IPNet.Contains then + // compares in 4-byte space using the last four bytes of the 16-byte + // /96 mask, and those are all zero. Verified with the stdlib: + // net.ParseCIDR("::ffff:0:0/96") yields a network that prints as + // 0.0.0.0/0 and whose Contains returns true for 8.8.8.8 and 1.1.1.1. + // Dropped into a reserved-range list, that single typo refuses every + // outbound request the AppView makes. + // + // (The correctly-spelled `::ffff:0:0:0/96` does NOT degenerate — it + // parses and prints as itself. It is still the wrong tool, because a + // range entry bans the prefix where the public-payload row below + // requires it to be decoded.) + {"SIIT translated embedding loopback", "::ffff:0:7f00:1", true}, + {"SIIT translated embedding RFC1918 10/8", "::ffff:0:a00:1", true}, + {"SIIT translated embedding a public address", "::ffff:0:808:808", false}, + + // IPv4-compatible IPv6, ::/96 — deprecated, the IPv4 is the last 4 bytes, + // and Go's To4 does NOT normalise this form (it normalises only ::ffff:), + // which is why it reaches the classifier as sixteen opaque bytes. + {"IPv4-compatible embedding loopback", "::7f00:1", true}, + {"IPv4-compatible embedding RFC1918 10/8", "::a00:1", true}, + + // ::1 IS BLOCKED TWICE OVER, AND IT USED TO BE BLOCKED ONCE. + // + // IsLoopback catches it, and that check runs BEFORE the embedded decode. + // The ordering used to be the only thing protecting it: read as an + // IPv4-compatible address, ::1's payload is 0.0.0.1, so a decode running + // first would have classified THAT instead — and 0.0.0.1 was public. + // Mutation testing found the exposure: moving the decode block to the top + // of isPrivateIP was killed by exactly one assertion in the whole package, + // the ::1 row in transport_test.go. "Unspecified IPv6" (::) did not cover + // it, because :: survives that reorder on its payload's own merits + // (0.0.0.0 is still unspecified). + // + // Blocking 0.0.0.0/8 removed the dependency rather than documenting it: + // the payload is now private in its own right, so the two checks agree and + // the order between them no longer decides the answer. This row stays as + // the statement of the invariant, in the file whose mechanism it concerns + // instead of only in a legacy file nobody knows is load-bearing. + {"IPv6 loopback via the IPv4-compatible decode path", "::1", true}, + + // Control: the mapped form, which To4 DOES normalise, so it reaches the + // loopback check as a 4-byte address and never needs the decode at all. + // Pinned because it is the half that always worked — changes to its + // siblings must not disturb it. + {"IPv4-mapped embedding loopback", "::ffff:127.0.0.1", true}, + + // THE TEREDO LANDMINE, pinned rather than merely described. + // + // Teredo (2001:0000::/32) also embeds IPv4 and is a plausible future + // addition to this file. The mistake to guard against is writing the + // prefix as 2001::/16, which is sixteen bits too wide and swallows a large + // slice of live production IPv6. These two addresses are real atProto + // PDSes — socl.is and pds.zzls.xyz — and a /16 rule blocks both while + // 2001:0000::/32 blocks neither. + // + // They pass today because nothing matches 2001::/anything. That is the + // point: they are a tripwire for a change nobody has made yet, and the + // cost of not having them is silently unreachable federation peers. + {"Live atProto PDS under 2001::/16", "2001:19f0:7002:191::", false}, + {"Second live atProto PDS under 2001::/16", "2001:550:5a00:785b::1", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tt.ip) + require.NotNil(t, ip, "the test's own input %q must parse as an IP", tt.ip) + + if tt.blocked { + assert.True(t, isPrivateIP(ip), + "isPrivateIP(%s) returned false: this IPv6 address reaches a local or internal destination "+ + "through a spelling the guard does not read — either it carries a private IPv4 payload, or "+ + "it is a tunnelling prefix whose far side the payload does not describe", tt.ip) + return + } + assert.False(t, isPrivateIP(ip), + "isPrivateIP(%s) returned true: this address is an ordinary public destination, so blocking it "+ + "means a prefix was banned wholesale where the embedded address should have been extracted "+ + "and re-checked", tt.ip) + }) + } +} + +// TestEmbeddedIPv4_DoesNotPanicOnMalformedAddresses pins the guard that +// embeddedIPv4's own comment says is load-bearing. +// +// The function indexes a 16-byte slice directly, and its length check is what +// stands between a malformed net.IP and an out-of-range panic. That panic would +// not be a wrong answer for one request — isPrivateIP runs on every resolved +// address of every outbound call, so it would take down every outbound request +// the process makes. Nothing exercised it before this test. +// +// A malformed slice is not hypothetical: net.IP is a byte slice with exported +// contents, callers construct them, and net.ParseIP is not the only way one +// arrives. The assertion is both halves — no panic, AND nil, because inventing a +// destination out of three arbitrary bytes would be its own bug. +func TestEmbeddedIPv4_DoesNotPanicOnMalformedAddresses(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ip net.IP + }{ + {"too short", net.IP{1, 2, 3}}, + {"too long", net.IP{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}}, + {"empty", net.IP{}}, + {"nil", nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.NotPanics(t, func() { + assert.Nil(t, embeddedIPv4(tt.ip), + "a %d-byte slice is not an address and must decode to nothing", len(tt.ip)) + }, "embeddedIPv4 panicked on a %d-byte slice; isPrivateIP runs on every resolved address of every "+ + "outbound request, so a panic here fails all of them", len(tt.ip)) + + assert.NotPanics(t, func() { + _ = isPrivateIP(tt.ip) + }, "isPrivateIP panicked on a %d-byte slice", len(tt.ip)) + }) + } +} diff --git a/internal/atproto/oauth/transport_reserved_ranges_test.go b/internal/atproto/oauth/transport_reserved_ranges_test.go new file mode 100644 index 0000000..63904de --- /dev/null +++ b/internal/atproto/oauth/transport_reserved_ranges_test.go @@ -0,0 +1,293 @@ +package oauth + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestIsPrivateIP_ReservedAndUnspecifiedRanges pins the reserved address classes +// beyond RFC1918 and loopback. +// +// # WHY AN ALLOWLIST-SHAPED PROBLEM IS BEING SOLVED WITH A DENYLIST +// +// The predicate is a denylist, so anything it does not recognise is "public" by +// default and gets dialled. That default is the dangerous direction: the address +// space holds far more reserved territory than RFC1918 and loopback, and every +// range below either reaches this host, reaches the operator's own network, or +// reaches something the kernel treats specially. A URL is chosen by a stranger — +// a DID document's PDS endpoint, an acceptance record's subject — so the attacker +// picks from the whole space, not from the part we remembered. Each class here +// was missing at some point, and the table is what stops the list regressing to +// the part that was obvious. +// +// The classes, and what each one actually reaches: +// +// - UNSPECIFIED (0.0.0.0, ::, ::ffff:0.0.0.0). A wildcard in bind(), not in +// connect(): the kernel substitutes the local host, so it is loopback wearing +// a different number. This is the class the acceptance test pins end-to-end. +// - CGNAT (100.64.0.0/10). Carrier-grade NAT, and the default subnet of a great +// deal of infrastructure — Tailscale hands out 100.64/10 addresses, so this +// range is frequently the operator's private mesh. +// - IETF PROTOCOL ASSIGNMENTS (192.0.0.0/24). Reserved for protocol machinery +// (DS-Lite's 192.0.0.0/29 among it); nothing here is a destination a caller +// legitimately names. +// - BENCHMARKING (198.18.0.0/15). Reserved for device test harnesses and, in +// practice, routed internally where it is used at all. +// - RESERVED (240.0.0.0/4) and BROADCAST (255.255.255.255). Former class E and +// the all-hosts broadcast; the stack handles both unlike a normal unicast +// destination. +// - MULTICAST, both families. A single packet addressed to a group, which is a +// way to touch hosts on the local segment without naming one. +// +// The multicast case carries a trap worth stating, because the obvious +// implementation walks into it: adding "224.0.0.0/4" to the CIDR list covers IPv4 +// ONLY, and leaves every IPv6 multicast scope open. That is why ff02::1 +// (link-local all-nodes), ff05::1 (site-local) and ff0e::1 (global) are each +// pinned separately rather than represented by one row — a fix that generalises +// over scope passes all three, and a CIDR-shaped fix does not. +// +// # THE ALLOWED ROWS, AND WHICH ERROR EACH ONE CATCHES +// +// They are not padding, but they do not all guard the same mistake, and the +// distinction is easy to get backwards because net.ParseCIDR normalises a prefix +// to its network address. +// +// - The "just BELOW" rows catch a range written one bit too wide. Widen +// 100.64.0.0/10 to /9 and it normalises to 100.0.0.0/9 — 100.0.0.0 through +// 100.127.255.255 — which swallows `100.63.255.255`. Same shape for +// 198.18.0.0/15 → 198.16.0.0/14, which swallows `198.17.255.255`. +// - The "just ABOVE" rows catch a different error: a wrong base address, or a +// widening of two bits or more. They do NOT catch the one-bit case, because +// normalising to the network address extends a range downward rather than +// upward — 100.0.0.0/9 stops below `100.128.0.0`, and 198.16.0.0/14 stops +// below `198.20.0.0`. +// +// One row is neither: `9.255.255.255` sits below 10/8, which was covered long +// before this table existed. It is a plain regression guard on the pre-existing +// RFC1918 boundary, kept because the table is where someone will look. +func TestIsPrivateIP_ReservedAndUnspecifiedRanges(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ip string + blocked bool + }{ + // Unspecified — the local host by another name, in all three spellings. + {"Unspecified IPv4", "0.0.0.0", true}, + {"Unspecified IPv6", "::", true}, + {"Unspecified IPv4-mapped IPv6", "::ffff:0.0.0.0", true}, + + // The REST of 0.0.0.0/8, which is a different claim from the unspecified + // address and rests on a different argument. + // + // It is NOT that these are independently reachable. Two reviewers checked + // in a Linux container and found they are not: `ip route get 0.0.0.5` + // takes the default route, not lo. The reason to block the /8 is the + // ordering dependency it removes. ::1 read as an IPv4-compatible address + // decodes to 0.0.0.1, so IPv6 loopback stays blocked today only because + // isPrivateIP's loopback check runs BEFORE its embedded-payload decode — + // an invariant no assertion in this package stated until now, and one that + // a reordering refactor would silently delete. Blocking 0.0.0.0/8 makes + // the payload private on its own merits, so the order stops mattering. + // See the ::1 row in transport_embedded_ipv4_test.go. + // + // "This host on this network" is in any case not a destination a caller + // legitimately names, so the range costs nothing to give up. + {"Zero network low", "0.0.0.1", true}, + {"Zero network mid", "0.1.2.3", true}, + {"Zero network high edge", "0.255.255.255", true}, + + // CGNAT 100.64.0.0/10 — carrier NAT and Tailscale meshes. + {"CGNAT low edge", "100.64.0.1", true}, + {"CGNAT high edge", "100.127.255.255", true}, + + // IETF protocol assignments 192.0.0.0/24. Both ends are pinned because a + // single interior row does not distinguish the /24 from the /29 that + // DS-Lite occupies inside it — an implementation narrowed to 192.0.0.0/29 + // would pass on 192.0.0.1 alone. + {"IETF protocol assignments first", "192.0.0.0", true}, + {"IETF protocol assignments last", "192.0.0.255", true}, + + // Benchmarking 198.18.0.0/15. + {"Benchmarking low edge", "198.18.0.1", true}, + {"Benchmarking high edge", "198.19.255.255", true}, + + // Reserved 240.0.0.0/4, pinned at both ends for the same reason. It runs + // all the way to the limited broadcast address, which is why there is no + // "just above" row: there is no above. Nor is there a meaningful "just + // below" — 239.255.255.255 is multicast and blocked on that ground. + {"Reserved former class E first", "240.0.0.0", true}, + {"Reserved former class E interior", "240.0.0.1", true}, + {"Reserved former class E penultimate", "255.255.255.254", true}, + {"Limited broadcast", "255.255.255.255", true}, + + // Multicast IPv4 224.0.0.0/4. + {"Multicast IPv4 internetwork control", "224.0.1.1", true}, + {"Multicast IPv4 source-specific", "233.1.2.3", true}, + {"Multicast IPv4 SSDP", "239.255.255.250", true}, + + // Multicast IPv6 — every scope, because scope is where a v4-shaped fix leaks. + {"Multicast IPv6 link-local all-nodes", "ff02::1", true}, + {"Multicast IPv6 site-local", "ff05::1", true}, + {"Multicast IPv6 global scope", "ff0e::1", true}, + + // IPv6 site-local fec0::/10 — deprecated by RFC 3879 and superseded by + // fc00::/7, but still an "internal network" range, and stacks that + // recognise it route it. It falls outside BOTH stdlib predicates that + // look like they should cover it: IsPrivate is fc00::/7 and + // IsLinkLocalUnicast is fe80::/10, and fec0::/10's bit pattern is + // disjoint from each. Neither neighbour gives it a boundary row — + // fe80::/10 below and ff00::/8 multicast above are both blocked already. + {"IPv6 site-local deprecated", "fec0::1", true}, + + // Boundaries that must STAY reachable, so the ranges above cannot be + // implemented one bit too wide. + {"Just above the zero network", "1.0.0.0", false}, + {"Just below CGNAT", "100.63.255.255", false}, + {"Just above CGNAT", "100.128.0.0", false}, + {"Just below benchmarking", "198.17.255.255", false}, + {"Just above benchmarking", "198.20.0.0", false}, + {"Just above IETF protocol assignments", "192.0.1.0", false}, + {"Above IETF protocol assignments", "192.0.1.1", false}, + {"Just below multicast", "223.255.255.255", false}, + {"Just below 10/8", "9.255.255.255", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tt.ip) + require.NotNil(t, ip, "the test's own input %q must parse as an IP", tt.ip) + + if tt.blocked { + assert.True(t, isPrivateIP(ip), + "isPrivateIP(%s) returned false: this address reaches a local or internal destination, "+ + "so a caller-supplied URL naming it is an SSRF the guard waves through", tt.ip) + return + } + assert.False(t, isPrivateIP(ip), + "isPrivateIP(%s) returned true: this is an ordinary public address one step outside a "+ + "reserved range, and blocking it means the range was implemented too wide", tt.ip) + }) + } +} + +// TestReservedNetworks_ContainNoPublicAddress is a tripwire on the range list +// itself, not on any address in particular. +// +// A CIDR entry can silently mean something far wider than it reads, and the +// failure is total rather than partial. The worked example, verified against the +// stdlib: `net.ParseCIDR("::ffff:0:0/96")` — a plausible-looking way to write the +// SIIT IPv4-translated prefix, and one group short of the real one — returns a +// network that prints as `0.0.0.0/0`. Its network number is the IPv4-mapped +// 0.0.0.0, so it passes To4, and Contains then compares in 4-byte space against +// the last four bytes of a 16-byte /96 mask, which are zero. Contains(8.8.8.8) is +// true. Added to the list below, that one entry refuses every outbound request +// the AppView makes — no federation, no identity resolution, nothing. +// +// A per-address table cannot catch this, because the table only asks about +// addresses someone thought to write down. This asks the inverse question of the +// whole list at once: does any entry claim an address that is unambiguously +// public? An entry that has degenerated answers yes to all of them. +// +// See the SIIT block in transport_embedded_ipv4_test.go for why that prefix must +// be a byte-pattern decode and must never be added here. +func TestReservedNetworks_ContainNoPublicAddress(t *testing.T) { + t.Parallel() + + // Ordinary public destinations, deliberately spread across the space rather + // than clustered: a degenerate entry catches all of them, a merely + // over-broad one might catch only its neighbourhood. + publicAddresses := []string{ + "8.8.8.8", + "1.1.1.1", + "93.184.216.34", + "2001:4860:4860::8888", + "2600::1", + } + + require.NotEmpty(t, reservedNetworks, + "the reserved-range list is empty, so every range test below is passing vacuously") + + for _, network := range reservedNetworks { + for _, address := range publicAddresses { + ip := net.ParseIP(address) + require.NotNil(t, ip, "the test's own input %q must parse as an IP", address) + + assert.False(t, network.Contains(ip), + "the reserved range %s contains the public address %s. Either it was written too wide, or it "+ + "is a prefix that DEGENERATED on parse — net.ParseCIDR reduces some IPv6 prefixes whose "+ + "network number passes To4 down to an IPv4 mask, and ::ffff:0:0/96 becomes 0.0.0.0/0 that "+ + "way. A range in this list that matches public traffic takes down every outbound request", + network, address) + } + } +} + +// TestIsPrivateIP_MappedSpellingsOfReservedRanges pins an assumption the range +// checks make without stating it. +// +// Every range above is expressed as an IPv4 CIDR, and every one of them is +// nonetheless matched when the same address arrives in its ::ffff: mapped form. +// That works because net.IPNet.Contains normalises through To4 internally — a +// standard-library behaviour, not something this package does. So the mapped +// spellings are correct today by inheritance, and nothing in the tree says they +// have to be. +// +// The rows matter because the inheritance is not guaranteed to survive a +// refactor. A range check rewritten as a byte-prefix comparison, a length-16 +// fast path, or a hand-rolled mask — all reasonable-looking optimisations for a +// predicate on the per-request hot path — drops the normalisation, and every one +// of these addresses becomes public while the unmapped spelling stays blocked. +// One prefix on a URL is not a difficult thing for an attacker to try. +// +// One row per range, plus a public control so a fix cannot pass by treating the +// mapped prefix itself as reserved. +func TestIsPrivateIP_MappedSpellingsOfReservedRanges(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ip string + blocked bool + }{ + {"Mapped zero network", "::ffff:0.0.0.1", true}, + {"Mapped CGNAT", "::ffff:100.64.0.1", true}, + {"Mapped IETF protocol assignments", "::ffff:192.0.0.1", true}, + {"Mapped benchmarking", "::ffff:198.18.0.1", true}, + {"Mapped reserved former class E", "::ffff:240.0.0.1", true}, + {"Mapped limited broadcast", "::ffff:255.255.255.255", true}, + {"Mapped multicast", "::ffff:224.0.1.1", true}, + {"Mapped link-local metadata service", "::ffff:169.254.169.254", true}, + {"Mapped RFC1918", "::ffff:10.0.0.1", true}, + + // The control: a public address in the same spelling. If this were + // blocked, the mapped prefix would be acting as a reserved range in its + // own right and the rows above would prove nothing. + {"Mapped public address", "::ffff:100.128.0.0", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tt.ip) + require.NotNil(t, ip, "the test's own input %q must parse as an IP", tt.ip) + + if tt.blocked { + assert.True(t, isPrivateIP(ip), + "isPrivateIP(%s) returned false: the unmapped spelling of this address is blocked, so a "+ + "caller need only write it with an ::ffff: prefix to reach the same destination", tt.ip) + return + } + assert.False(t, isPrivateIP(ip), + "isPrivateIP(%s) returned true: this is a public address, and blocking it means the mapped "+ + "prefix is being treated as reserved rather than decoded", tt.ip) + }) + } +} diff --git a/internal/atproto/oauth/transport_revetting_test.go b/internal/atproto/oauth/transport_revetting_test.go new file mode 100644 index 0000000..25fb8c6 --- /dev/null +++ b/internal/atproto/oauth/transport_revetting_test.go @@ -0,0 +1,257 @@ +package oauth + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "slices" + "sync" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Two properties of the guard, written as CHARACTERIZATION rather than +// regression tests. +// +// The distinction is worth keeping in view: neither test was written to drive a +// fix. Both properties are load-bearing and *incidental* — each falls out of +// where the vetting happens to sit rather than from anything that states it, so +// a reasonable-looking refactor could remove either and, before these tests +// existed, nothing would have noticed. +// +// Vetting inside `RoundTrip` is what gives per-hop redirect coverage, because +// `http.Client` calls `RoundTrip` once per hop; move the check up into a wrapper +// around `client.Do` and hop 2 goes unguarded while the rest of the suite stays +// green. Likewise the answer loop refuses on ANY private address in the slice, +// which is a `for` over all of them rather than an inspection of `ips[0]`. + +// hostRoutedResolver answers by hostname and records what it was asked, which is +// what lets a redirect test distinguish "hop 2 was resolved and refused" from +// "hop 2 was never looked at". +type hostRoutedResolver struct { + mu sync.Mutex + answers map[string][]net.IP + asked []string +} + +func (r *hostRoutedResolver) lookup(host string) ([]net.IP, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.asked = append(r.asked, host) + if ips, ok := r.answers[host]; ok { + return ips, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} +} + +func (r *hostRoutedResolver) hostsAsked() []string { + r.mu.Lock() + defer r.mu.Unlock() + return slices.Clone(r.asked) +} + +// TestSSRFSafeHTTPClient_RevetsEachRedirectHop pins that a redirect target is +// vetted as thoroughly as the URL the caller supplied. +// +// A redirect is an attacker-controlled input arriving from a server the caller +// already agreed to talk to, and it is the classic way to launder an SSRF: hop 1 +// is a public host that passes any front-door validation, and its 302 names +// 169.254.169.254. The only thing standing between that and the cloud metadata +// service is that the guard runs again on the second hop. +// +// `TestSSRFSafeHTTPClient_RedirectLimit` does not cover this — it calls +// `client.CheckRedirect` directly with fabricated requests and never performs a +// redirect, so it exercises the hop COUNT and nothing about hop CONTENT. +func TestSSRFSafeHTTPClient_RevetsEachRedirectHop(t *testing.T) { + t.Parallel() + + // One real listener stands in for both hops. Its handler counts, and the + // count is the assertion: since the dialler below sends every connection + // here regardless of destination, an unvetted hop 2 would arrive as a second + // invocation. Exactly one means the redirect was refused BEFORE the dial. + var invocations atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + invocations.Add(1) + w.Header().Set("Location", "http://hop2.test/") + w.WriteHeader(http.StatusFound) + })) + defer server.Close() + + // Checked, not assumed. A typo in either literal makes ParseIP return nil, + // isPrivateIP(nil) returns false, and hop 2 "vets clean" — and because the + // substituted dialler below ignores the address it is handed, the request + // would still land on the same listener and produce a plausible-looking + // failure. A fixture that can silently become nil can turn this whole test + // into one that asserts nothing. + hop1 := net.ParseIP("93.184.216.34") // public: vets clean + hop2 := net.ParseIP("169.254.169.254") // link-local: must be refused + require.NotNil(t, hop1, "the test's own hop-1 address must parse") + require.NotNil(t, hop2, "the test's own hop-2 address must parse") + + resolver := &hostRoutedResolver{answers: map[string][]net.IP{ + "hop1.test": {hop1}, + "hop2.test": {hop2}, + }} + + client := NewSSRFSafeHTTPClient(false) + transport, ok := client.Transport.(*ssrfSafeTransport) + require.True(t, ok, "NewSSRFSafeHTTPClient must install an ssrfSafeTransport, got %T", client.Transport) + transport.lookupIP = resolver.lookup + + // The base transport is substituted so hop 1 can vet as PUBLIC while still + // connecting to a loopback listener — no packet leaves the machine. That + // deliberately discards the vetted-address-dialling property for the duration + // of this test, which is safe only because + // TestSSRFTransport_DialsOnlyTheAddressItVetted owns that property outright. + // What is left under test here is RoundTrip's per-hop vetting, alone. + serverAddr := server.Listener.Addr().String() + dialer := &net.Dialer{} + transport.base = &http.Transport{ + DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) { + return dialer.DialContext(ctx, network, serverAddr) + }, + } + + resp, err := client.Get("http://hop1.test/") + if err == nil { + _ = resp.Body.Close() + } + + require.Error(t, err, "the redirect to a link-local host must be refused") + assert.Contains(t, err.Error(), "SSRF blocked", + "the refusal must name the guard that made it, so a transport error cannot be mistaken for a block; got: %v", err) + + assert.Equal(t, int64(1), invocations.Load(), + "the listener was reached %d times. Every connection this test makes lands on that one server, so a "+ + "second invocation means hop 2 was dialled without being vetted — the redirect target is chosen by "+ + "hop 1, not by the caller, which is precisely how an SSRF gets laundered through a host that passes "+ + "the front door", invocations.Load()) + + assert.Equal(t, []string{"hop1.test", "hop2.test"}, resolver.hostsAsked(), + "both hops must go through resolution: hop 2 being absent would mean it was refused for some reason "+ + "other than what it resolves to, and a repeated hop 1 would mean the redirect target was never read") +} + +// TestSSRFSafeHTTPClient_RefusesAMixedLookupAnswer pins that one private address +// anywhere in a lookup answer refuses the whole request. +// +// A hostname resolves to a SET, and the caller does not choose which member the +// dialler picks. An attacker who can publish two A records — one public, one +// 127.0.0.1 — needs the guard to inspect only `ips[0]`, or to be satisfied by +// "at least one answer is public", to get a coin-flip at a local service on every +// request. Refusing wholesale is the only answer that does not depend on which +// element the connection happens to use. +// +// Both orderings are pinned because a check that reads only the first answer +// passes one of them. +func TestSSRFSafeHTTPClient_RefusesAMixedLookupAnswer(t *testing.T) { + t.Parallel() + + // Checked for the same reason as the redirect test's fixtures: a nil from a + // typo'd literal is classified as public, which would quietly delete the + // mixed-answer premise this test is built on. + public := net.ParseIP("93.184.216.34") + private := net.ParseIP("127.0.0.1") + require.NotNil(t, public, "the test's own public address must parse") + require.NotNil(t, private, "the test's own private address must parse") + + tests := []struct { + name string + answer []net.IP + }{ + {"public answer first", []net.IP{public, private}}, + {"private answer first", []net.IP{private, public}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + resolver := &hostRoutedResolver{answers: map[string][]net.IP{"mixed.test": tt.answer}} + + client := NewSSRFSafeHTTPClient(false) + transport, ok := client.Transport.(*ssrfSafeTransport) + require.True(t, ok, "NewSSRFSafeHTTPClient must install an ssrfSafeTransport, got %T", client.Transport) + transport.lookupIP = resolver.lookup + + // A dialler that records and then fails. Recording is the point: an + // error alone cannot distinguish "the guard refused" from "the dial + // was attempted and failed", and only the former is the property. + var dialled atomic.Bool + transport.base = &http.Transport{ + DialContext: func(_ context.Context, _, addr string) (net.Conn, error) { + dialled.Store(true) + return nil, &net.OpError{Op: "dial", Net: "tcp", Err: net.UnknownNetworkError(addr)} + }, + } + + resp, err := client.Get("http://mixed.test/") + if err == nil { + _ = resp.Body.Close() + } + + require.Error(t, err, "an answer containing a private address must be refused") + assert.Contains(t, err.Error(), "SSRF blocked", + "the refusal must name the guard that made it; got: %v", err) + assert.Contains(t, err.Error(), private.String(), + "the refusal must name the address that caused it, so an operator reading the log knows which "+ + "of the answers was the problem; got: %v", err) + + assert.False(t, dialled.Load(), + "a connection was attempted despite a private address in the answer. The dialler picks which "+ + "member of the set to use and the caller has no say, so any private member has to refuse the "+ + "whole request rather than hoping a public one is chosen") + }) + } +} + +// TestSSRFSafeTransport_BaseTransportFailsClosedWhenBypassed pins the dialler's +// refusal to connect when nothing has vetted the destination. +// +// The dial reads its approved addresses off the request context, where RoundTrip +// put them. Reaching the dialler with that value absent means the base transport +// was driven directly instead of through RoundTrip — the unguarded path the +// wrapper exists to prevent — and the dialler's answer is to refuse rather than +// to fall back on the address in `addr`. A fallback would be worse than no guard +// at all, because it would look guarded. +// +// Bypassing RoundTrip is therefore the thing under test, and it is why this test +// reaches into `transport.base` rather than calling `client.Get`. There is no +// way to exercise the branch through the public API: going through the front +// door is precisely what populates the context value. +// +// The branch had no coverage before this test, which matters more than it sounds +// now that `transport.base` substitution is an established pattern in this +// package (see the two tests above). A refactor that dropped the fail-closed +// check — or replaced it with a dial of `addr` — would leave the entire suite +// green while removing the last barrier on a code path that reaches any address +// a caller names. +func TestSSRFSafeTransport_BaseTransportFailsClosedWhenBypassed(t *testing.T) { + t.Parallel() + + client := NewSSRFSafeHTTPClient(false) + transport, ok := client.Transport.(*ssrfSafeTransport) + require.True(t, ok, "NewSSRFSafeHTTPClient must install an ssrfSafeTransport, got %T", client.Transport) + + // Port 9 is discard. The point is that nothing should get far enough to find + // out whether anything is listening: the refusal must come from the guard, + // before a socket is opened. + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:9/", nil) + require.NoError(t, err, "building the request") + + resp, err := transport.base.RoundTrip(req) + if err == nil { + _ = resp.Body.Close() + } + + require.Error(t, err, "the base transport must refuse a request that carries no vetted address") + assert.Contains(t, err.Error(), "the SSRF-safe transport was bypassed", + "the refusal must say that the wrapper was skipped rather than report a generic dial failure — an "+ + "operator seeing this in a log needs to know a caller reached the base transport directly; got: %v", err) + assert.Contains(t, err.Error(), "SSRF blocked", + "the refusal must name the guard that made it; got: %v", err) +} diff --git a/internal/atproto/oauth/transport_unspecified_address_test.go b/internal/atproto/oauth/transport_unspecified_address_test.go new file mode 100644 index 0000000..9840ab7 --- /dev/null +++ b/internal/atproto/oauth/transport_unspecified_address_test.go @@ -0,0 +1,80 @@ +package oauth + +import ( + "net" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSSRFSafeHTTPClient_RefusesTheUnspecifiedAddress pins that 0.0.0.0 is +// treated as the loopback address it behaves like. +// +// # WHY 0.0.0.0 REACHES A LOCAL SERVICE +// +// 0.0.0.0 is RFC 1122's "this host on this network", a wildcard that means +// something only in a bind() call. In a connect() call the kernel does not treat +// it as a wildcard — on Linux and on darwin alike it substitutes the local host, +// so connecting to 0.0.0.0:5432 lands on whatever is listening on +// 127.0.0.1:5432. +// +// That is what makes it dangerous to a guard built by enumerating blocks. An +// address classifier that listed loopback, link-local, RFC1918 and the IPv6 +// private ranges — which is what this one did before this test — would find +// 0.0.0.0 in none of them and call it public, and its classification and the +// kernel's behaviour would then disagree about where the packet goes. The kernel +// is the one that opens the socket. The result would be a complete bypass rather +// than a gap: `http://0.0.0.0:5432/` is a URL an attacker types, it passes such a +// check on its face, and it reaches Postgres. Every input that reaches this +// transport is chosen by a stranger — a DID document's PDS endpoint, an +// acceptance record's subject — so nothing upstream constrains the literal to be +// one we would have picked. +// +// The predicate now recognises it (via IsUnspecified, which also covers :: and +// ::ffff:0.0.0.0), and this test is what holds that in place. +// +// It asserts through NewSSRFSafeHTTPClient rather than against `isPrivateIP`, +// because the contract is the client's refusal to CONNECT. A predicate returning +// true is evidence; a service that was never touched is the property. +func TestSSRFSafeHTTPClient_RefusesTheUnspecifiedAddress(t *testing.T) { + t.Parallel() + + // A real listener on loopback, so the test can tell "refused" from "the port + // happened to be closed". The handler flag is the load-bearing assertion: an + // implementation that connects and only then reports an error still delivered + // the request to a local service, and must fail here. + var reached atomic.Bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached.Store(true) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + _, port, err := net.SplitHostPort(strings.TrimPrefix(server.URL, "http://")) + require.NoError(t, err, "splitting the test server address %q", server.URL) + + // The port is the server's; the host is rewritten to the unspecified address. + // Rebuilding the URL rather than hardcoding a port is what keeps the target a + // listener this test owns. + target := "http://" + net.JoinHostPort("0.0.0.0", port) + "/" + + client := NewSSRFSafeHTTPClient(false) + resp, err := client.Get(target) + if err == nil { + _ = resp.Body.Close() + } + + assert.False(t, reached.Load(), + "GET %s reached the loopback listener: 0.0.0.0 passed the private-address check, and the kernel "+ + "then resolved it to the local host at connect time. The guard classified an address it never "+ + "actually dialled, so an attacker-supplied URL like http://0.0.0.0:5432/ hits a local service", + target) + require.Error(t, err, "GET %s must be refused", target) + assert.Contains(t, err.Error(), "SSRF blocked", + "the refusal must name the guard that made it, so a closed port cannot green this test; got: %v", err) +}