diff --git a/README.md b/README.md index 7a71a5d..6338c40 100644 --- a/README.md +++ b/README.md @@ -694,7 +694,10 @@ Key settings in more detail: and generator pages. Default: empty - `blocked_networks` — list of CIDR ranges to refuse for SSRF protection, added to the always-enforced built-in ranges (loopback, private, link-local, CGNAT, - benchmark, NAT64, and the like); an invalid CIDR aborts startup + benchmark, NAT64, and the like); an invalid CIDR aborts startup. An IPv6 + address in the IPv4-mapped form (`::ffff:a.b.c.d`) is matched only as the IPv4 + address it holds, so an IPv6 range such as `::/0` refuses neither it nor a + plain IPv4 address - `trusted_proxies` — list of CIDR ranges of the reverse proxies in front of pixa. `X-Forwarded-For` is believed only when the direct peer falls inside one of these ranges; the logged and login-recorded client address is then the diff --git a/TODO.md b/TODO.md index 5009531..596940e 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,18 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-16 P1 security bug: the check that keeps pixa from fetching internal + addresses refuses the local-use NAT64 range (`64:ff9b:1::/48`, RFC 8215) as it + refuses `64:ff9b::/96`, and refuses an IPv6 address in the IPv4-compatible + form (`::/96`) when `blocked_networks` contains either the address as written + or the IPv4 address it holds (closes #299). A source host such as + `[64:ff9b:1::a00:1]` was fetched, which on a network that runs NAT64 on that + range reaches the IPv4 address it holds, here `10.0.0.1`; and with + `203.0.113.0/24` in `blocked_networks`, `[::ffff:203.0.113.5]` was refused but + `[::203.0.113.5]` was fetched. Both now answer 403. The mapped form + (`::ffff:a.b.c.d`) is still matched only as the IPv4 address it holds: Go + keeps a plain IPv4 address in that form, and an IPv6 entry such as `::/0` must + not refuse every IPv4 address. - 2026-10-16 P1 security bug: the check that keeps pixa from fetching internal addresses judges an IPv6 address in the IPv4-compatible form (`::/96`), such as `::127.0.0.1` or `::10.0.0.1`, by the IPv4 address in its last four bytes diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 53d5b20..29373f4 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -59,15 +59,17 @@ const ( // builtinBlockedPrefixes are internal or special-use ranges that Go's // net.IP predicates (IsPrivate, IsLinkLocalUnicast, and the like) do not // already cover. They are always blocked, in addition to any -// operator-supplied networks. IPv4-mapped IPv6 addresses are unmapped -// before matching, so these IPv4 ranges are caught in both forms. +// operator-supplied networks. IPv4-mapped and IPv4-compatible IPv6 +// addresses are read as the IPv4 address they hold before matching, so +// these IPv4 ranges are caught in all three forms. // //nolint:gochecknoglobals // immutable built-in blocklist var builtinBlockedPrefixes = []netip.Prefix{ - netip.MustParsePrefix("100.64.0.0/10"), // RFC 6598 CGNAT / carrier-grade NAT - netip.MustParsePrefix("192.0.0.0/24"), // RFC 6890 IETF protocol assignments - netip.MustParsePrefix("198.18.0.0/15"), // RFC 2544 benchmarking range - netip.MustParsePrefix("64:ff9b::/96"), // RFC 6052 NAT64 (maps onto IPv4) + netip.MustParsePrefix("100.64.0.0/10"), // RFC 6598 CGNAT / carrier-grade NAT + netip.MustParsePrefix("192.0.0.0/24"), // RFC 6890 IETF protocol assignments + netip.MustParsePrefix("198.18.0.0/15"), // RFC 2544 benchmarking range + netip.MustParsePrefix("64:ff9b::/96"), // RFC 6052 NAT64 (maps onto IPv4) + netip.MustParsePrefix("64:ff9b:1::/48"), // RFC 8215 local-use NAT64 (maps onto IPv4) } // Fetcher errors. @@ -543,12 +545,8 @@ func isPrivateIP(ip net.IP) bool { return true } - // An IPv4-compatible IPv6 address (in ::/96, such as ::127.0.0.1) holds - // an IPv4 address in its last four bytes and is judged as that address - if addr, ok := netip.AddrFromSlice(ip); ok && - netip.MustParsePrefix("::/96").Contains(addr) { - return isPrivateIP(ip[12:]) - } + // An IPv4-compatible IPv6 address is judged as the IPv4 address it holds + ip = ipv4CompatibleToIPv4(ip) // Check for loopback if ip.IsLoopback() { @@ -601,7 +599,12 @@ func isPrivateIP(ip net.IP) bool { } // isBlockedIP reports whether ip is refused, either by the built-in -// internal-range check or by one of the operator-supplied prefixes. +// internal-range check or by one of the operator-supplied prefixes. An +// IPv4-compatible address (in ::/96) is refused when a prefix contains the +// address as written or the IPv4 address it holds. An IPv4-mapped address (in +// ::ffff:0:0/96) is matched only as the IPv4 address it holds: Go keeps a +// plain IPv4 address in that form too, and an IPv6 prefix such as ::/0 must +// not refuse every IPv4 address. func isBlockedIP(ip net.IP, blocked []netip.Prefix) bool { if isPrivateIP(ip) { return true @@ -614,11 +617,28 @@ func isBlockedIP(ip net.IP, blocked []netip.Prefix) bool { addr = addr.Unmap() + // ip read as IPv4 where it is or holds an IPv4 address, otherwise the + // zero netip.Addr, which no prefix contains. Matching asIPv4 as well as + // addr refuses more only for an IPv4-compatible address. + asIPv4, _ := netip.AddrFromSlice(ipv4CompatibleToIPv4(ip).To4()) + return slices.ContainsFunc(blocked, func(prefix netip.Prefix) bool { - return prefix.Contains(addr) + return prefix.Contains(addr) || prefix.Contains(asIPv4) }) } +// ipv4CompatibleToIPv4 returns the IPv4 address that an IPv4-compatible IPv6 +// address (in ::/96, such as ::127.0.0.1) holds in its last four bytes, and +// any other address unchanged. +func ipv4CompatibleToIPv4(ip net.IP) net.IP { + addr, ok := netip.AddrFromSlice(ip) + if ok && netip.MustParsePrefix("::/96").Contains(addr) { + return ip[12:] + } + + return ip +} + // ssrfSafeDialer validates IP addresses against the built-in blocked ranges // before connecting. New wraps dialSSRFSafe with the operator-supplied // blocklist; this entry point enforces the built-in ranges alone. diff --git a/internal/server/image_proxy_internal_test.go b/internal/server/image_proxy_internal_test.go index 24b7961..ba56e4f 100644 --- a/internal/server/image_proxy_internal_test.go +++ b/internal/server/image_proxy_internal_test.go @@ -14,10 +14,12 @@ import ( "net" "net/http" "net/http/httptest" + "net/netip" "os" "path/filepath" "sync/atomic" "testing" + "time" "go.uber.org/fx" "go.uber.org/fx/fxtest" @@ -183,6 +185,57 @@ func startImageProxy( return s, db.DB(), stateDir } +// startImageProxyWithBlockedNetworks starts the components pixad's fx app +// builds, from a config with a fresh state directory and blockedNetworks as +// blocked_networks. Unlike startImageProxy it provides no upstream fetcher, so +// the handlers build the one pixad uses, which checks every address it +// connects to against the built-in blocked ranges and blocked_networks. It +// returns the server with its routes. +func startImageProxyWithBlockedNetworks( + t *testing.T, blockedNetworks []netip.Prefix, +) *Server { + t.Helper() + + stateDir := t.TempDir() + cfg := &config.Config{ + SigningKey: testSigningKey, + StateDir: stateDir, + DBURL: "file:" + filepath.Join(stateDir, "state.sqlite3"), + BlockedNetworks: blockedNetworks, + // A fetch that gets past the checks and finds nothing at the address + // fails within this time at the latest. + UpstreamFetchTimeout: 2 * time.Second, + // With no connection slots, the fetch would fail before connecting. + UpstreamConnections: config.DefaultUpstreamConnections, + DownstreamTimeout: config.DefaultDownstreamTimeout, + } + + var ( + h *handlers.Handlers + mw *middleware.Middleware + ) + + app := fxtest.New(t, + fx.Supply(cfg), + fx.Provide( + globals.New, + logger.New, + database.New, + healthcheck.New, + handlers.New, + middleware.New, + ), + fx.Populate(&h, &mw), + ) + app.RequireStart() + t.Cleanup(app.RequireStop) + + s := &Server{config: cfg, mw: mw, h: h} + s.SetupRoutes() + + return s +} + // getImage sends a GET for target to s and fails unless it answers 200. func getImage(t *testing.T, s *Server, target string) *httptest.ResponseRecorder { t.Helper() diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index 172de90..27b963b 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -3,6 +3,7 @@ package server import ( "net/http" "net/http/httptest" + "net/netip" "net/url" "regexp" "slices" @@ -586,3 +587,79 @@ func TestImageRoutesRefuseIPv6SourceHostHoldingInternalIPv4Address(t *testing.T) t.Errorf("upstream received %q, want no request", got) } } + +// TestImageRoutesCheckIPv4AddressInIPv6SourceHost requests ipv6SourcePath on +// source hosts that hold an IPv4 address, through a /v1/image/ URL with a +// signature and through an encrypted URL, with the fetcher pixad uses. +// [64:ff9b:1::a00:1], in the local-use NAT64 range (64:ff9b:1::/48, RFC 8215), +// answers 403. [::203.0.113.5] holds the public address 203.0.113.5 in the +// IPv4-compatible form (::/96) and answers 403 when blocked_networks lists +// 203.0.113.0/24 or ::/96. [::ffff:203.0.113.5] holds it in the IPv4-mapped +// form (::ffff:0:0/96) and answers 403 when blocked_networks lists +// 203.0.113.0/24. Go keeps the plain address 203.0.113.5 in the mapped form +// too, and an IPv6 range must not refuse it: with ::/0 listed it is not +// refused. Neither is [::203.0.113.5] with blocked_networks empty. Where a +// host is not refused, the fetcher tries to connect to it, finds nothing +// there, and the request fails with another status. +func TestImageRoutesCheckIPv4AddressInIPv6SourceHost(t *testing.T) { + t.Parallel() + + const ( + ipv4CompatibleHost = "[::203.0.113.5]" + ipv4MappedHost = "[::ffff:203.0.113.5]" + ) + + tests := []struct { + name string + sourceHost string + blockedNetworks []netip.Prefix + wantForbidden bool + }{ + {"local-use NAT64 form of 10.0.0.1", "[64:ff9b:1::a00:1]", nil, true}, + {"IPv4-compatible form of an address in blocked_networks", + ipv4CompatibleHost, + []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, true}, + {"IPv4-compatible form with ::/96 in blocked_networks", + ipv4CompatibleHost, + []netip.Prefix{netip.MustParsePrefix("::/96")}, true}, + {"IPv4-mapped form of an address in blocked_networks", + ipv4MappedHost, + []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, true}, + {"plain IPv4 address with ::/0 in blocked_networks", + "203.0.113.5", + []netip.Prefix{netip.MustParsePrefix("::/0")}, false}, + {"IPv4-compatible form of an address with blocked_networks empty", + ipv4CompatibleHost, nil, false}, + } + + for _, tt := range tests { + imageURLs := map[string]string{ + "image URL with a signature": signedImageURL(t, tt.sourceHost), + "encrypted URL": encryptedImageURL(t, tt.sourceHost), + } + + for route, imageURL := range imageURLs { + t.Run(tt.name+" "+route, func(t *testing.T) { + t.Parallel() + + s := startImageProxyWithBlockedNetworks(t, tt.blockedNetworks) + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, imageURL, nil)) + t.Logf("GET %s: %d", imageURL, rec.Code) + + forbidden := rec.Code == http.StatusForbidden + if tt.wantForbidden && !forbidden { + t.Errorf("status = %d, want %d; body %s", + rec.Code, http.StatusForbidden, rec.Body.String()) + } + + if !tt.wantForbidden && forbidden { + t.Errorf("status = %d, want any other status; body %s", + rec.Code, rec.Body.String()) + } + }) + } + } +}