From 7a01be9ae44e950b6bdfd9608e96912d29e19417 Mon Sep 17 00:00:00 2001 From: Jordan Webb Date: Sat, 5 Nov 2022 18:27:28 -0500 Subject: [PATCH] Validate redirect domain (#13) * Validate redirect domain This change introduces a validation step prior to redirect as discussed in #77 * Fix tests * Try harder to make CodeQL happy * Fix tests * Try just a little bit harder to appease CodeQL Co-authored-by: Thom Seddon --- internal/auth.go | 33 ++++++++++++++- internal/auth_test.go | 90 +++++++++++++++++++++++++++++++++++++++++ internal/server.go | 12 +++++- internal/server_test.go | 16 ++++---- 4 files changed, 141 insertions(+), 10 deletions(-) diff --git a/internal/auth.go b/internal/auth.go index 4bd8b292..7e1f6f0b 100644 --- a/internal/auth.go +++ b/internal/auth.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strconv" "strings" "time" @@ -122,9 +123,39 @@ func ValidateDomains(user string, domains CommaSeparatedList) bool { return false } +// ValidateRedirect validates that the given redirect is valid and permitted for +// the given request +func ValidateRedirect(r *http.Request, redirect string) (*url.URL, error) { + redirectURL, err := url.Parse(redirect) + + if err != nil { + return nil, errors.New("Unable to parse redirect") + } + + if redirectURL.Scheme != "http" && redirectURL.Scheme != "https" { + return nil, errors.New("Invalid redirect URL scheme") + } + + // If we're using an auth domain? + if use, base := useAuthDomain(r); use { + // If we are using an auth domain, they redirect must share a common + // suffix with the requested redirect + if !strings.HasSuffix(redirectURL.Host, base) { + return nil, errors.New("Redirect host does not match any expected hosts (should match cookie domain when using auth host)") + } + } else { + // If not, we should only ever redirect to the same domain + if redirectURL.Host != r.Header.Get("X-Forwarded-Host") { + return nil, errors.New("Redirect host does not match request host (must match when not using auth host)") + } + } + + return redirectURL, nil +} + // Utility methods -// Get the redirect base +// Get the request base from forwarded request func redirectBase(r *http.Request) string { return fmt.Sprintf("%s://%s", r.Header.Get("X-Forwarded-Proto"), r.Host) } diff --git a/internal/auth_test.go b/internal/auth_test.go index 26bd1b8c..3d058941 100644 --- a/internal/auth_test.go +++ b/internal/auth_test.go @@ -215,6 +215,96 @@ func TestAuthValidateUser(t *testing.T) { assert.True(v, "should allow user in whitelist") } +func TestAuthValidateRedirect(t *testing.T) { + assert := assert.New(t) + config, _ = NewConfig([]string{}) + + newRedirectRequest := func(urlStr string) *http.Request { + u, err := url.Parse(urlStr) + assert.Nil(err) + + r, _ := http.NewRequest("GET", urlStr, nil) + r.Header.Add("X-Forwarded-Proto", u.Scheme) + r.Header.Add("X-Forwarded-Host", u.Host) + r.Header.Add("X-Forwarded-Uri", u.RequestURI()) + + return r + } + + errStr := "Redirect host does not match request host (must match when not using auth host)" + + _, err := ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://app.example.com.bad.com", + ) + if assert.Error(err) { + assert.Equal(errStr, err.Error(), "Should not allow redirect to subdomain") + } + + _, err = ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://app.example.combad.com", + ) + if assert.Error(err) { + assert.Equal(errStr, err.Error(), "Should not allow redirect to overlapping domain") + } + + _, err = ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://example.com", + ) + if assert.Error(err) { + assert.Equal(errStr, err.Error(), "Should not allow redirect from subdomain") + } + + _, err = ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://app.example.com/profile", + ) + assert.Nil(err, "Should allow same domain") + + // + // With Auth Host + // + config.AuthHost = "auth.example.com" + config.CookieDomains = []CookieDomain{*NewCookieDomain("example.com")} + errStr = "Redirect host does not match any expected hosts (should match cookie domain when using auth host)" + + _, err = ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://app.example.com.bad.com", + ) + if assert.Error(err) { + assert.Equal(errStr, err.Error(), "Should not allow redirect to subdomain") + } + + _, err = ValidateRedirect( + newRedirectRequest("http://app.example.com/_oauth?state=123"), + "http://app.example.combad.com", + ) + if assert.Error(err) { + assert.Equal(errStr, err.Error(), "Should not allow redirect to overlapping domain") + } + + _, err = ValidateRedirect( + newRedirectRequest("http://auth.example.com/_oauth?state=123"), + "http://app.example.com/profile", + ) + assert.Nil(err, "Should allow between subdomains when using auth host") + + _, err = ValidateRedirect( + newRedirectRequest("http://auth.example.com/_oauth?state=123"), + "http://auth.example.com/profile", + ) + assert.Nil(err, "Should allow same domain when using auth host") + + _, err = ValidateRedirect( + newRedirectRequest("http://auth.example.com/_oauth?state=123"), + "http://example.com/profile", + ) + assert.Nil(err, "Should allow from subdomain when using auth host") +} + func TestRedirectUri(t *testing.T) { assert := assert.New(t) diff --git a/internal/server.go b/internal/server.go index fceb586c..1fb9d84d 100644 --- a/internal/server.go +++ b/internal/server.go @@ -190,6 +190,16 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc { // Clear CSRF cookie http.SetCookie(w, ClearCSRFCookie(r, c)) + // Validate redirect + redirectURL, err := ValidateRedirect(r, redirect) + if err != nil { + logger.WithFields(logrus.Fields{ + "receieved_redirect": redirect, + }).Warnf("Invalid redirect in CSRF. %v", err) + http.Error(w, "Not authorized", 401) + return + } + // Exchange code for token token, err := p.ExchangeCode(redirectUri(r), r.URL.Query().Get("code")) if err != nil { @@ -215,7 +225,7 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc { }).Info("Successfully generated auth cookie, redirecting user.") // Redirect - http.Redirect(w, r, redirect, http.StatusTemporaryRedirect) + http.Redirect(w, r, redirectURL.String(), http.StatusTemporaryRedirect) } } diff --git a/internal/server_test.go b/internal/server_test.go index b1bfb734..f4c94fcf 100644 --- a/internal/server_test.go +++ b/internal/server_test.go @@ -222,26 +222,26 @@ func TestServerAuthCallback(t *testing.T) { // Should catch invalid csrf cookie nonce := "12345678901234567890123456789012" - req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":http://redirect") + req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":http://example.com") c := MakeCSRFCookie(req, "nononononononononononononononono") res, _ = doHttpRequest(req, c) assert.Equal(401, res.StatusCode, "auth callback with invalid cookie shouldn't be authorised") // Should catch invalid provider cookie - req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":invalid:http://redirect") + req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":invalid:http://example.com") c = MakeCSRFCookie(req, nonce) res, _ = doHttpRequest(req, c) assert.Equal(401, res.StatusCode, "auth callback with invalid provider shouldn't be authorised") // Should redirect valid request - req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":google:http://redirect") + req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":google:http://example.com") c = MakeCSRFCookie(req, nonce) res, _ = doHttpRequest(req, c) require.Equal(307, res.StatusCode, "valid auth callback should be allowed") fwd, _ := res.Location() assert.Equal("http", fwd.Scheme, "valid request should be redirected to return url") - assert.Equal("redirect", fwd.Host, "valid request should be redirected to return url") + assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url") assert.Equal("", fwd.Path, "valid request should be redirected to return url") } @@ -264,7 +264,7 @@ func TestServerAuthCallbackExchangeFailure(t *testing.T) { } // Should handle failed code exchange - req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://redirect") + req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://example.com") c := MakeCSRFCookie(req, "12345678901234567890123456789012") res, _ := doHttpRequest(req, c) assert.Equal(503, res.StatusCode, "auth callback should handle failed code exchange") @@ -291,7 +291,7 @@ func TestServerAuthCallbackUserFailure(t *testing.T) { } // Should handle failed user request - req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://redirect") + req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://example.com") c := MakeCSRFCookie(req, "12345678901234567890123456789012") res, _ := doHttpRequest(req, c) assert.Equal(503, res.StatusCode, "auth callback should handle failed user request") @@ -317,7 +317,7 @@ func TestServerLogout(t *testing.T) { require.Less(cookie.Expires.Local().Unix(), time.Now().Local().Unix()-50, "cookie should have expired") // Test with redirect - config.LogoutRedirect = "http://redirect/path" + config.LogoutRedirect = "http://example.com/path" req = newDefaultHttpRequest("/_oauth/logout") res, _ = doHttpRequest(req, nil) require.Equal(307, res.StatusCode, "should return a 307") @@ -335,7 +335,7 @@ func TestServerLogout(t *testing.T) { fwd, _ := res.Location() require.NotNil(fwd) assert.Equal("http", fwd.Scheme, "valid request should be redirected to return url") - assert.Equal("redirect", fwd.Host, "valid request should be redirected to return url") + assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url") assert.Equal("/path", fwd.Path, "valid request should be redirected to return url") }