Skip to content
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.

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

feat: Allow wildcard domains for redirect_to checks #1528

Merged
merged 15 commits into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions docs/docs/reference/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -1202,6 +1203,7 @@ selfservice:
- https://app.my-app.com/dashboard
- /dashboard
- https://www.my-app.com/
- https://*.my-app.com/

## serve ##
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 14 additions & 0 deletions driver/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"testing"
"time"

"golang.org/x/net/publicsuffix"

"github.com/duo-labs/webauthn/protocol"

"github.com/duo-labs/webauthn/webauthn"
Expand Down Expand Up @@ -798,6 +800,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)
}
Expand Down
1 change: 1 addition & 0 deletions driver/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 5 additions & 0 deletions driver/config/stub/.kratos.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion x/http_secure_redirect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add tests for this?

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) {
Expand Down Expand Up @@ -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, "/")) {
Expand Down
25 changes: 25 additions & 0 deletions x/http_secure_redirect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down