From 0d5fa211df8ef2449347a56b22c779eb8d894c43 Mon Sep 17 00:00:00 2001
From: Joel Speed <Joel.speed@hotmail.co.uk>
Date: Wed, 6 May 2020 12:42:02 +0100
Subject: [PATCH] Merge pull request from GHSA-j7px-6hwj-hpjg
---
oauthproxy.go | 6 ++++-
oauthproxy_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/oauthproxy.go b/oauthproxy.go
index 587215f..a75fb3c 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 957b433..a216f2c 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 {
--
GitLab