From f5ff202f8135540debc9dd95cbce1e6704d9b75e Mon Sep 17 00:00:00 2001 From: Jean Prat Date: Tue, 15 Oct 2019 17:40:18 +0200 Subject: [PATCH 1/3] The host given in state parameter cannot be different from the host calling ServerAuthCallback --- internal/server_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/server_test.go b/internal/server_test.go index e62267c4..01bc6ef2 100644 --- a/internal/server_test.go +++ b/internal/server_test.go @@ -130,21 +130,21 @@ func TestServerAuthCallback(t *testing.T) { assert.Equal(401, res.StatusCode, "auth callback without cookie shouldn't be authorised") // Should catch invalid csrf cookie - req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://redirect") + req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012: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 redirect valid request - req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://redirect") + req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://example.com/redirect") c = MakeCSRFCookie(req, "12345678901234567890123456789012") res, _ = doHttpRequest(req, c) assert.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("", fwd.Path, "valid request should be redirected to return url") + assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url") + assert.Equal("/redirect", fwd.Path, "valid request should be redirected to return url") } func TestServerDefaultAction(t *testing.T) { From da61867fea3d5cde3e94e986dcce52286821e9ea Mon Sep 17 00:00:00 2001 From: Jean Prat Date: Tue, 15 Oct 2019 17:41:42 +0200 Subject: [PATCH 2/3] Possible open redirect fixed --- internal/server.go | 10 ++++++++++ internal/server_test.go | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/internal/server.go b/internal/server.go index 3119876b..8a86c11b 100644 --- a/internal/server.go +++ b/internal/server.go @@ -6,6 +6,8 @@ import ( "github.com/containous/traefik/pkg/rules" "github.com/sirupsen/logrus" + + "strings" ) type Server struct { @@ -146,6 +148,14 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc { return } + //Validate redirect + allowedHostForRedirect := redirectBase(r) + if !strings.HasPrefix(redirect+"/", allowedHostForRedirect+"/") { + logger.Errorf("Redirection %v does not match the redirectBase", redirect) + http.Error(w, "Not authorized", 401) + return + } + // Generate cookie http.SetCookie(w, MakeCookie(r, user.Email)) logger.WithFields(logrus.Fields{ diff --git a/internal/server_test.go b/internal/server_test.go index 01bc6ef2..aa455371 100644 --- a/internal/server_test.go +++ b/internal/server_test.go @@ -145,6 +145,16 @@ func TestServerAuthCallback(t *testing.T) { assert.Equal("http", fwd.Scheme, "valid request should be redirected to return url") assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url") assert.Equal("/redirect", fwd.Path, "valid request should be redirected to return url") + + req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://example.com.notallowed") + c = MakeCSRFCookie(req, "12345678901234567890123456789012") + res, _ = doHttpRequest(req, c) + assert.Equal(401, res.StatusCode, "redirect domain cannot differ from the host defined in the state parameter") + + req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://notallowed.example.com") + c = MakeCSRFCookie(req, "12345678901234567890123456789012") + res, _ = doHttpRequest(req, c) + assert.Equal(401, res.StatusCode, "subdomain cannot differ from the host defined in the state parameter") } func TestServerDefaultAction(t *testing.T) { @@ -355,6 +365,7 @@ func newHttpRequest(method, dest, uri string) *http.Request { p, _ := url.Parse(dest) r.Header.Add("X-Forwarded-Method", method) r.Header.Add("X-Forwarded-Host", p.Host) + r.Header.Add("X-Forwarded-Proto", p.Scheme) r.Header.Add("X-Forwarded-Uri", uri) return r } From 5d581503be9c68aa773a81d92b7e056a16e67123 Mon Sep 17 00:00:00 2001 From: Jean Prat Date: Tue, 15 Oct 2019 17:44:44 +0200 Subject: [PATCH 3/3] Naming --- internal/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/server.go b/internal/server.go index 8a86c11b..64f1987e 100644 --- a/internal/server.go +++ b/internal/server.go @@ -149,8 +149,8 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc { } //Validate redirect - allowedHostForRedirect := redirectBase(r) - if !strings.HasPrefix(redirect+"/", allowedHostForRedirect+"/") { + redirectBaseUseToForgeStatePrameter := redirectBase(r) + if !strings.HasPrefix(redirect+"/", redirectBaseUseToForgeStatePrameter+"/") { logger.Errorf("Redirection %v does not match the redirectBase", redirect) http.Error(w, "Not authorized", 401) return