From ae79b2e784c4cd72c40a0f2042a23e36c1c9a641 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 19 Jan 2024 14:15:27 -0800 Subject: [PATCH] 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 --- tsweb/tsweb.go | 69 +++++++++++++++++++++++++++++++++++++++++++++ tsweb/tsweb_test.go | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index 652f09692..c9d1c3a43 100644 --- a/tsweb/tsweb.go +++ b/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: diff --git a/tsweb/tsweb_test.go b/tsweb/tsweb_test.go index f943e3a94..9e7005cb0 100644 --- a/tsweb/tsweb_test.go +++ b/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) + } + } + } +}