From 6b956b49e029ecc500bfae96a3175a6b4db4811d Mon Sep 17 00:00:00 2001 From: Will Norris Date: Thu, 2 Nov 2023 20:05:40 -0700 Subject: [PATCH] client/web: add some security checks for full client Require that requests to servers in manage mode are made to the Tailscale IP (either ipv4 or ipv6) or quad-100. Also set various security headers on those responses. These might be too restrictive, but we can relax them as needed. Allow requests to /ok (even in manage mode) with no checks. This will be used for the connectivity check from a login client to see if the management client is reachable. Updates tailscale/corp#14335 Signed-off-by: Will Norris --- client/web/web.go | 62 +++++++++++++++++++++ client/web/web_test.go | 95 ++++++++++++++++++++++++++++++++- ipn/ipnlocal/local.go | 4 +- ipn/ipnlocal/web_client.go | 2 + ipn/ipnlocal/web_client_stub.go | 2 + 5 files changed, 161 insertions(+), 4 deletions(-) diff --git a/client/web/web.go b/client/web/web.go index a66bd7245..518bc151c 100644 --- a/client/web/web.go +++ b/client/web/web.go @@ -29,12 +29,17 @@ import ( "tailscale.com/ipn/ipnstate" "tailscale.com/licenses" "tailscale.com/net/netutil" + "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" "tailscale.com/types/logger" "tailscale.com/util/httpm" "tailscale.com/version/distro" ) +// ListenPort is the static port used for the web client when run inside tailscaled. +// (5252 are the numbers above the letters "TSTS" on a qwerty keyboard.) +const ListenPort = 5252 + // Server is the backend server for a Tailscale web client. type Server struct { mode ServerMode @@ -202,6 +207,24 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func (s *Server) serve(w http.ResponseWriter, r *http.Request) { + if s.mode == ManageServerMode { + // In manage mode, requests must be sent directly to the bare Tailscale IP address. + // If a request comes in on any other hostname, redirect. + if s.requireTailscaleIP(w, r) { + return // user was redirected + } + + // serve HTTP 204 on /ok requests as connectivity check + if r.Method == httpm.GET && r.URL.Path == "/ok" { + w.WriteHeader(http.StatusNoContent) + return + } + + w.Header().Set("X-Frame-Options", "DENY") + w.Header().Set("Content-Security-Policy", "default-src 'self'") + w.Header().Set("Cross-Origin-Resource-Policy", "same-origin") + } + if strings.HasPrefix(r.URL.Path, "/api/") { switch { case r.URL.Path == "/api/auth" && r.Method == httpm.GET: @@ -228,6 +251,45 @@ func (s *Server) serve(w http.ResponseWriter, r *http.Request) { s.assetsHandler.ServeHTTP(w, r) } +// requireTailscaleIP redirects an incoming request if the HTTP request was not made to a bare Tailscale IP address. +// The request will be redirected to the Tailscale IP, port 5252, with the original request path. +// This allows any custom hostname to be used to access the device, but protects against DNS rebinding attacks. +// Returns true if the request has been fully handled, either be returning a redirect or an HTTP error. +func (s *Server) requireTailscaleIP(w http.ResponseWriter, r *http.Request) (handled bool) { + const ( + ipv4ServiceHost = tsaddr.TailscaleServiceIPString + ipv6ServiceHost = "[" + tsaddr.TailscaleServiceIPv6String + "]" + ) + // allow requests on quad-100 (or ipv6 equivalent) + if r.URL.Host == ipv4ServiceHost || r.URL.Host == ipv6ServiceHost { + return false + } + + st, err := s.lc.StatusWithoutPeers(r.Context()) + if err != nil { + s.logf("error getting status: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return true + } + + var ipv4 string // store the first IPv4 address we see for redirect later + for _, ip := range st.Self.TailscaleIPs { + if ip.Is4() { + if r.Host == fmt.Sprintf("%s:%d", ip, ListenPort) { + return false + } + ipv4 = ip.String() + } + if ip.Is6() && r.Host == fmt.Sprintf("[%s]:%d", ip, ListenPort) { + return false + } + } + newURL := *r.URL + newURL.Host = fmt.Sprintf("%s:%d", ipv4, ListenPort) + http.Redirect(w, r, newURL.String(), http.StatusMovedPermanently) + return true +} + // authorizeRequest reports whether the request from the web client // is authorized to be completed. // It reports true if the request is authorized, and false otherwise. diff --git a/client/web/web_test.go b/client/web/web_test.go index c19bb3b4d..648b66d9c 100644 --- a/client/web/web_test.go +++ b/client/web/web_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/netip" "net/url" "strings" "testing" @@ -410,7 +411,11 @@ func TestAuthorizeRequest(t *testing.T) { func TestServeAuth(t *testing.T) { user := &tailcfg.UserProfile{ID: tailcfg.UserID(1)} - self := &ipnstate.PeerStatus{ID: "self", UserID: user.ID} + self := &ipnstate.PeerStatus{ + ID: "self", + UserID: user.ID, + TailscaleIPs: []netip.Addr{netip.MustParseAddr("100.1.2.3")}, + } remoteNode := &apitype.WhoIsResponse{Node: &tailcfg.Node{ID: 1}, UserProfile: user} remoteIP := "100.100.100.101" @@ -605,7 +610,7 @@ func TestServeAuth(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - r := httptest.NewRequest("GET", tt.path, nil) + r := httptest.NewRequest("GET", "http://100.1.2.3:5252"+tt.path, nil) r.RemoteAddr = remoteIP r.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tt.cookie}) w := httptest.NewRecorder() @@ -663,6 +668,92 @@ func TestServeAuth(t *testing.T) { } } +func TestRequireTailscaleIP(t *testing.T) { + self := &ipnstate.PeerStatus{ + TailscaleIPs: []netip.Addr{ + netip.MustParseAddr("100.1.2.3"), + netip.MustParseAddr("fd7a:115c::1234"), + }, + } + + lal := memnet.Listen("local-tailscaled.sock:80") + defer lal.Close() + localapi := mockLocalAPI(t, nil, func() *ipnstate.PeerStatus { return self }) + defer localapi.Close() + go localapi.Serve(lal) + + s := &Server{ + mode: ManageServerMode, + lc: &tailscale.LocalClient{Dial: lal.Dial}, + timeNow: time.Now, + logf: t.Logf, + } + + tests := []struct { + name string + target string + wantHandled bool + wantLocation string + }{ + { + name: "localhost", + target: "http://localhost/", + wantHandled: true, + wantLocation: "http://100.1.2.3:5252/", + }, + { + name: "ipv4-no-port", + target: "http://100.1.2.3/", + wantHandled: true, + wantLocation: "http://100.1.2.3:5252/", + }, + { + name: "ipv4-correct-port", + target: "http://100.1.2.3:5252/", + wantHandled: false, + }, + { + name: "ipv6-no-port", + target: "http://[fd7a:115c::1234]/", + wantHandled: true, + wantLocation: "http://100.1.2.3:5252/", + }, + { + name: "ipv6-correct-port", + target: "http://[fd7a:115c::1234]:5252/", + wantHandled: false, + }, + { + name: "quad-100", + target: "http://100.100.100.100/", + wantHandled: false, + }, + { + name: "ipv6-service-addr", + target: "http://[fd7a:115c:a1e0::53]/", + wantHandled: false, + }, + } + + for _, tt := range tests { + t.Run(tt.target, func(t *testing.T) { + s.logf = t.Logf + r := httptest.NewRequest(httpm.GET, tt.target, nil) + w := httptest.NewRecorder() + handled := s.requireTailscaleIP(w, r) + + if handled != tt.wantHandled { + t.Errorf("request(%q) was handled; want=%v, got=%v", tt.target, tt.wantHandled, handled) + } + + location := w.Header().Get("Location") + if location != tt.wantLocation { + t.Errorf("request(%q) wrong location; want=%q, got=%q", tt.target, tt.wantLocation, location) + } + }) + } +} + var ( testControlURL = "http://localhost:8080" testAuthPath = "/a/12345" diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7180bb8a4..a0d8e6e43 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3132,7 +3132,7 @@ func (b *LocalBackend) TCPHandlerForDst(src, dst netip.AddrPort) (handler func(c return b.handleSSHConn, opts } // TODO(will,sonia): allow customizing web client port ? - if dst.Port() == 5252 && b.ShouldRunWebClient() { + if dst.Port() == webClientPort && b.ShouldRunWebClient() { return b.handleWebClientConn, opts } if port, ok := b.GetPeerAPIPort(dst.Addr()); ok && dst.Port() == port { @@ -4474,7 +4474,7 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. handlePorts = append(handlePorts, 22) } if b.ShouldRunWebClient() { - handlePorts = append(handlePorts, 5252) + handlePorts = append(handlePorts, webClientPort) } b.reloadServeConfigLocked(prefs) diff --git a/ipn/ipnlocal/web_client.go b/ipn/ipnlocal/web_client.go index 4b18019f7..4cb25174e 100644 --- a/ipn/ipnlocal/web_client.go +++ b/ipn/ipnlocal/web_client.go @@ -16,6 +16,8 @@ import ( "tailscale.com/net/netutil" ) +const webClientPort = web.ListenPort + // webClient holds state for the web interface for managing // this tailscale instance. The web interface is not used by // default, but initialized by calling LocalBackend.WebOrInit. diff --git a/ipn/ipnlocal/web_client_stub.go b/ipn/ipnlocal/web_client_stub.go index 2f593837c..b9e8ce919 100644 --- a/ipn/ipnlocal/web_client_stub.go +++ b/ipn/ipnlocal/web_client_stub.go @@ -12,6 +12,8 @@ import ( "tailscale.com/client/tailscale" ) +const webClientPort = 5252 + type webClient struct{} func (b *LocalBackend) SetWebLocalClient(lc *tailscale.LocalClient) {}