diff --git a/pkg/app/redirect/pagewriter_suite_test.go b/pkg/app/redirect/pagewriter_suite_test.go new file mode 100644 index 00000000..45703ff3 --- /dev/null +++ b/pkg/app/redirect/pagewriter_suite_test.go @@ -0,0 +1,17 @@ +package redirect + +import ( + "testing" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestOptionsSuite(t *testing.T) { + logger.SetOutput(GinkgoWriter) + logger.SetErrOutput(GinkgoWriter) + + RegisterFailHandler(Fail) + RunSpecs(t, "Redirect Suite") +} diff --git a/pkg/app/redirect/validator.go b/pkg/app/redirect/validator.go new file mode 100644 index 00000000..78834425 --- /dev/null +++ b/pkg/app/redirect/validator.go @@ -0,0 +1,120 @@ +package redirect + +import ( + "net/url" + "regexp" + "strings" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" +) + +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})[/\\]`) +) + +// Validator is an interface to allow validation of application +// redirect URLs. +// As these values are determined from the request, they must go +// through thorough checks to ensure the safety of the end user. +type Validator interface { + IsValidRedirect(redirect string) bool +} + +// NewValidator constructs a new redirect validator. +func NewValidator(allowedDomains []string) Validator { + return &validator{ + allowedDomains: allowedDomains, + } +} + +// validator implements the Validator interface to allow validation +// of redirect URLs. +type validator struct { + allowedDomains []string +} + +// IsValidRedirect checks whether the redirect URL is safe and allowed. +func (v *validator) IsValidRedirect(redirect string) bool { + switch { + case redirect == "": + // The user didn't specify a redirect. + // In this case, we expect the proxt to fallback to `/` + return false + 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) + if err != nil { + logger.Printf("Rejecting invalid redirect %q: scheme unsupported or missing", redirect) + return false + } + redirectHostname := redirectURL.Hostname() + + for _, allowedDomain := range v.allowedDomains { + allowedHost, allowedPort := splitHostPort(allowedDomain) + if allowedHost == "" { + continue + } + + if redirectHostname == strings.TrimPrefix(allowedHost, ".") || + (strings.HasPrefix(allowedHost, ".") && + strings.HasSuffix(redirectHostname, allowedHost)) { + // 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 allowedPort == "*" || + allowedPort == redirectPort || + (allowedPort == "" && redirectPort == "") { + return true + } + } + } + + logger.Printf("Rejecting invalid redirect %q: domain / port not in whitelist", redirect) + return false + default: + logger.Printf("Rejecting invalid redirect %q: not an absolute or relative URL", redirect) + return false + } +} + +// 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 +} diff --git a/pkg/app/redirect/validator_test.go b/pkg/app/redirect/validator_test.go new file mode 100644 index 00000000..ac08f6dc --- /dev/null +++ b/pkg/app/redirect/validator_test.go @@ -0,0 +1,102 @@ +package redirect + +import ( + "bufio" + "net/url" + "os" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" +) + +var _ = Describe("Validator suite", func() { + var testAllowedDomains []string + + BeforeEach(func() { + testAllowedDomains = []string{ + "foo.bar", + ".bar.foo", + "port.bar:8080", + ".sub.port.bar:8080", + "anyport.bar:*", + ".sub.anyport.bar:*", + "www.whitelisteddomain.tld", + } + }) + + Context("OpenRedirect List", func() { + file, err := os.Open("../../../testdata/openredirects.txt") + Expect(err).ToNot(HaveOccurred()) + defer func() { + Expect(file.Close()).To(Succeed()) + }() + + scanner := bufio.NewScanner(file) + for scanner.Scan() { + rd := scanner.Text() + It(rd, func() { + rdUnescaped, err := url.QueryUnescape(rd) + Expect(err).ToNot(HaveOccurred()) + + validator := NewValidator(testAllowedDomains) + Expect(validator.IsValidRedirect(rdUnescaped)).To(BeFalse(), "Expected redirect not to be valid") + }) + } + + Expect(scanner.Err()).ToNot(HaveOccurred()) + }) + + Context("Validator", func() { + DescribeTable("IsValidRedirect", + func(testRedirect string, expected bool) { + validator := NewValidator(testAllowedDomains) + Expect(validator.IsValidRedirect(testRedirect)).To(Equal(expected)) + }, + Entry("No Redirect", "", false), + Entry("Single Slash", "/redirect", true), + Entry("Double Slash", "//redirect", false), + Entry("Valid HTTP", "http://foo.bar/redirect", true), + Entry("Valid HTTPS", "https://foo.bar/redirect", true), + Entry("Invalid HTTP subdomain", "http://baz.foo.bar/redirect", false), + Entry("Invalid HTTPS subdomain", "https://baz.foo.bar/redirect", false), + Entry("Valid HTTP subdomain", "http://baz.bar.foo/redirect", true), + Entry("Valid HTTPS subdomain", "https://baz.bar.foo/redirect", true), + Entry("Valid HTTP Domain", "http://bar.foo/redirect", true), // Is this correct, do we want to match the root domain? + Entry("Invalid HTTP Similar Domain", "http://foo.bar.evil.corp/redirect", false), + Entry("Invalid HTTPS Similar Domain", "https://foo.bar.evil.corp/redirect", false), + Entry("Invalid HTTP RD Parameter", "http://evil.corp/redirect?rd=foo.bar", false), + Entry("Invalid HTTPS RD Parameter", "https://evil.corp/redirect?rd=foo.bar", false), + Entry("Invalid Port and Domain", "https://evil.corp:3838/redirect", false), + Entry("Invalid Port on Allowed Domain", "http://foo.bar:3838/redirect", false), + Entry("Invalid Port on Allowed Subdomain", "http://baz.bar.foo:3838/redirect", false), + Entry("Valid Specified Port and Domain", "http://port.bar:8080/redirect", true), + Entry("Invalid Specified Port and Domain", "http://port.bar:3838/redirect", false), + Entry("Valid Specified Port and Subdomain", "http://foo.sub.port.bar:8080/redirect", true), + Entry("Invalid Specified Port andSubdomain", "http://foo.subport.bar:3838/redirect", false), + Entry("Valid Any Port, Specified Domain", "http://anyport.bar:8080/redirect", true), + Entry("Valid Different Any Port, Specified Domain", "http://anyport.bar:8081/redirect", true), + Entry("Valid Any Port, Specified Subdomain", "http://a.sub.anyport.bar:8080/redirect", true), + Entry("Valid Different Any Port, Specified Subdomain", "http://a.sub.anyport.bar:8081/redirect", true), + Entry("Escape Double Slash", "/\\evil.com", false), + Entry("Space Single Slash", "/ /evil.com", false), + Entry("Space Double Slash", "/ \\evil.com", false), + Entry("Tab Single Slash", "/\t/evil.com", false), + Entry("Tab Double Slash", "/\t\\evil.com", false), + Entry("Vertical Tab Single Slash", "/\v/evil.com", false), + Entry("Vertiacl Tab Double Slash", "/\v\\evil.com", false), + Entry("New Line Single Slash", "/\n/evil.com", false), + Entry("New Line Double Slash", "/\n\\evil.com", false), + Entry("Carriage Return Single Slash", "/\r/evil.com", false), + Entry("Carriage Return Double Slash", "/\r\\evil.com", false), + Entry("Double Tab", "/\t/\t\\evil.com", false), + Entry("Triple Tab 1", "/\t\t/\t/evil.com", false), + Entry("Triple Tab 2", "/\t\t\\\t/evil.com", false), + Entry("Quad Tab 1", "/\t\t/\t\t\\evil.com", false), + Entry("Quad Tab 2", "/\t\t\\\t\t/evil.com", false), + Entry("Relative Path", "/./\\evil.com", false), + Entry("Relative Subpath", "/./../../\\evil.com", false), + Entry("Partial Subdomain", "evilbar.foo", false), + ) + }) +})