From a12bae35ca0a19e6571ca4ca4ea5497d6f1c31d5 Mon Sep 17 00:00:00 2001 From: Kamal Nasser Date: Wed, 23 Oct 2019 16:38:44 +0300 Subject: [PATCH] update port whitelisting rules, refactor IsValidRedirect tests --- oauthproxy.go | 56 +++++++++++-- oauthproxy_test.go | 204 ++++++++++++++++++++++++++++++++------------- 2 files changed, 193 insertions(+), 67 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index 5278445c..8d781770 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -494,6 +494,43 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error) return } +// splitHostPort separates host and port. If the port is not valid, it returns +// the entire input as host, and it doesn't check the validity of the host. +// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric. +// *** taken from net/url, modified validOptionalPort() to accept ":*" +func splitHostPort(hostport string) (host, port string) { + host = hostport + + colon := strings.LastIndexByte(host, ':') + if colon != -1 && validOptionalPort(host[colon:]) { + host, port = host[:colon], host[colon+1:] + } + + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] + } + + return +} + +// validOptionalPort reports whether port is either an empty string +// or matches /^:\d*$/ +// *** taken from net/url, modified to accept ":*" +func validOptionalPort(port string) bool { + if port == "" || port == ":*" { + return true + } + if port[0] != ':' { + return false + } + for _, b := range port[1:] { + if b < '0' || b > '9' { + return false + } + } + return true +} + // IsValidRedirect checks whether the redirect URL is whitelisted func (p *OAuthProxy) IsValidRedirect(redirect string) bool { switch { @@ -507,19 +544,20 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool { redirectHostname := redirectURL.Hostname() for _, domain := range p.whitelistDomains { - domainURL := url.URL{ - Host: strings.TrimLeft(domain, "."), - } - domainHostname := domainURL.Hostname() - if domainHostname == "" { + domainHostname, domainPort := splitHostPort(strings.TrimLeft(domain, ".")) + if err != nil || domainHostname == "" { continue } if (redirectHostname == domainHostname) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectHostname, domainHostname)) { - // if the domain has a port, only allow that port - // otherwise allow any port - domainPort := domainURL.Port() - if (domainPort == "") || (domainPort == redirectURL.Port()) { + // the domain names match, now validate the ports + // if the whitelisted domain's port is '*', allow all ports + // if the whitelisted domain contains a specific port, only allow that port + // if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https + redirectPort := redirectURL.Port() + if (domainPort == "*") || + (domainPort == redirectPort) || + (domainPort == "" && redirectPort == "") { return true } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 2745c1b7..5bd6523c 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -182,70 +182,158 @@ func TestIsValidRedirect(t *testing.T) { opts.ClientSecret = "fgkdsgj" opts.CookieSecret = "ljgiogbj" // Should match domains that are exactly foo.bar and any subdomain of bar.foo - opts.WhitelistDomains = []string{"foo.bar", ".bar.foo", "port.bar:8080", ".sub.port.bar:8080"} + opts.WhitelistDomains = []string{ + "foo.bar", + ".bar.foo", + "port.bar:8080", + ".sub.port.bar:8080", + "anyport.bar:*", + ".sub.anyport.bar:*", + } opts.Validate() proxy := NewOAuthProxy(opts, func(string) bool { return true }) - noRD := proxy.IsValidRedirect("") - assert.Equal(t, false, noRD) + testCases := []struct { + Desc, Redirect string + ExpectedResult bool + }{ + { + Desc: "noRD", + Redirect: "", + ExpectedResult: false, + }, + { + Desc: "singleSlash", + Redirect: "/redirect", + ExpectedResult: true, + }, + { + Desc: "doubleSlash", + Redirect: "//redirect", + ExpectedResult: false, + }, + { + Desc: "validHTTP", + Redirect: "http://foo.bar/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPS", + Redirect: "https://foo.bar/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidHTTPSubdomain", + Redirect: "http://baz.foo.bar/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPSSubdomain", + Redirect: "https://baz.foo.bar/redirect", + ExpectedResult: false, + }, + { + Desc: "validHTTPSubdomain", + Redirect: "http://baz.bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPSSubdomain", + Redirect: "https://baz.bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPDomain", + Redirect: "http://bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidHTTP1", + Redirect: "http://foo.bar.evil.corp/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPS1", + Redirect: "https://foo.bar.evil.corp/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTP2", + Redirect: "http://evil.corp/redirect?rd=foo.bar", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPS2", + Redirect: "https://evil.corp/redirect?rd=foo.bar", + ExpectedResult: false, + }, + { + Desc: "invalidPort", + Redirect: "https://evil.corp:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidEmptyPort", + Redirect: "http://foo.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidEmptyPortSubdomain", + Redirect: "http://baz.bar.foo:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validSpecificPort", + Redirect: "http://port.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidSpecificPort", + Redirect: "http://port.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validSpecificPortSubdomain", + Redirect: "http://foo.sub.port.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidSpecificPortSubdomain", + Redirect: "http://foo.sub.port.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validAnyPort1", + Redirect: "http://anyport.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPort2", + Redirect: "http://anyport.bar:8081/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPortSubdomain1", + Redirect: "http://a.sub.anyport.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPortSubdomain2", + Redirect: "http://a.sub.anyport.bar:8081/redirect", + ExpectedResult: true, + }, + } - singleSlash := proxy.IsValidRedirect("/redirect") - assert.Equal(t, true, singleSlash) + for _, tc := range testCases { + t.Run(tc.Desc, func(t *testing.T) { + result := proxy.IsValidRedirect(tc.Redirect) - doubleSlash := proxy.IsValidRedirect("//redirect") - assert.Equal(t, false, doubleSlash) - - validHTTP := proxy.IsValidRedirect("http://foo.bar/redirect") - assert.Equal(t, true, validHTTP) - - validHTTPS := proxy.IsValidRedirect("https://foo.bar/redirect") - assert.Equal(t, true, validHTTPS) - - invalidHTTPSubdomain := proxy.IsValidRedirect("http://baz.foo.bar/redirect") - assert.Equal(t, false, invalidHTTPSubdomain) - - invalidHTTPSSubdomain := proxy.IsValidRedirect("https://baz.foo.bar/redirect") - assert.Equal(t, false, invalidHTTPSSubdomain) - - validHTTPSubdomain := proxy.IsValidRedirect("http://baz.bar.foo/redirect") - assert.Equal(t, true, validHTTPSubdomain) - - validHTTPSSubdomain := proxy.IsValidRedirect("https://baz.bar.foo/redirect") - assert.Equal(t, true, validHTTPSSubdomain) - - invalidHTTP1 := proxy.IsValidRedirect("http://foo.bar.evil.corp/redirect") - assert.Equal(t, false, invalidHTTP1) - - invalidHTTPS1 := proxy.IsValidRedirect("https://foo.bar.evil.corp/redirect") - assert.Equal(t, false, invalidHTTPS1) - - invalidHTTP2 := proxy.IsValidRedirect("http://evil.corp/redirect?rd=foo.bar") - assert.Equal(t, false, invalidHTTP2) - - invalidHTTPS2 := proxy.IsValidRedirect("https://evil.corp/redirect?rd=foo.bar") - assert.Equal(t, false, invalidHTTPS2) - - invalidPort := proxy.IsValidRedirect("https://evil.corp:3838/redirect") - assert.Equal(t, false, invalidPort) - - validAnyPort := proxy.IsValidRedirect("http://foo.bar:3838/redirect") - assert.Equal(t, true, validAnyPort) - - validAnyPortSubdomain := proxy.IsValidRedirect("http://baz.bar.foo:3838/redirect") - assert.Equal(t, true, validAnyPortSubdomain) - - validSpecificPort := proxy.IsValidRedirect("http://port.bar:8080/redirect") - assert.Equal(t, true, validSpecificPort) - - invalidSpecificPort := proxy.IsValidRedirect("http://port.bar:3838/redirect") - assert.Equal(t, false, invalidSpecificPort) - - validSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:8080/redirect") - assert.Equal(t, true, validSpecificPortSubdomain) - - invalidSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:3838/redirect") - assert.Equal(t, false, invalidSpecificPortSubdomain) + if result != tc.ExpectedResult { + t.Errorf("expected %t got %t", tc.ExpectedResult, result) + } + }) + } } type TestProvider struct {