From 3a6b3bc202b62563dd6f5ed42ac6e3850b8bab52 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 16 Oct 2026 05:31:55 +0200 Subject: [PATCH] Refuse an IPv6 address that holds an internal IPv4 address (closes #297) The check that keeps pixa from fetching internal addresses read an IPv6 address as IPv4 only in the mapped form (::ffff:a.b.c.d), so the IPv4-compatible form, such as ::127.0.0.1, was judged as IPv6 and passed. An address in ::/96 is now judged by the IPv4 address it holds, so one holding a private, loopback or link-local address is refused on both image routes before any upstream request. The NAT64 range 64:ff9b::/96 stays blocked whole, as it was on next. Model: opus-5-5 --- TODO.md | 8 ++++ internal/httpfetcher/httpfetcher.go | 7 +++ .../httpfetcher/httpfetcher_internal_test.go | 38 ++++++++++++++++ internal/server/source_url_internal_test.go | 43 +++++++++++++++++++ 4 files changed, 96 insertions(+) diff --git a/TODO.md b/TODO.md index 9ae228e..5009531 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,14 @@ P2: security: per-origin rate limiting # Completed Steps +- 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 + (closes #297). It read only the mapped form (`::ffff:a.b.c.d`) as IPv4, so a + source host such as `[::127.0.0.1]` was checked as a public IPv6 address and + fetched, which on a network that routes it reaches the internal IPv4 host; it + is now refused with 403. The NAT64 form (`64:ff9b::/96`) was already refused + as a whole, as one of the built-in blocked ranges, and still is. - 2026-10-16 a source host that is an IPv6 address in brackets with no port, such as `[2001:db8::a]`, is fetched like one with a port, through `/v1/image/` and through an encrypted URL (closes #295). The fetcher's check of the source diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 159ee68..53d5b20 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -543,6 +543,13 @@ 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:]) + } + // Check for loopback if ip.IsLoopback() { return true diff --git a/internal/httpfetcher/httpfetcher_internal_test.go b/internal/httpfetcher/httpfetcher_internal_test.go index a774614..c7a8970 100644 --- a/internal/httpfetcher/httpfetcher_internal_test.go +++ b/internal/httpfetcher/httpfetcher_internal_test.go @@ -186,6 +186,44 @@ func TestIsPrivateIP(t *testing.T) { } } +// TestIsPrivateIPJudgesEmbeddedIPv4Address checks IPv6 addresses that hold an +// IPv4 address in their last four bytes: the IPv4-compatible form (::/96) and +// the NAT64 form (64:ff9b::/96). One holding a loopback, private or +// link-local IPv4 address is refused as that address is, and one holding a +// public IPv4 address in the IPv4-compatible form passes. +func TestIsPrivateIPJudgesEmbeddedIPv4Address(t *testing.T) { + t.Parallel() + + tests := []struct { + ip string + want bool + }{ + {"::127.0.0.1", true}, // loopback + {"::10.0.0.1", true}, // private + {"::169.254.169.254", true}, // link-local + {"64:ff9b::7f00:1", true}, // NAT64 form of 127.0.0.1 + {"::8.8.8.8", false}, // public + } + + for _, tc := range tests { + t.Run(tc.ip, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tc.ip) + if ip == nil { + t.Fatalf("failed to parse IP %q", tc.ip) + } + + got := isPrivateIP(ip) + t.Logf("isPrivateIP(%s) = %v", ip, got) + + if got != tc.want { + t.Errorf("isPrivateIP(%q) = %v, want %v", tc.ip, got, tc.want) + } + }) + } +} + func TestValidateURL_RejectsNonHTTPS(t *testing.T) { t.Parallel() diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index 0a5b4a1..172de90 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -543,3 +543,46 @@ func encryptedImageURL(t *testing.T, sourceHost string) string { return "/v1/e/" + token + "/img.png" } + +// TestImageRoutesRefuseIPv6SourceHostHoldingInternalIPv4Address requests +// ipv6SourcePath on IPv6 source hosts that hold a loopback or private IPv4 +// address in their last four bytes, in the IPv4-compatible form (::/96) or the +// NAT64 form (64:ff9b::/96), through a /v1/image/ URL with a signature and +// through an encrypted URL. Each answers 403, and the upstream receives no +// request: the test's fetcher connects every address to the test upstream, so +// only the fetcher's check of the source URL can refuse it. +func TestImageRoutesRefuseIPv6SourceHostHoldingInternalIPv4Address(t *testing.T) { + t.Parallel() + + upstream := startRecordingUpstream(t) + s, _, _ := startImageProxy(t, upstream.Server) + + sourceHosts := []string{ + "[::127.0.0.1]", // IPv4-compatible form of a loopback address + "[::10.0.0.1]", // IPv4-compatible form of a private address + "[64:ff9b::7f00:1]", // NAT64 form of 127.0.0.1 + } + + for _, sourceHost := range sourceHosts { + imageURLs := []string{ + signedImageURL(t, sourceHost), + encryptedImageURL(t, sourceHost), + } + + for _, imageURL := range imageURLs { + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, imageURL, nil)) + t.Logf("GET %s (source host %s): %d", imageURL, sourceHost, rec.Code) + + if rec.Code != http.StatusForbidden { + t.Errorf("source host %s: status = %d, want %d; body %s", + sourceHost, rec.Code, http.StatusForbidden, rec.Body.String()) + } + } + } + + if got := upstream.requestTargets(); len(got) != 0 { + t.Errorf("upstream received %q, want no request", got) + } +}