diff --git a/pkg/s3-proxy/authx/authentication/oidc.go b/pkg/s3-proxy/authx/authentication/oidc.go index 725e8b44..275af136 100644 --- a/pkg/s3-proxy/authx/authentication/oidc.go +++ b/pkg/s3-proxy/authx/authentication/oidc.go @@ -125,13 +125,24 @@ func (s *service) OIDCEndpoints(providerKey string, oidcCfg *config.OIDCAuthConf } // Check if rdVal exists and that redirect url value is valid - if rdVal != "" && !isValidRedirect(rdVal) { - // Create error - err := errors.New("redirect url is invalid") - // Answer - responsehandler.GeneralBadRequestError(r, w, s.cfgManager, err) + if rdVal != "" { + isValid, err := isValidRedirect(rdVal, utils.GetRequestURI(r)) + // Check error + if err != nil { + // Answer + responsehandler.GeneralInternalServerError(r, w, s.cfgManager, err) - return + return + } + // Check if it is invalid + if !isValid { + // Create error + err = errors.New("redirect url is invalid") + // Answer + responsehandler.GeneralBadRequestError(r, w, s.cfgManager, err) + + return + } } // Create OIDC configuration @@ -471,6 +482,30 @@ func generateOIDCConfig( } // IsValidRedirect checks whether the redirect URL is whitelisted. -func isValidRedirect(redirect string) bool { - return strings.HasPrefix(redirect, "http://") || strings.HasPrefix(redirect, "https://") +func isValidRedirect(redirectURLStr, reqURLStr string) (bool, error) { + // Check if it isn't forged with complete urls + if !(strings.HasPrefix(redirectURLStr, "http://") || strings.HasPrefix(redirectURLStr, "https://")) { + return false, nil + } + + // Parse request URL + currentURL, err := url.Parse(reqURLStr) + // Check error + if err != nil { + return false, errors.WithStack(err) + } + // Parse redirect URL + redURL, err := url.Parse(redirectURLStr) + // Check error + if err != nil { + return false, errors.WithStack(err) + } + + // Check if hosts aren't the same + if redURL.Host != currentURL.Host { + return false, nil + } + + // Default + return true, nil } diff --git a/pkg/s3-proxy/authx/authentication/oidc_test.go b/pkg/s3-proxy/authx/authentication/oidc_test.go index 2699969b..dc5a3ffe 100644 --- a/pkg/s3-proxy/authx/authentication/oidc_test.go +++ b/pkg/s3-proxy/authx/authentication/oidc_test.go @@ -125,97 +125,114 @@ func Test_getJWTToken(t *testing.T) { func Test_isValidRedirect(t *testing.T) { type args struct { - redirect string + redirectURLStr string + reqURLStr string } tests := []struct { - name string - args args - want bool + name string + args args + want bool + wantErr bool }{ { name: "empty redirect", - args: args{redirect: ""}, + args: args{redirectURLStr: ""}, want: false, }, { name: "singleSlash", - args: args{redirect: "/redirect"}, + args: args{redirectURLStr: "/redirect"}, want: false, }, { name: "doubleSlash", - args: args{redirect: "//redirect"}, + args: args{redirectURLStr: "//redirect"}, want: false, }, { name: "validHTTP", - args: args{redirect: "http://foo.bar/redirect"}, + args: args{redirectURLStr: "http://foo.bar/redirect", reqURLStr: "http://foo.bar/"}, want: true, }, { name: "validHTTPS", - args: args{redirect: "https://foo.bar/redirect"}, + args: args{redirectURLStr: "https://foo.bar/redirect", reqURLStr: "http://foo.bar/"}, want: true, }, + { + name: "not same domain http", + args: args{redirectURLStr: "http://foo.bar/redirect", reqURLStr: "http://fake.com/"}, + want: false, + }, + { + name: "not same domain https", + args: args{redirectURLStr: "https://foo.bar/redirect", reqURLStr: "http://fake.com/"}, + want: false, + }, { name: "openRedirect1", - args: args{redirect: "/\\evil.com"}, + args: args{redirectURLStr: "/\\evil.com"}, want: false, }, { name: "openRedirectSpace1", - args: args{redirect: "/ /evil.com"}, + args: args{redirectURLStr: "/ /evil.com"}, want: false, }, { name: "openRedirectSpace2", - args: args{redirect: "/ \\evil.com"}, + args: args{redirectURLStr: "/ \\evil.com"}, want: false, }, { name: "openRedirectTab1", - args: args{redirect: "/\t/evil.com"}, + args: args{redirectURLStr: "/\t/evil.com"}, want: false, }, { name: "openRedirectTab2", - args: args{redirect: "/\t\\evil.com"}, + args: args{redirectURLStr: "/\t\\evil.com"}, want: false, }, { name: "openRedirectVerticalTab1", - args: args{redirect: "/\v/evil.com"}, + args: args{redirectURLStr: "/\v/evil.com"}, want: false, }, { name: "openRedirectVerticalTab2", - args: args{redirect: "/\v\\evil.com"}, + args: args{redirectURLStr: "/\v\\evil.com"}, want: false, }, { name: "openRedirectNewLine1", - args: args{redirect: "/\n/evil.com"}, + args: args{redirectURLStr: "/\n/evil.com"}, want: false, }, { name: "openRedirectNewLine2", - args: args{redirect: "/\n\\evil.com"}, + args: args{redirectURLStr: "/\n\\evil.com"}, want: false, }, { name: "openRedirectCarriageReturn1", - args: args{redirect: "/\r/evil.com"}, + args: args{redirectURLStr: "/\r/evil.com"}, want: false, }, { name: "openRedirectCarriageReturn2", - args: args{redirect: "/\r\\evil.com"}, + args: args{redirectURLStr: "/\r\\evil.com"}, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := isValidRedirect(tt.args.redirect); got != tt.want { + got, err := isValidRedirect(tt.args.redirectURLStr, tt.args.reqURLStr) + if (err != nil) != tt.wantErr { + t.Errorf("isValidRedirect() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { t.Errorf("isValidRedirect() = %v, want %v", got, tt.want) } })