mirror of
https://github.com/tailscale/tailscale.git
synced 2026-10-09 03:42:01 -04:00
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 <basavaraj@digiscrypt.com> Co-authored-by: Mike Jensen <mikej@tailscale.com> Signed-off-by: Mike Jensen <mikej@tailscale.com>
This commit is contained in:
1 parent
ecedffff5b
commit
a40d145b21
9 files changed
+120
-6
No files matched your search
@@ -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+
|
||||
|
||||
@@ -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+
|
||||
|
||||
@@ -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+
|
||||
|
||||
@@ -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+
|
||||
|
||||
@@ -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+
|
||||
|
||||
@@ -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+
|
||||
|
||||
@@ -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" {
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
+1
-1
@@ -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+
|
||||
|
||||
Reference in new issue
Block a user