From a40d145b21cb2cf239308ca2f28a5dfb4d1a3a6c Mon Sep 17 00:00:00 2001 From: Basavaraj S m Date: Tue, 29 Sep 2026 00:26:36 +0530 Subject: [PATCH] derp/derphttp: reject invalid DERP node hostname before proxy CONNECT (#21038) * derp/derphttp: reject invalid DERP node hostname before proxy CONNECT When a DERP client reaches a node through an HTTP(S) proxy, dialNodeUsingProxy writes the CONNECT request by hand and puts net.JoinHostPort(n.HostName, port) into both the request line and the Host header. n.HostName comes from the control-supplied DERP map and net.JoinHostPort does no sanitizing, so a hostname carrying CR/LF was written verbatim into the plaintext request sent to the proxy. That let whoever populated the DERP map inject extra headers, or a second pipelined request, into the connection to the operator's proxy. Validate n.HostName with httpguts.ValidHostHeader at the top of dialNodeUsingProxy, before the proxy is dialed, and also reject the empty hostname, which ValidHostHeader accepts. DNS names and IP literals continue to work. Add a table-driven test that runs accepted and rejected hostnames against a fake proxy and checks the CONNECT target that goes out. Fixes tailscale/corp#48122 Signed-off-by: basavaraj-sm05 Co-authored-by: Mike Jensen Signed-off-by: Mike Jensen --- cmd/derper/depaware.txt | 3 +- cmd/tailscale/depaware.txt | 1 + cmd/tailscaled/depaware-min.txt | 2 +- cmd/tailscaled/depaware-minbox.txt | 2 +- cmd/tailscaled/depaware.txt | 2 +- cmd/tsidp/depaware.txt | 2 +- derp/derphttp/derphttp_client.go | 11 +++ derp/derphttp/derphttp_proxy_test.go | 101 +++++++++++++++++++++++++++ tsnet/depaware.txt | 2 +- 9 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 derp/derphttp/derphttp_proxy_test.go diff --git a/cmd/derper/depaware.txt b/cmd/derper/depaware.txt index 765589bd2..5cfebf4de 100644 --- a/cmd/derper/depaware.txt +++ b/cmd/derper/depaware.txt @@ -195,7 +195,8 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa golang.org/x/exp/maps from tailscale.com/util/syspolicy/setting L golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/net/dnscache - golang.org/x/net/idna from golang.org/x/crypto/acme/autocert + golang.org/x/net/http/httpguts from tailscale.com/derp/derphttp + golang.org/x/net/idna from golang.org/x/crypto/acme/autocert+ golang.org/x/net/internal/socks from golang.org/x/net/proxy golang.org/x/net/proxy from tailscale.com/net/netns D golang.org/x/net/route from tailscale.com/net/netmon+ diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 6eb51318c..4be4bfcd0 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -369,6 +369,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep L golang.org/x/image/math/fixed from github.com/fogleman/gg+ golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/cmd/tailscale/cli+ + golang.org/x/net/http/httpguts from tailscale.com/derp/derphttp golang.org/x/net/http/httpproxy from tailscale.com/net/tshttpproxy golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpproxy+ diff --git a/cmd/tailscaled/depaware-min.txt b/cmd/tailscaled/depaware-min.txt index 497cf0445..fb12159c3 100644 --- a/cmd/tailscaled/depaware-min.txt +++ b/cmd/tailscaled/depaware-min.txt @@ -220,7 +220,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de golang.org/x/exp/maps from tailscale.com/ipn/store/mem golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/ipn/ipnlocal+ - golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal + golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal+ golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpguts golang.org/x/net/internal/iana from golang.org/x/net/icmp+ diff --git a/cmd/tailscaled/depaware-minbox.txt b/cmd/tailscaled/depaware-minbox.txt index d8edca0c9..caf037ae0 100644 --- a/cmd/tailscaled/depaware-minbox.txt +++ b/cmd/tailscaled/depaware-minbox.txt @@ -241,7 +241,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de golang.org/x/exp/maps from tailscale.com/ipn/store/mem golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/cmd/tailscale/cli+ - golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal + golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal+ golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpguts+ golang.org/x/net/internal/iana from golang.org/x/net/icmp+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 7d14816b8..5eccd04cc 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -564,7 +564,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de golang.org/x/exp/maps from tailscale.com/ipn/store/mem+ golang.org/x/net/bpf from github.com/mdlayher/genetlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/appc+ - golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal + golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal+ golang.org/x/net/http/httpproxy from tailscale.com/net/tshttpproxy golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpguts+ diff --git a/cmd/tsidp/depaware.txt b/cmd/tsidp/depaware.txt index 2c45030a8..da2a4b867 100644 --- a/cmd/tsidp/depaware.txt +++ b/cmd/tsidp/depaware.txt @@ -352,7 +352,7 @@ tailscale.com/cmd/tsidp dependencies: (generated by github.com/tailscale/depawar golang.org/x/exp/maps from tailscale.com/ipn/store/mem+ golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/appc+ - golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal + golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal+ golang.org/x/net/http/httpproxy from tailscale.com/net/tshttpproxy golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpguts+ diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go index 1f97237d7..dfa0d1b52 100644 --- a/derp/derphttp/derphttp_client.go +++ b/derp/derphttp/derphttp_client.go @@ -29,6 +29,7 @@ import ( "time" "go4.org/mem" + "golang.org/x/net/http/httpguts" "tailscale.com/derp" "tailscale.com/derp/derpconst" "tailscale.com/envknob" @@ -845,6 +846,16 @@ func firstStr(a, b string) string { // dialNodeUsingProxy connects to n using a CONNECT to the HTTP(s) proxy in proxyURL. func (c *Client) dialNodeUsingProxy(ctx context.Context, n *tailcfg.DERPNode, proxyURL *url.URL) (_ net.Conn, err error) { + // n.HostName comes from the control-supplied DERP map and is written + // verbatim into the CONNECT request line and Host header below. Reject + // anything that isn't a valid host value so a hostname carrying CR/LF (or + // other control bytes) can't inject extra headers or a second request into + // the proxy connection. ValidHostHeader accepts the empty string, so check + // for that separately. + if n.HostName == "" || !httpguts.ValidHostHeader(n.HostName) { + return nil, fmt.Errorf("derphttp: invalid DERP node hostname %q", n.HostName) + } + pu := proxyURL var proxyConn net.Conn if pu.Scheme == "https" { diff --git a/derp/derphttp/derphttp_proxy_test.go b/derp/derphttp/derphttp_proxy_test.go new file mode 100644 index 000000000..038aebd24 --- /dev/null +++ b/derp/derphttp/derphttp_proxy_test.go @@ -0,0 +1,101 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +package derphttp + +import ( + "bufio" + "context" + "io" + "net" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "tailscale.com/net/netmon" + "tailscale.com/tailcfg" + "tailscale.com/types/key" +) + +// TestDialNodeUsingProxyHostname checks which control-supplied DERP hostnames +// dialNodeUsingProxy is willing to write into the proxy CONNECT request. A +// rejected hostname must fail before anything is sent to the proxy; an accepted +// one must show up as the CONNECT target. +func TestDialNodeUsingProxyHostname(t *testing.T) { + tests := []struct { + name string + hostname string + wantRej bool + wantTarget string // CONNECT target seen by the proxy, if !wantRej + }{ + {"crlf_injection", "127.0.0.1\r\nX-Injected: 1", true, ""}, + {"lf_only", "127.0.0.1\nX-Injected: 1", true, ""}, + {"empty", "", true, ""}, + {"dns_name", "derp1.example.com", false, "derp1.example.com:443"}, + {"ipv4", "127.0.0.1", false, "127.0.0.1:443"}, + {"ipv6", "fe80::1", false, "[fe80::1]:443"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Fake HTTP proxy that records the CONNECT target and replies 200. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + gotTarget := make(chan string, 1) + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer conn.Close() + req, err := http.ReadRequest(bufio.NewReader(conn)) + if err != nil { + return + } + gotTarget <- req.RequestURI + io.WriteString(conn, "HTTP/1.1 200 OK\r\n\r\n") + }() + + c := NewRegionClient(key.NewNode(), t.Logf, netmon.NewStatic(), + func() *tailcfg.DERPRegion { return nil }) + defer c.Close() + + n := &tailcfg.DERPNode{ + HostName: tt.hostname, + DERPPort: 443, + } + proxyURL := &url.URL{Scheme: "http", Host: ln.Addr().String()} + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + conn, err := c.dialNodeUsingProxy(ctx, n, proxyURL) + if tt.wantRej { + if err == nil { + conn.Close() + t.Fatalf("dialNodeUsingProxy accepted hostname %q", tt.hostname) + } + if !strings.Contains(err.Error(), "invalid DERP node hostname") { + t.Fatalf("got error %v, want an invalid-hostname error", err) + } + return + } + if err != nil { + t.Fatalf("dialNodeUsingProxy(%q): %v", tt.hostname, err) + } + defer conn.Close() + select { + case got := <-gotTarget: + if got != tt.wantTarget { + t.Errorf("CONNECT target = %q, want %q", got, tt.wantTarget) + } + case <-ctx.Done(): + t.Fatal("timed out waiting for CONNECT request") + } + }) + } +} diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index 03b508063..ec962ab08 100644 --- a/tsnet/depaware.txt +++ b/tsnet/depaware.txt @@ -345,7 +345,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware) golang.org/x/exp/maps from tailscale.com/ipn/store/mem+ golang.org/x/net/bpf from github.com/mdlayher/netlink+ golang.org/x/net/dns/dnsmessage from tailscale.com/appc+ - golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal + golang.org/x/net/http/httpguts from tailscale.com/ipn/ipnlocal+ golang.org/x/net/http/httpproxy from tailscale.com/net/tshttpproxy golang.org/x/net/icmp from tailscale.com/net/ping golang.org/x/net/idna from golang.org/x/net/http/httpguts+