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.

Sign up for GitHub

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

Jump to bottom

feat: Allow wildcard domains for redirect_to checks #1528

Merged
merged 15 commits into from
Nov 9, 2021
Merged

feat: Allow wildcard domains for redirect_to checks #1528

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
67d79ef
feat:Allow wildcard domains for redirect_to checks
abador Jul 12, 2021
b5351c3
Check if host is set and case-insensitive verification of the redirec…
abador Oct 21, 2021
67481b6
Add SecureRedirectToIsWhiteListedHost tests
abador Oct 21, 2021
3d25bcc
Merge remote-tracking branch 'origin/master' into allow-wildcard-redi…
abador Oct 21, 2021
e826cc5
Add go mod for publicsuffix
abador Oct 21, 2021
add81f0
fix tests
abador Oct 21, 2021
8021987
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 25, 2021
0d6e69b
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 26, 2021
31f60f5
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 28, 2021
b0931b8
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 28, 2021
17e9bcc
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 28, 2021
40c3702
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Oct 29, 2021
008f704
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Nov 2, 2021
3b8a606
Merge branch 'master' into allow-wildcard-redirect-to-domains
abador Nov 8, 2021
5ff7ee5
Add info in documentation
abador Nov 8, 2021
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -1029,6 +1030,7 @@ selfservice:
- https://app.my-app.com/dashboard
- /dashboard
- https://www.my-app.com/
- https://*.my-app.com/

## serve ##
#
Expand Down
13 changes: 13 additions & 0 deletions driver/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"testing"
"time"

"golang.org/x/net/publicsuffix"

"github.com/google/uuid"

"github.com/ory/x/dbal"
Expand Down Expand Up @@ -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] == "*" &&
Copy link
Member

Choose a reason for hiding this comment

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

This panics if host is an empty string. Also, what about foo.*.bar?

Copy link
Contributor Author

@abador abador Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Most of the cases I can think of is adding some return domain eg: page1.foo.bar or page2.foo.bar where those changes are valid. If we want to make more complicated situations work I would probably go with regex, but is this case really needed?

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("", ""),
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
5 changes: 5 additions & 0 deletions internal/.kratos.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion x/http_secure_redirect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
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(returnTo.Host, allowed.Host[1:])
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan for case sensitivity here?

  • On documentation, the function docstring is ambiguous,
  • when wildcards are used, the match is case-sensitive,
  • when wildcards are not used, the match is case-insensitive.

To follow the principle of least surprise, you might want to do one of

  • Do strings.HasSuffix(strings.ToLower(returnTo.Host), strings.ToLower(allowed.Host[1:]) here to be case-insensitive.
  • .. or do return allowed.Host == returnTo.Host few lines later to be case-sensitive.
  • .. or mention this semi-case-insensitivity behavior in the function comment if it is intentional and preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Hostnames are case insensitive, so the correct approach would be to have the wildcard matches also case insensitive :)

}
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 @@ -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, "/")) {
Expand Down