Merge branch 'fix/open-redirect' of github.com:orvice/traefik-forward…

…-auth into orvice-fix/open-redirect

internal/auth.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -121,9 +122,35 @@ func ValidateDomains(email 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) error {
+	redirectURL, err := url.Parse(redirect)
+
+	if err != nil {
+		return errors.New("Unable to parse redirect")
+	}
+
+	// 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 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 errors.New("Redirect host does not match request host (must match when not using auth host)")
+		}
+	}
+
+	return 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)
 }

internal/auth_test.go

@@ -194,6 +194,96 @@ func TestAuthValidateEmail(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)
 

internal/server.go

@@ -115,7 +115,7 @@ func (s *Server) AuthHandler(providerName, rule string) http.HandlerFunc {
 				// Clearing the cookie will allow the user to try another email address and avoid being trapped on 'Not authorized'
 				http.SetCookie(w, ClearCookie(r))
 				http.Error(w, "Not authorized (Refresh to try again with a different email address)", 401)
-			}else {
+			} else {
 				http.Error(w, "Not authorized", 401)
 			}
 			return
@@ -178,6 +178,16 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc {
 		// Clear CSRF cookie
 		http.SetCookie(w, ClearCSRFCookie(r, c))
 
+		// Validate redirect
+		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 {

internal/server_test.go

@@ -205,7 +205,7 @@ func TestServerAuthCallback(t *testing.T) {
 
 	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")
 }