tsweb: add a helper to validate redirect URLs

We issue redirects in a few different places, it's time to have
a common helper to do target validation.

Updates tailscale/corp#16875

Signed-off-by: David Anderson <[email protected]>

tsweb/tsweb.go

@@ -15,6 +15,7 @@ import (
 	"net/http"
 	_ "net/http/pprof"
 	"net/netip"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -447,6 +448,74 @@ func VarzHandler(w http.ResponseWriter, r *http.Request) {
 	varz.Handler(w, r)
 }
 
+// CleanRedirectURL ensures that urlStr is a valid redirect URL to the
+// current server, or one of allowedHosts. Returns the cleaned URL or
+// a validation error.
+func CleanRedirectURL(urlStr string, allowedHosts []string) (*url.URL, error) {
+	// In some places, we unfortunately query-escape the redirect URL
+	// too many times, and end up needing to redirect to a URL that's
+	// still escaped by one level. Try to unescape the input.
+	unescaped, err := url.QueryUnescape(urlStr)
+	if err == nil && unescaped != urlStr {
+		urlStr = unescaped
+	}
+
+	// Go's URL parser and browser URL parsers disagree on the meaning
+	// of malformed HTTP URLs. Given the input https:/evil.com, Go
+	// parses it as hostname="", path="/evil.com". Browsers parse it
+	// as hostname="evil.com", path="". This means that, using
+	// malformed URLs, an attacker could trick us into approving of a
+	// "local" redirect that in fact sends people elsewhere.
+	//
+	// This very blunt check enforces that we'll only process
+	// redirects that are definitely well-formed URLs.
+	//
+	// Note that the check for just / also allows URLs of the form
+	// "//foo.com/bar", which are scheme-relative redirects. These
+	// must be handled with care below when determining whether a
+	// redirect is relative to the current host. Notably,
+	// url.URL.IsAbs reports // URLs as relative, whereas we want to
+	// treat them as absolute redirects and verify the target host.
+	if !hasSafeRedirectPrefix(urlStr) {
+		return nil, fmt.Errorf("invalid redirect URL %q", urlStr)
+	}
+
+	url, err := url.Parse(urlStr)
+	if err != nil {
+		return nil, fmt.Errorf("invalid redirect URL %q: %w", urlStr, err)
+	}
+	// Redirects to self are always allowed. A self redirect must
+	// start with url.Path, all prior URL sections must be empty.
+	isSelfRedirect := url.Scheme == "" && url.Opaque == "" && url.User == nil && url.Host == ""
+	if isSelfRedirect {
+		return url, nil
+	}
+	for _, allowed := range allowedHosts {
+		if strings.EqualFold(allowed, url.Hostname()) {
+			return url, nil
+		}
+	}
+
+	return nil, fmt.Errorf("disallowed target host %q in redirect URL %q", url.Hostname(), urlStr)
+}
+
+// hasSafeRedirectPrefix reports whether url starts with a slash, or
+// one of the case-insensitive strings "http://" or "https://".
+func hasSafeRedirectPrefix(url string) bool {
+	if len(url) >= 1 && url[0] == '/' {
+		return true
+	}
+	const http = "http://"
+	if len(url) >= len(http) && strings.EqualFold(url[:len(http)], http) {
+		return true
+	}
+	const https = "https://"
+	if len(url) >= len(https) && strings.EqualFold(url[:len(https)], https) {
+		return true
+	}
+	return false
+}
+
 // AddBrowserHeaders sets various HTTP security headers for browser-facing endpoints.
 //
 // The specific headers:

tsweb/tsweb_test.go

@@ -617,3 +617,54 @@ func TestPort80Handler(t *testing.T) {
 		})
 	}
 }
+
+func TestCleanRedirectURL(t *testing.T) {
+	tailscaleHost := []string{"tailscale.com"}
+	tailscaleAndOtherHost := []string{"microsoft.com", "tailscale.com"}
+	localHost := []string{"127.0.0.1", "localhost"}
+	myServer := []string{"myserver"}
+	cases := []struct {
+		url   string
+		hosts []string
+		want  string
+	}{
+		{"http://tailscale.com/foo", tailscaleHost, "http://tailscale.com/foo"},
+		{"http://tailscale.com/foo", tailscaleAndOtherHost, "http://tailscale.com/foo"},
+		{"http://microsoft.com/foo", tailscaleAndOtherHost, "http://microsoft.com/foo"},
+		{"https://tailscale.com/foo", tailscaleHost, "https://tailscale.com/foo"},
+		{"/foo", tailscaleHost, "/foo"},
+		{"//tailscale.com/foo", tailscaleHost, "//tailscale.com/foo"},
+
+		{"/a/foobar", tailscaleHost, "/a/foobar"},
+		{"http://127.0.0.1/a/foobar", localHost, "http://127.0.0.1/a/foobar"},
+		{"http://127.0.0.1:123/a/foobar", localHost, "http://127.0.0.1:123/a/foobar"},
+		{"http://127.0.0.1:31544/a/foobar", localHost, "http://127.0.0.1:31544/a/foobar"},
+		{"http://localhost/a/foobar", localHost, "http://localhost/a/foobar"},
+		{"http://localhost:123/a/foobar", localHost, "http://localhost:123/a/foobar"},
+		{"http://localhost:31544/a/foobar", localHost, "http://localhost:31544/a/foobar"},
+		{"http://myserver/a/foobar", myServer, "http://myserver/a/foobar"},
+		{"http://myserver:123/a/foobar", myServer, "http://myserver:123/a/foobar"},
+		{"http://myserver:31544/a/foobar", myServer, "http://myserver:31544/a/foobar"},
+		{"http://evil.com/foo", tailscaleHost, ""},
+		{"//evil.com", tailscaleHost, ""},
+		{"HttP://tailscale.com", tailscaleHost, "http://tailscale.com"},
+		{"http://TaIlScAlE.CoM/spongebob", tailscaleHost, "http://TaIlScAlE.CoM/spongebob"},
+		{"ftp://tailscale.com", tailscaleHost, ""},
+		{"https:/evil.com", tailscaleHost, ""},                    // regression test for tailscale/corp#892
+		{"%2Fa%2F44869c061701", tailscaleHost, "/a/44869c061701"}, // regression test for tailscale/corp#13288
+		{"https%3A%2Ftailscale.com", tailscaleHost, ""},           // escaped colon-single-slash malformed URL
+	}
+
+	for _, tc := range cases {
+		gotURL, err := CleanRedirectURL(tc.url, tc.hosts)
+		if err != nil {
+			if tc.want != "" {
+				t.Errorf("CleanRedirectURL(%q, %v) got error: %v", tc.url, tc.hosts, err)
+			}
+		} else {
+			if got := gotURL.String(); got != tc.want {
+				t.Errorf("CleanRedirectURL(%q, %v) = %q, want %q", tc.url, tc.hosts, got, tc.want)
+			}
+		}
+	}
+}