diff --git a/oauthproxy.go b/oauthproxy.go index 587215fd2a..a75fb3c27f 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -57,6 +57,10 @@ var SignatureHeaders = []string{ var ( // ErrNeedsLogin means the user should be redirected to the login page ErrNeedsLogin = errors.New("redirect to login page") + + // Used to check final redirects are not susceptible to open redirects. + // Matches //, /\ and both of these with whitespace in between (eg / / or / \). + invalidRedirectRegex = regexp.MustCompile(`^/(\s|\v)?(/|\\)`) ) // OAuthProxy is the main authentication proxy @@ -578,7 +582,7 @@ func validOptionalPort(port string) bool { // IsValidRedirect checks whether the redirect URL is whitelisted func (p *OAuthProxy) IsValidRedirect(redirect string) bool { switch { - case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !strings.HasPrefix(redirect, "/\\"): + case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !invalidRedirectRegex.MatchString(redirect): return true case strings.HasPrefix(redirect, "http://") || strings.HasPrefix(redirect, "https://"): redirectURL, err := url.Parse(redirect) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 957b4334a3..a216f2cb8f 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -322,6 +322,61 @@ func TestIsValidRedirect(t *testing.T) { Redirect: "http://a.sub.anyport.bar:8081/redirect", ExpectedResult: true, }, + { + Desc: "openRedirect1", + Redirect: "/\\evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectSpace1", + Redirect: "/ /evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectSpace2", + Redirect: "/ \\evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectTab1", + Redirect: "/\t/evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectTab2", + Redirect: "/\t\\evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectVerticalTab1", + Redirect: "/\v/evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectVerticalTab2", + Redirect: "/\v\\evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectNewLine1", + Redirect: "/\n/evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectNewLine2", + Redirect: "/\n\\evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectCarriageReturn1", + Redirect: "/\r/evil.com", + ExpectedResult: false, + }, + { + Desc: "openRedirectCarriageReturn2", + Redirect: "/\r\\evil.com", + ExpectedResult: false, + }, } for _, tc := range testCases {