fix(authx:authentication): Validate redirect hostname to avoid redire…

…ct injection
oxyno-zeta · Mar 24, 2024 · 9df6a1e · 9df6a1e
1 parent 1a773cd
commit 9df6a1e
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 29 deletions.
diff --git 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
@@ -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)
 			}
 		})