From 349cdcf4b1298d9e544344705ecd8e7b5eada48c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Czaus?= Date: Tue, 9 Nov 2021 09:18:24 +0100 Subject: [PATCH] feat: allow wildcard domains for redirect_to checks (#1528) Support wildcard domains in redirect_to checks. Closes #943 --- docs/docs/reference/configuration.md | 2 ++ .../browser-redirect-flow-completion.mdx | 21 ++++++++++++++++ driver/config/config.go | 14 +++++++++++ driver/config/config_test.go | 1 + driver/config/stub/.kratos.yaml | 5 ++++ go.mod | 2 ++ x/http_secure_redirect.go | 10 +++++++- x/http_secure_redirect_test.go | 25 +++++++++++++++++++ 8 files changed, 79 insertions(+), 1 deletion(-) diff --git a/docs/docs/reference/configuration.md b/docs/docs/reference/configuration.md index 46d0f70dd71b..22507bfd05cf 100644 --- a/docs/docs/reference/configuration.md +++ b/docs/docs/reference/configuration.md @@ -1191,6 +1191,7 @@ selfservice: # - - https://app.my-app.com/dashboard # - /dashboard # - https://www.my-app.com/ + # - https://*.my-app.com/ # # Set this value using environment variables on # - Linux/macOS: @@ -1202,6 +1203,7 @@ selfservice: - https://app.my-app.com/dashboard - /dashboard - https://www.my-app.com/ + - https://*.my-app.com/ ## serve ## # diff --git a/docs/versioned_docs/version-v0.8/concepts/browser-redirect-flow-completion.mdx b/docs/versioned_docs/version-v0.8/concepts/browser-redirect-flow-completion.mdx index 99f64bf516bb..0e7c12479666 100644 --- a/docs/versioned_docs/version-v0.8/concepts/browser-redirect-flow-completion.mdx +++ b/docs/versioned_docs/version-v0.8/concepts/browser-redirect-flow-completion.mdx @@ -70,6 +70,27 @@ selfservice: - https://www.myapp.com/ ``` +It's possible to add domain wildcards to our configuration. +As an example, we're adding all subdomains of `myapp.com` to the whitelist, thus +we can now specify a dynamic subdomain return url like so +`?return_to=https://foo.myapp.com` or `?return_to=https://bar.myapp.com`. + +```yaml file="path/to/my/kratos.config.yml" +selfservice: + whitelisted_return_urls: + - https://*.myapp.com/ +``` + +Please remember that adding wildcards for top level domains is blocked (to prevent +Open Redirect Attacks).If we try to add a wild card for a top level domain we'll get +an error: `Ignoring wildcard ...`. So adding something like this won't work: + +```yaml file="path/to/my/kratos.config.yml" +selfservice: + whitelisted_return_urls: + - https://*.com/ +``` + ### Post-Login Redirection Post-login redirection considers the following configuration keys: diff --git a/driver/config/config.go b/driver/config/config.go index a7a71b5beaac..116d5d17b472 100644 --- a/driver/config/config.go +++ b/driver/config/config.go @@ -16,6 +16,8 @@ import ( "testing" "time" + "golang.org/x/net/publicsuffix" + "github.com/duo-labs/webauthn/protocol" "github.com/duo-labs/webauthn/webauthn" @@ -808,6 +810,18 @@ func (p *Config) SelfServiceBrowserWhitelistedReturnToDomains() (us []url.URL) { p.l.WithError(err).Warnf("Ignoring URL \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k) continue } + if parsed.Host == "*" { + p.l.Warnf("Ignoring wildcard \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k) + continue + } + eTLD, icann := publicsuffix.PublicSuffix(parsed.Host) + if len(parsed.Host) > 0 && + parsed.Host[:1] == "*" && + icann && + parsed.Host == fmt.Sprintf("*.%s", eTLD) { + p.l.Warnf("Ignoring wildcard \"%s\" from configuration key \"%s.%d\".", u, ViperKeyURLsWhitelistedReturnToDomains, k) + continue + } us = append(us, *parsed) } diff --git a/driver/config/config_test.go b/driver/config/config_test.go index f2d12f089af8..332a2a26534e 100644 --- a/driver/config/config_test.go +++ b/driver/config/config_test.go @@ -63,6 +63,7 @@ func TestViperProvider(t *testing.T) { assert.Equal(t, []string{ "http://return-to-1-test.ory.sh/", "http://return-to-2-test.ory.sh/", + "http://*.wildcards.ory.sh", "/return-to-relative-test/", }, ds) diff --git a/driver/config/stub/.kratos.yaml b/driver/config/stub/.kratos.yaml index b479cc2ea5a7..dde344db1f14 100644 --- a/driver/config/stub/.kratos.yaml +++ b/driver/config/stub/.kratos.yaml @@ -50,6 +50,11 @@ selfservice: whitelisted_return_urls: - http://return-to-1-test.ory.sh/ - http://return-to-2-test.ory.sh/ + - http://*.wildcards.ory.sh + - http://*.sh + - http://*.com + - http://*.com.pl + - http://* - /return-to-relative-test/ methods: totp: diff --git a/go.mod b/go.mod index 3f17c8c62f3d..9f954dc10273 100644 --- a/go.mod +++ b/go.mod @@ -91,6 +91,8 @@ require ( github.com/tidwall/sjson v1.2.2 github.com/urfave/negroni v1.0.0 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 + golang.org/x/mod v0.5.1 // indirect + golang.org/x/net v0.0.0-20211020060615-d418f374d309 golang.org/x/oauth2 v0.0.0-20210514164344-f6687ab2804c golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/tools v0.1.7 diff --git a/x/http_secure_redirect.go b/x/http_secure_redirect.go index fb80f8ef03dc..30007da53bc8 100644 --- a/x/http_secure_redirect.go +++ b/x/http_secure_redirect.go @@ -57,6 +57,14 @@ func SecureRedirectOverrideDefaultReturnTo(defaultReturnTo *url.URL) SecureRedir } } +// SecureRedirectToIsWhitelisted validates if the redirect_to param is allowed for a given wildcard +func SecureRedirectToIsWhiteListedHost(returnTo *url.URL, allowed url.URL) bool { + if allowed.Host != "" && allowed.Host[:1] == "*" { + return strings.HasSuffix(strings.ToLower(returnTo.Host), strings.ToLower(allowed.Host)[1:]) + } + return strings.EqualFold(allowed.Host, returnTo.Host) +} + // SecureRedirectTo implements a HTTP redirector who mitigates open redirect vulnerabilities by // working with whitelisting. func SecureRedirectTo(r *http.Request, defaultReturnTo *url.URL, opts ...SecureRedirectOption) (returnTo *url.URL, err error) { @@ -89,7 +97,7 @@ func SecureRedirectTo(r *http.Request, defaultReturnTo *url.URL, opts ...SecureR var found bool for _, allowed := range o.whitelist { if strings.EqualFold(allowed.Scheme, returnTo.Scheme) && - strings.EqualFold(allowed.Host, returnTo.Host) && + SecureRedirectToIsWhiteListedHost(returnTo, allowed) && strings.HasPrefix( stringsx.Coalesce(returnTo.Path, "/"), stringsx.Coalesce(allowed.Path, "/")) { diff --git a/x/http_secure_redirect_test.go b/x/http_secure_redirect_test.go index 4ea78a0fca4c..234c5630ee83 100644 --- a/x/http_secure_redirect_test.go +++ b/x/http_secure_redirect_test.go @@ -73,6 +73,31 @@ func TestSecureContentNegotiationRedirection(t *testing.T) { }) } +func TestSecureRedirectToIsWhiteListedHost(t *testing.T) { + type testCase struct { + whitelistedURL string + redirectURL string + valid bool + } + tests := map[string]testCase{ + "case=Domain is whitelisted": {whitelistedURL: "https://foo.bar", redirectURL: "https://foo.bar/redir", valid: true}, + "case=Domain prefix is whitelisted": {whitelistedURL: "https://*.bar", redirectURL: "https://foo.bar/redir", valid: true}, + "case=Subdomain prefix is whitelisted": {whitelistedURL: "https://*.foo.bar", redirectURL: "https://auth.foo.bar/redir", valid: true}, + "case=Domain is not whitelisted": {whitelistedURL: "https://foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, + "case=Domain wildcard is not whitelisted": {whitelistedURL: "https://*.foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, + "case=Subdomain is not whitelisted": {whitelistedURL: "https://*.foo.baz", redirectURL: "https://auth.foo.bar/redir", valid: false}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + whitelistedURL, err := url.Parse(tc.whitelistedURL) + require.NoError(t, err) + redirectURL, err := url.Parse(tc.redirectURL) + require.NoError(t, err) + assert.Equal(t, x.SecureRedirectToIsWhiteListedHost(redirectURL, *whitelistedURL), tc.valid) + }) + } +} + func TestSecureRedirectTo(t *testing.T) { var newServer = func(t *testing.T, isTLS bool, isRelative bool, expectErr bool, opts func(ts *httptest.Server) []x.SecureRedirectOption) *httptest.Server {