diff --git a/access.go b/access.go index 3d462f0..8c9fecf 100644 --- a/access.go +++ b/access.go @@ -212,9 +212,11 @@ func (s *Server) handleAuthorizationCodeRequest(w *Response, r *http.Request) *A if ret.RedirectUri == "" { ret.RedirectUri = FirstUri(ret.Client.GetRedirectUri(), s.Config.RedirectUriSeparator) } - if err = ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil { + if realRedirectUri, err := ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil { s.setErrorAndLog(w, E_INVALID_REQUEST, err, "auth_code_request=%s", "error validating client redirect") return nil + } else { + ret.RedirectUri = realRedirectUri } if ret.AuthorizeData.RedirectUri != ret.RedirectUri { s.setErrorAndLog(w, E_INVALID_REQUEST, errors.New("Redirect uri is different"), "auth_code_request=%s", "client redirect does not match authorization data") diff --git a/authorize.go b/authorize.go index 982d000..8c14017 100644 --- a/authorize.go +++ b/authorize.go @@ -148,10 +148,12 @@ func (s *Server) HandleAuthorizeRequest(w *Response, r *http.Request) *Authorize ret.RedirectUri = FirstUri(ret.Client.GetRedirectUri(), s.Config.RedirectUriSeparator) } - if err = ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil { + if realRedirectUri, err := ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil { w.SetErrorState(E_INVALID_REQUEST, "", ret.State) w.InternalError = err return nil + } else { + ret.RedirectUri = realRedirectUri } w.SetRedirect(ret.RedirectUri) diff --git a/urivalidate.go b/urivalidate.go index f72e84e..5496b06 100644 --- a/urivalidate.go +++ b/urivalidate.go @@ -18,10 +18,44 @@ func newUriValidationError(msg string, base string, redirect string) UriValidati return UriValidationError(fmt.Sprintf("%s: %s / %s", msg, base, redirect)) } +// Parse urls, resolving uri references to base url +func ParseUrls(baseUrl, redirectUrl string) (retBaseUrl, retRedirectUrl *url.URL, err error) { + var base, redirect *url.URL + // parse base url + if base, err = url.Parse(baseUrl); err != nil { + return nil, nil, err + } + + // parse redirect url + if redirect, err = url.Parse(redirectUrl); err != nil { + return nil, nil, err + } + + // must not have fragment + if base.Fragment != "" || redirect.Fragment != "" { + return nil, nil, newUriValidationError("url must not include fragment.", baseUrl, redirectUrl) + } + + // Scheme must match + if redirect.Scheme != base.Scheme { + return nil, nil, newUriValidationError("scheme mismatch", baseUrl, redirectUrl) + } + + // Host must match + if redirect.Host != base.Host { + return nil, nil, newUriValidationError("host mismatch", baseUrl, redirectUrl) + } + + // resolve references to base url + retBaseUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: base.Path}) + retRedirectUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: redirect.Path}) + return +} + // ValidateUriList validates that redirectUri is contained in baseUriList. // baseUriList may be a string separated by separator. // If separator is blank, validate only 1 URI. -func ValidateUriList(baseUriList string, redirectUri string, separator string) error { +func ValidateUriList(baseUriList string, redirectUri string, separator string) (realRedirectUri string, err error) { // make a list of uris var slist []string if separator != "" { @@ -32,71 +66,44 @@ func ValidateUriList(baseUriList string, redirectUri string, separator string) e } for _, sitem := range slist { - err := ValidateUri(sitem, redirectUri) + realRedirectUri, err = ValidateUri(sitem, redirectUri) // validated, return no error if err == nil { - return nil + return realRedirectUri, nil } // if there was an error that is not a validation error, return it if _, iok := err.(UriValidationError); !iok { - return err + return "", err } } - return newUriValidationError("urls don't validate", baseUriList, redirectUri) + return "", newUriValidationError("urls don't validate", baseUriList, redirectUri) } // ValidateUri validates that redirectUri is contained in baseUri -func ValidateUri(baseUri string, redirectUri string) error { +func ValidateUri(baseUri string, redirectUri string) (realRedirectUri string, err error) { if baseUri == "" || redirectUri == "" { - return errors.New("urls cannot be blank.") + return "", errors.New("urls cannot be blank.") } - // parse base url - base, err := url.Parse(baseUri) - if err != nil { - return err - } - - // parse passed url - redirect, err := url.Parse(redirectUri) + base, redirect, err := ParseUrls(baseUri, redirectUri) if err != nil { - return err - } - - // must not have fragment - if base.Fragment != "" || redirect.Fragment != "" { - return errors.New("url must not include fragment.") - } - - // check if urls match - if base.Scheme != redirect.Scheme { - return newUriValidationError("scheme mismatch", baseUri, redirectUri) - } - if base.Host != redirect.Host { - return newUriValidationError("host mismatch", baseUri, redirectUri) + return "", err } // allow exact path matches if base.Path == redirect.Path { - return nil + return redirect.String(), nil } // ensure prefix matches are actually subpaths requiredPrefix := strings.TrimRight(base.Path, "/") + "/" if !strings.HasPrefix(redirect.Path, requiredPrefix) { - return newUriValidationError("path is not a subpath", baseUri, redirectUri) - } - - // ensure prefix matches don't contain path traversals - for _, s := range strings.Split(strings.TrimPrefix(redirect.Path, requiredPrefix), "/") { - if s == ".." { - return newUriValidationError("subpath cannot contain path traversal", baseUri, redirectUri) - } + return "", newUriValidationError("path prefix doesn't match", baseUri, redirectUri) } - return nil + return redirect.String(),nil } // Returns the first uri from an uri list diff --git a/urivalidate_test.go b/urivalidate_test.go index 3864a83..6c566f3 100644 --- a/urivalidate_test.go +++ b/urivalidate_test.go @@ -10,41 +10,62 @@ func TestURIValidate(t *testing.T) { // Exact match "http://localhost:14000/appauth", "http://localhost:14000/appauth", + "http://localhost:14000/appauth", }, { // Trailing slash "http://www.google.com/myapp", "http://www.google.com/myapp/", + "http://www.google.com/myapp/", }, { // Exact match with trailing slash "http://www.google.com/myapp/", "http://www.google.com/myapp/", + "http://www.google.com/myapp/", }, { // Subpath "http://www.google.com/myapp", "http://www.google.com/myapp/interface/implementation", + "http://www.google.com/myapp/interface/implementation", }, { // Subpath with trailing slash "http://www.google.com/myapp/", "http://www.google.com/myapp/interface/implementation", + "http://www.google.com/myapp/interface/implementation", }, { // Subpath with things that are close to path traversals, but aren't "http://www.google.com/myapp", "http://www.google.com/myapp/.../..implementation../...", + "http://www.google.com/myapp/.../..implementation../...", }, { // If the allowed basepath contains path traversals, allow them? "http://www.google.com/traversal/../allowed", "http://www.google.com/traversal/../allowed/with/subpath", + "http://www.google.com/allowed/with/subpath", + }, + { + // Backslashes + "https://mysafewebsite.com/secure/redirect", + "https://mysafewebsite.com/secure/redirect/\\../\\../\\../evil", + "https://mysafewebsite.com/secure/redirect/%5C../%5C../%5C../evil", + }, + { + // Backslashes + "https://mysafewebsite.com/secure/redirect", + "https://mysafewebsite.com/secure/redirect/\\..\\../\\../evil", + "https://mysafewebsite.com/secure/redirect/%5C..%5C../%5C../evil", }, } for _, v := range valid { - if err := ValidateUri(v[0], v[1]); err != nil { + if realRedirectUri, err := ValidateUri(v[0], v[1]); err != nil { t.Errorf("Expected ValidateUri(%s, %s) to succeed, got %v", v[0], v[1], err) + } else if len(v) == 3 && realRedirectUri != v[2] { + t.Errorf("Expected ValidateUri(%s, %s) to return uri %s, got %s", v[0], v[1], v[2], realRedirectUri) } } @@ -89,9 +110,14 @@ func TestURIValidate(t *testing.T) { "http://www.google.com/myapp", "http://www.google.com/myapp../test", }, + { + // Backslashes + "https://mysafewebsite.com/secure/redirect", + "https://mysafewebsite.com/secure%2fredirect/../evil", + }, } for _, v := range invalid { - if err := ValidateUri(v[0], v[1]); err == nil { + if _, err := ValidateUri(v[0], v[1]); err == nil { t.Errorf("Expected ValidateUri(%s, %s) to fail", v[0], v[1]) } } @@ -99,22 +125,22 @@ func TestURIValidate(t *testing.T) { func TestURIListValidate(t *testing.T) { // V1 - if err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/appauth", ""); err != nil { + if _, err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/appauth", ""); err != nil { t.Errorf("V1: %s", err) } // V2 - if err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/app", ""); err == nil { + if _, err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/app", ""); err == nil { t.Error("V2 should have failed") } // V3 - if err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/appauth", ";"); err != nil { + if _, err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/appauth", ";"); err != nil { t.Errorf("V3: %s", err) } // V4 - if err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/app", ";"); err == nil { + if _, err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/app", ";"); err == nil { t.Error("V4 should have failed") } }