Skip to content

OCPBUGS-115304:[release-5.0] bugfix: rewrite open redirect strings to '/' #376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
everettraven wants to merge 1 commit into
base: release-5.0
Choose a base branch
from
+180 −10
Open

OCPBUGS-115304:[release-5.0] bugfix: rewrite open redirect strings to '/' #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions oauthproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,13 +522,40 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)
}

redirect = req.Form.Get("rd")
if redirect == "" || !strings.HasPrefix(redirect, "/") || strings.HasPrefix(redirect, "//") {
if !isValidRedirect(redirect) {
redirect = "/"
}

return
}

var (
// 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]*|\.{1,2})[/\\]`)
)

// isValidRedirect replicates the behavior of the upstream oauth2-proxy redirect validation
// in https://github.com/oauth2-proxy/oauth2-proxy/blob/c5529b42b7ebf2bf098b4b139f40fcea9a57bffd/pkg/app/redirect/validator.go#L1-L66
// which appropriately ensures that redirects are not susceptible to open redirect attacks.
// It does not copy over the domain whitelisting behavior because it did not already exist within this earlier
// fork of the project.
// We also do not allow actual URLs because this fork of the proxy is only ever intended to be run as a sidecar
// to the actual upstream application it is acting as a proxy for.
// We should never have the use case that resulted in allowing non-local redirects in the upstream oauth2-proxy project.
// (reference: https://github.com/oauth2-proxy/oauth2-proxy/pull/15)
func isValidRedirect(redirect string) bool {
if redirect == "" {
return false
}

if strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !invalidRedirectRegex.MatchString(redirect) {
return true
}

return false
}

func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) (ok bool) {
isPreflightRequestAllowed := p.skipAuthPreflight && req.Method == "OPTIONS"
return isPreflightRequestAllowed || (!p.IsProtectedPath(req.URL.Path) || p.IsWhitelistedPath(req.URL.Path))
Expand Down Expand Up @@ -682,7 +709,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
return
}

if !strings.HasPrefix(redirect, "/") || strings.HasPrefix(redirect, "//") {
if !isValidRedirect(redirect) {
redirect = "/"
}

Expand Down
159 changes: 151 additions & 8 deletions oauthproxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

func init() {
log.SetFlags(log.Ldate | log.Ltime | log.Lshortfile)

}

type WebSocketOrRestHandler struct {
Expand Down Expand Up @@ -87,7 +86,7 @@ func TestWebSocketProxy(t *testing.T) {
if err != nil {
t.Fatalf("err %s", err)
}
var response = make([]byte, 1024)
response := make([]byte, 1024)
websocket.Message.Receive(ws, &response)
if err != nil {
t.Fatalf("err %s", err)
Expand Down Expand Up @@ -416,7 +415,8 @@ func (pat_test *PassAccessTokenTest) Close() {
}

func (pat_test *PassAccessTokenTest) getCallbackEndpoint() (http_code int,
cookie string) {
cookie string,
) {
rw := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/oauth2/callback?code=callback_code&state=nonce:",
strings.NewReader(""))
Expand Down Expand Up @@ -714,7 +714,8 @@ func NewAuthOnlyEndpointTest() *ProcessCookieTest {
func TestAuthOnlyEndpointAccepted(t *testing.T) {
test := NewAuthOnlyEndpointTest()
startSession := &providers.SessionState{
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token"}
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token",
}
test.SaveSession(startSession, time.Now())

test.proxy.ServeHTTP(test.rw, test.req)
Expand All @@ -737,7 +738,8 @@ func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) {
test.proxy.CookieExpire = time.Duration(24) * time.Hour
reference := time.Now().Add(time.Duration(25) * time.Hour * -1)
startSession := &providers.SessionState{
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token"}
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token",
}
test.SaveSession(startSession, reference)

test.proxy.ServeHTTP(test.rw, test.req)
Expand All @@ -749,7 +751,8 @@ func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) {
func TestAuthOnlyEndpointUnauthorizedOnEmailValidationFailure(t *testing.T) {
test := NewAuthOnlyEndpointTest()
startSession := &providers.SessionState{
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token"}
Email: "michael.bland@gsa.gov", AccessToken: "my_access_token",
}
test.SaveSession(startSession, time.Now())
test.validate_user = false

Expand Down Expand Up @@ -779,7 +782,8 @@ func TestAuthOnlyEndpointSetXAuthRequestHeaders(t *testing.T) {
pc_test.opts.ProxyPrefix+"/auth", nil)

startSession := &providers.SessionState{
User: "oauth_user", Email: "oauth_user@example.com", AccessToken: "oauth_token"}
User: "oauth_user", Email: "oauth_user@example.com", AccessToken: "oauth_token",
}
pc_test.SaveSession(startSession, time.Now())

pc_test.proxy.ServeHTTP(pc_test.rw, pc_test.req)
Expand Down Expand Up @@ -915,7 +919,8 @@ func (st *SignatureTest) MakeRequestWithExpectedKey(method, body, key string) {
req.Header = st.header

state := &providers.SessionState{
Email: "mbland@acm.org", AccessToken: "my_access_token"}
Email: "mbland@acm.org", AccessToken: "my_access_token",
}
value, err := proxy.provider.CookieForSession(state, proxy.CookieCipher)
if err != nil {
panic(err)
Expand Down Expand Up @@ -1175,3 +1180,141 @@ func Test_stripNormalizedHeader(t *testing.T) {
})
}
}

const redirectFormKey = "rd"

func TestGetRedirect(t *testing.T) {
type testcase struct {
name string
redirect string
expectedRedirect string
}

testcases := []testcase{
{
name: "non-empty redirect starting with a '/' returns the full redirect string",
redirect: "/test",
expectedRedirect: "/test",
},
{
name: "non-empty redirect starting with HTTP scheme returns '/'",
redirect: "http://google.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with HTTPS scheme returns '/'",
redirect: "https://google.com",
expectedRedirect: "/",
},
{
name: "empty redirect returns '/'",
redirect: "",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with a '//' returns '/'",
redirect: "//test",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with an escaped back slash returns '/'",
redirect: "/\\evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with space between slash returns '/'",
redirect: "/ /evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with space before escaped back slash returns '/'",
redirect: "/ \\evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with tab between forward slash returns '/'",
redirect: "/\t/evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with tab between escaped back slash returns '/'",
redirect: "/\t\\evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with vertical tab between forward slash returns '/'",
redirect: "/\v/evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with vertical tab between escaped back slash returns '/'",
redirect: "/\v\\evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with newline between forward slash returns '/'",
redirect: "/\n/evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with newline between escaped back slash returns '/'",
redirect: "/\n\\evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with carriage return between forward slash returns '/'",
redirect: "/\r/evil.com",
expectedRedirect: "/",
},
{
name: "non-empty redirect starting with carriage return between escaped back slash returns '/'",
redirect: "/\r\\evil.com",
expectedRedirect: "/",
},
{
name: "double tabs",
redirect: "/\t/\t\\evil.com",
expectedRedirect: "/",
},
{
name: "triple tabs",
redirect: "/\t\t/\t/evil.com",
expectedRedirect: "/",
},
{
name: "triple tabs with escaped back slash",
redirect: "/\t\t\\\t/evil.com",
expectedRedirect: "/",
},
{
name: "relative path",
redirect: "/./\\evil.com",
expectedRedirect: "/",
},
{
name: "relative subpath",
redirect: "/./../../\\evil.com",
expectedRedirect: "/",
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
req := &http.Request{
Form: url.Values{
redirectFormKey: []string{tc.redirect},
},
}

oap := &OAuthProxy{}
actual, err := oap.GetRedirect(req)
if err != nil {
t.Fatalf("received an unexpected error: %v", err)
}

if actual != tc.expectedRedirect {
t.Fatalf("expected redirect value of %q but received %q", tc.expectedRedirect, actual)
}
})
}
}