From 67d79efd2fa0a4d1dc48ffe8ee109a4d8020daff Mon Sep 17 00:00:00 2001 From: abador Date: Mon, 12 Jul 2021 11:52:24 +0200 Subject: [PATCH 1/6] feat:Allow wildcard domains for redirect_to checks --- docs/docs/reference/configuration.md | 2 ++ driver/config/config.go | 13 +++++++++++++ driver/config/config_test.go | 1 + go.mod | 1 + go.sum | 1 - internal/.kratos.yaml | 5 +++++ x/http_secure_redirect.go | 10 +++++++++- 7 files changed, 31 insertions(+), 2 deletions(-) diff --git a/docs/docs/reference/configuration.md b/docs/docs/reference/configuration.md index 5de071b14f54..cc2727a9427b 100644 --- a/docs/docs/reference/configuration.md +++ b/docs/docs/reference/configuration.md @@ -1018,6 +1018,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: @@ -1029,6 +1030,7 @@ selfservice: - https://app.my-app.com/dashboard - /dashboard - https://www.my-app.com/ + - https://*.my-app.com/ ## serve ## # diff --git a/driver/config/config.go b/driver/config/config.go index 1cdfcecad494..30b377e7e46f 100644 --- a/driver/config/config.go +++ b/driver/config/config.go @@ -14,6 +14,8 @@ import ( "testing" "time" + "golang.org/x/net/publicsuffix" + "github.com/google/uuid" "github.com/ory/x/dbal" @@ -672,6 +674,17 @@ 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 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 b04d15c0894a..f73baef107aa 100644 --- a/driver/config/config_test.go +++ b/driver/config/config_test.go @@ -47,6 +47,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", }, ds) pWithFragments := config.MustNew(t, logrusx.New("", ""), diff --git a/go.mod b/go.mod index 6c8c70012623..e9f55a7f044c 100644 --- a/go.mod +++ b/go.mod @@ -88,6 +88,7 @@ require ( github.com/urfave/negroni v1.0.0 github.com/ziutek/mymysql v1.5.4 // indirect golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 + golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 golang.org/x/oauth2 v0.0.0-20210323180902-22b0adad7558 golang.org/x/sync v0.0.0-20201207232520-09787c993a3a golang.org/x/tools v0.1.0 diff --git a/go.sum b/go.sum index ec810fd5e709..e22806b5e167 100644 --- a/go.sum +++ b/go.sum @@ -384,7 +384,6 @@ github.com/ghodss/yaml v1.0.0 h1:wQHKEahhL6wmXdzwWG11gIVCkOv05bNOh+Rxn0yngAk= github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= -github.com/go-bindata/go-bindata v3.1.1+incompatible h1:tR4f0e4VTO7LK6B2YWyAoVEzG9ByG1wrXB4TL9+jiYg= github.com/go-bindata/go-bindata v3.1.1+incompatible/go.mod h1:xK8Dsgwmeed+BBsSy2XTopBn/8uK2HWuGSnA11C3Joo= github.com/go-errors/errors v1.0.1 h1:LUHzmkK3GUKUrL/1gfBUxAHzcev3apQlezX/+O7ma6w= github.com/go-errors/errors v1.0.1/go.mod h1:f4zRHt4oKfwPJE5k8C9vpYG+aDHdBFUsgrm6/TyX73Q= diff --git a/internal/.kratos.yaml b/internal/.kratos.yaml index 52d4e1501b00..30dbe9938283 100644 --- a/internal/.kratos.yaml +++ b/internal/.kratos.yaml @@ -45,6 +45,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://* methods: password: enabled: true diff --git a/x/http_secure_redirect.go b/x/http_secure_redirect.go index 3189498a9a18..3d29ba25d46d 100644 --- a/x/http_secure_redirect.go +++ b/x/http_secure_redirect.go @@ -55,6 +55,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(returnTo.Host, 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) { @@ -87,7 +95,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, "/")) { From b5351c3e1ef9ac4455968952266873fc1a498b96 Mon Sep 17 00:00:00 2001 From: abador Date: Thu, 21 Oct 2021 09:38:33 +0200 Subject: [PATCH 2/6] Check if host is set and case-insensitive verification of the redirect domain --- driver/config/config.go | 3 ++- x/http_secure_redirect.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/driver/config/config.go b/driver/config/config.go index 30b377e7e46f..b7dda02b76d1 100644 --- a/driver/config/config.go +++ b/driver/config/config.go @@ -679,7 +679,8 @@ func (p *Config) SelfServiceBrowserWhitelistedReturnToDomains() (us []url.URL) { continue } eTLD, icann := publicsuffix.PublicSuffix(parsed.Host) - if parsed.Host[:1] == "*" && + 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) diff --git a/x/http_secure_redirect.go b/x/http_secure_redirect.go index 3d29ba25d46d..3280578c63f6 100644 --- a/x/http_secure_redirect.go +++ b/x/http_secure_redirect.go @@ -58,7 +58,7 @@ 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(returnTo.Host, allowed.Host[1:]) + return strings.HasSuffix(strings.ToLower(returnTo.Host), strings.ToLower(allowed.Host)[1:]) } return strings.EqualFold(allowed.Host, returnTo.Host) } From 67481b67b59e2f3208df5810012950e8105f8bd7 Mon Sep 17 00:00:00 2001 From: abador Date: Thu, 21 Oct 2021 10:31:38 +0200 Subject: [PATCH 3/6] Add SecureRedirectToIsWhiteListedHost tests --- x/http_secure_redirect_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/x/http_secure_redirect_test.go b/x/http_secure_redirect_test.go index 4ea78a0fca4c..83d04899a04d 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) { + testCases := []struct { + n string + whitelistedURL string + redirectURL string + valid bool + }{ + {n: "case= Domain is whitelisted", whitelistedURL: "foo.bar", redirectURL: "https://foo.bar/redir", valid: true}, + {n: "case= Domain prefix is whitelisted", whitelistedURL: "*.bar", redirectURL: "https://foo.bar/redir", valid: true}, + {n: "case= Subdomain prefix is whitelisted", whitelistedURL: "*.foo.bar", redirectURL: "https://auth.foo.bar/redir", valid: true}, + {n: "case= Domain is not whitelisted", whitelistedURL: "foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, + {n: "case= Domain wildcard is not whitelisted", whitelistedURL: "*.foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, + {n: "case= Subdomain is not whitelisted", whitelistedURL: "*.foo.baz", redirectURL: "https://auth.foo.bar/redir", valid: false}, + } + for _, tCase := range testCases { + t.Run(tCase.n, func(t *testing.T) { + whitelistedURL, err := url.Parse(tCase.whitelistedURL) + require.NoError(t, err) + redirectURL, err := url.Parse(tCase.redirectURL) + require.NoError(t, err) + assert.Equal(t, x.SecureRedirectToIsWhiteListedHost(redirectURL, *whitelistedURL), tCase.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 { From e826cc5f5b8bfcf566e9496a4fb77a34837b7bd3 Mon Sep 17 00:00:00 2001 From: abador Date: Thu, 21 Oct 2021 13:40:01 +0200 Subject: [PATCH 4/6] Add go mod for publicsuffix --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 2918c7e3a2c4..cb723f3f9ed6 100644 --- a/go.mod +++ b/go.mod @@ -102,7 +102,7 @@ require ( github.com/urfave/negroni v1.0.0 golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 golang.org/x/mod v0.5.1 // indirect - golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 + golang.org/x/net v0.0.0-20211020060615-d418f374d309 golang.org/x/oauth2 v0.0.0-20210402161424-2e8d93401602 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6 // indirect diff --git a/go.sum b/go.sum index 3ddcca9c5728..9bd6b1feb942 100644 --- a/go.sum +++ b/go.sum @@ -2026,6 +2026,8 @@ golang.org/x/net v0.0.0-20210323141857-08027d57d8cf/go.mod h1:RBQZq4jEuRlivfhVLd golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM= golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d h1:20cMwl2fHAzkJMEA+8J4JgqBQcQGzbisXo31MIeenXI= golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= +golang.org/x/net v0.0.0-20211020060615-d418f374d309 h1:A0lJIi+hcTR6aajJH4YqKWwohY4aW9RO7oRMcdv+HKI= +golang.org/x/net v0.0.0-20211020060615-d418f374d309/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20181003184128-c57b0facaced/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= From add81f00afaa11a227879118ee3e36593b6b8526 Mon Sep 17 00:00:00 2001 From: abador Date: Thu, 21 Oct 2021 14:57:52 +0200 Subject: [PATCH 5/6] fix tests --- x/http_secure_redirect_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/x/http_secure_redirect_test.go b/x/http_secure_redirect_test.go index 83d04899a04d..234c5630ee83 100644 --- a/x/http_secure_redirect_test.go +++ b/x/http_secure_redirect_test.go @@ -74,26 +74,26 @@ func TestSecureContentNegotiationRedirection(t *testing.T) { } func TestSecureRedirectToIsWhiteListedHost(t *testing.T) { - testCases := []struct { - n string + type testCase struct { whitelistedURL string redirectURL string valid bool - }{ - {n: "case= Domain is whitelisted", whitelistedURL: "foo.bar", redirectURL: "https://foo.bar/redir", valid: true}, - {n: "case= Domain prefix is whitelisted", whitelistedURL: "*.bar", redirectURL: "https://foo.bar/redir", valid: true}, - {n: "case= Subdomain prefix is whitelisted", whitelistedURL: "*.foo.bar", redirectURL: "https://auth.foo.bar/redir", valid: true}, - {n: "case= Domain is not whitelisted", whitelistedURL: "foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, - {n: "case= Domain wildcard is not whitelisted", whitelistedURL: "*.foo.baz", redirectURL: "https://foo.bar/redir", valid: false}, - {n: "case= Subdomain is not whitelisted", whitelistedURL: "*.foo.baz", redirectURL: "https://auth.foo.bar/redir", valid: false}, } - for _, tCase := range testCases { - t.Run(tCase.n, func(t *testing.T) { - whitelistedURL, err := url.Parse(tCase.whitelistedURL) + 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(tCase.redirectURL) + redirectURL, err := url.Parse(tc.redirectURL) require.NoError(t, err) - assert.Equal(t, x.SecureRedirectToIsWhiteListedHost(redirectURL, *whitelistedURL), tCase.valid) + assert.Equal(t, x.SecureRedirectToIsWhiteListedHost(redirectURL, *whitelistedURL), tc.valid) }) } } From 5ff7ee5d7f4db8a1adb8c48352931e7de29a0e15 Mon Sep 17 00:00:00 2001 From: abador Date: Mon, 8 Nov 2021 08:56:37 +0100 Subject: [PATCH 6/6] Add info in documentation --- .../browser-redirect-flow-completion.mdx | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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: