From daedb3518fe02748fd76e9b59028c9f3839c4f3e Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 13:07:18 +0200 Subject: [PATCH] Check the destination of requests sent through a forward proxy safehttp.New clones http.DefaultTransport, which keeps ProxyFromEnvironment, and the address gate only wraps DialContext. With HTTP_PROXY or HTTPS_PROXY set the transport dials the forward proxy, so the gate checked the forward proxy's address and never the destination. newClient now also wraps transport.Proxy: for a request that goes to a forward proxy, the destination host is resolved and every address has to pass the same gate with the same Options. A name that cannot be resolved locally is still passed on, since some networks resolve public names only at the forward proxy. Direct requests are unchanged, and the TinyGo client is not affected. Fixes #115 --- safehttp/safehttp.go | 8 +++ safehttp/transport_std.go | 45 +++++++++++++ safehttp/transport_std_test.go | 118 +++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+) create mode 100644 safehttp/transport_std_test.go diff --git a/safehttp/safehttp.go b/safehttp/safehttp.go index 78d9035..f0a3212 100644 --- a/safehttp/safehttp.go +++ b/safehttp/safehttp.go @@ -8,6 +8,14 @@ // directly, so a rebind between check and connect cannot escape // the gate. // +// When the transport sends a request through a forward proxy (for +// example from HTTP_PROXY or HTTPS_PROXY), the dial reaches the +// forward proxy, so the destination is checked before the request +// is handed over: it is resolved and every address must pass the +// same gate. A name that cannot be resolved locally is passed to +// the forward proxy, since some networks resolve public names only +// there. +// // 2. Redirect cap. Go's default ignores chain length and re-trusts // each hop. CheckRedirect caps at 10 and re-validates every // redirect target. diff --git a/safehttp/transport_std.go b/safehttp/transport_std.go index 2244ec6..fed2b59 100644 --- a/safehttp/transport_std.go +++ b/safehttp/transport_std.go @@ -7,8 +7,13 @@ import ( "fmt" "net" "net/http" + "net/url" ) +// lookupIPAddr resolves the destination of a request sent through a forward +// proxy. It is a variable so tests can resolve names without DNS. +var lookupIPAddr = net.DefaultResolver.LookupIPAddr + func newClient(base *http.Client, opts Options) *http.Client { c := http.Client{Timeout: defaultTimeout} if base != nil { @@ -33,6 +38,7 @@ func newClient(base *http.Client, opts Options) *http.Client { transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { return gate.dial(ctx, network, addr, underlying) } + transport.Proxy = guardProxy(transport.Proxy, gate) c.Transport = transport c.CheckRedirect = func(req *http.Request, via []*http.Request) error { @@ -43,3 +49,42 @@ func newClient(base *http.Client, opts Options) *http.Client { } return &c } + +// guardProxy wraps proxy so that a request sent through a forward proxy is +// held to the same address rules as a direct one. The dial gate only sees the +// address it connects to, which for a proxied request is the forward proxy. +// +// The destination is resolved here and every address must pass. A host that +// cannot be resolved locally is passed on, since networks that route all +// traffic through a forward proxy often resolve public names only there. +func guardProxy(proxy func(*http.Request) (*url.URL, error), gate *ipGate) func(*http.Request) (*url.URL, error) { + if proxy == nil { + return nil + } + return func(req *http.Request) (*url.URL, error) { + proxyURL, err := proxy(req) + if err != nil || proxyURL == nil { + return proxyURL, err + } + if err := gate.checkProxied(req.Context(), req.URL.Hostname()); err != nil { + return nil, err + } + return proxyURL, nil + } +} + +func (g *ipGate) checkProxied(ctx context.Context, host string) error { + if ip := net.ParseIP(host); ip != nil { + return g.checkHost(host, ip) + } + addrs, err := lookupIPAddr(ctx, host) + if err != nil { + return nil //nolint:nilerr // the forward proxy resolves names this host cannot + } + for _, addr := range addrs { + if err := g.checkHost(host, addr.IP); err != nil { + return err + } + } + return nil +} diff --git a/safehttp/transport_std_test.go b/safehttp/transport_std_test.go new file mode 100644 index 0000000..0e5eb5d --- /dev/null +++ b/safehttp/transport_std_test.go @@ -0,0 +1,118 @@ +//go:build !tinygo + +package safehttp + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" +) + +// routeThroughTestProxy starts a forward proxy that answers every request +// itself, returns a base client whose transport sends requests to it, and +// resolves hostnames from hosts. +func routeThroughTestProxy(t *testing.T, hosts map[string]string) (*http.Client, *atomic.Int32) { + t.Helper() + var received atomic.Int32 + forward := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + received.Add(1) + _, _ = w.Write([]byte("via proxy")) + })) + t.Cleanup(forward.Close) + forwardURL, err := url.Parse(forward.URL) + if err != nil { + t.Fatal(err) + } + + oldLookup := lookupIPAddr + lookupIPAddr = func(_ context.Context, host string) ([]net.IPAddr, error) { + if ip, ok := hosts[host]; ok { + return []net.IPAddr{{IP: net.ParseIP(ip)}}, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} + } + t.Cleanup(func() { lookupIPAddr = oldLookup }) + + base := &http.Client{Transport: &http.Transport{Proxy: http.ProxyURL(forwardURL)}} + return base, &received +} + +func TestClient_ForwardProxyChecksDestination(t *testing.T) { + base, received := routeThroughTestProxy(t, map[string]string{ + "metadata.example.test": "169.254.169.254", + "intranet.example.test": "10.1.2.3", + "allowed.example.test": "10.1.2.4", + "public.example.test": "203.0.113.10", + }) + // The forward proxy listens on loopback, so the client has to be allowed + // to dial it. + c := New(base, Options{ + AllowLoopback: true, + AllowPrivateHosts: []string{"allowed.example.test", "10.9.9.9"}, + }) + + for _, tt := range []struct { + url string + blocked bool + }{ + {"http://169.254.169.254/latest/meta-data/", true}, + {"https://169.254.169.254/latest/meta-data/", true}, + {"http://[fe80::1]/", true}, + {"http://metadata.example.test/", true}, + {"http://10.1.2.3/", true}, + {"http://intranet.example.test/", true}, + {"http://allowed.example.test/", false}, + {"http://10.9.9.9/", false}, + {"http://203.0.113.10/", false}, + {"http://public.example.test/", false}, + {"http://unresolvable.example.test/", false}, + } { + before := received.Load() + resp, err := c.Get(tt.url) + if err == nil { + _ = resp.Body.Close() + } + reached := received.Load() > before + if tt.blocked { + if err == nil || reached { + t.Errorf("GET %s: err = %v, reached forward proxy = %v; want refused before the forward proxy", tt.url, err, reached) + } + continue + } + if err != nil || !reached { + t.Errorf("GET %s: err = %v, reached forward proxy = %v; want it passed to the forward proxy", tt.url, err, reached) + } + } +} + +func TestClient_DefaultTransportProxyIsGuarded(t *testing.T) { + if http.DefaultTransport.(*http.Transport).Proxy == nil { + t.Skip("http.DefaultTransport has no proxy func") + } + c := New(nil, Options{}) + if c.Transport.(*http.Transport).Proxy == nil { + t.Fatal("proxy func was dropped") + } +} + +func TestGuardProxyKeepsDirectRequests(t *testing.T) { + gate := newGate(Options{}) + if guardProxy(nil, gate) != nil { + t.Error("a transport without a proxy func got one") + } + req := httptest.NewRequest(http.MethodGet, "http://169.254.169.254/", nil) + proxyErr := errors.New("bad proxy config") + guarded := guardProxy(func(*http.Request) (*url.URL, error) { return nil, proxyErr }, gate) + if _, err := guarded(req); !errors.Is(err, proxyErr) { + t.Errorf("err = %v, want the proxy func's own error", err) + } + direct := guardProxy(func(*http.Request) (*url.URL, error) { return nil, nil }, gate) + if proxyURL, err := direct(req); proxyURL != nil || err != nil { + t.Errorf("direct request: proxy = %v, err = %v; want nil, nil so the dial gate checks it", proxyURL, err) + } +}