apiserver: CVE-2022-1996, validate cors-allowed-origins server option

Kubernetes-commit: 841311ada2b0ba58e623a89e2e5ac74de0d94d8c

pkg/server/filters/cors_test.go

@@ -68,6 +68,25 @@ func TestCORSAllowedOrigins(t *testing.T) {
 			origins:        []string{"http://example.com", "example.com"},
 			allowed:        true,
 		},
+		{
+			// CVE-2022-1996: regular expression 'example.com' matches
+			// 'example.com.hacker.org' because it does not pin to the start
+			// and end of the host.
+			// The CVE should not occur in a real kubernetes cluster since we
+			// validate the regular expression specified in --cors-allowed-origins
+			// to ensure that it pins to the start and end of the host name.
+			name:           "regex does not pin, CVE-2022-1996 is not prevented",
+			allowedOrigins: []string{"example.com"},
+			origins:        []string{"http://example.com.hacker.org", "http://example.com.hacker.org:8080"},
+			allowed:        true,
+		},
+		{
+			// with a proper regular expression we can prevent CVE-2022-1996
+			name:           "regex pins to start/end of the host name, CVE-2022-1996 is prevented",
+			allowedOrigins: []string{`//example.com(:|$)`},
+			origins:        []string{"http://example.com.hacker.org", "http://example.com.hacker.org:8080"},
+			allowed:        false,
+		},
 	}
 
 	for _, test := range tests {

pkg/server/options/server_run_options.go

@@ -19,6 +19,7 @@ package options
 import (
 	"fmt"
 	"net"
+	"regexp"
 	"strings"
 	"time"
 
@@ -30,6 +31,16 @@ import (
 	"github.com/spf13/pflag"
 )
 
+const (
+	corsAllowedOriginsHelpText = "List of allowed origins for CORS, comma separated. " +
+		"An allowed origin can be a regular expression to support subdomain matching. " +
+		"If this list is empty CORS will not be enabled. " +
+		"Please ensure each expression matches the entire hostname by anchoring " +
+		"to the start with '^' or including the '//' prefix, and by anchoring to the " +
+		"end with '$' or including the ':' port separator suffix. " +
+		"Examples of valid expressions are '//example\\.com(:|$)' and '^https://example\\.com(:|$)'"
+)
+
 // ServerRunOptions contains the options while running a generic api server.
 type ServerRunOptions struct {
 	AdvertiseAddress net.IP
@@ -160,6 +171,10 @@ func (s *ServerRunOptions) Validate() []error {
 	if err := validateHSTSDirectives(s.HSTSDirectives); err != nil {
 		errors = append(errors, err)
 	}
+
+	if err := validateCorsAllowedOriginList(s.CorsAllowedOriginList); err != nil {
+		errors = append(errors, err)
+	}
 	return errors
 }
 
@@ -182,6 +197,57 @@ func validateHSTSDirectives(hstsDirectives []string) error {
 	return errors.NewAggregate(allErrors)
 }
 
+func validateCorsAllowedOriginList(corsAllowedOriginList []string) error {
+	allErrors := []error{}
+	validateRegexFn := func(regexpStr string) error {
+		if _, err := regexp.Compile(regexpStr); err != nil {
+			return err
+		}
+
+		// the regular expression should pin to the start and end of the host
+		// in the origin header, this will prevent CVE-2022-1996.
+		// possible ways it can pin to the start of host in the origin header:
+		//   - match the start of the origin with '^'
+		//   - match what separates the scheme and host with '//' or '://',
+		//     this pins to the start of host in the origin header.
+		// possible ways it can match the end of the host in the origin header:
+		//   - match the end of the origin with '$'
+		//   - with a capture group that matches the host and port separator '(:|$)'
+		// We will relax the validation to check if these regex markers
+		// are present in the user specified expression.
+		var pinStart, pinEnd bool
+		for _, prefix := range []string{"^", "//"} {
+			if strings.Contains(regexpStr, prefix) {
+				pinStart = true
+				break
+			}
+		}
+		for _, suffix := range []string{"$", ":"} {
+			if strings.Contains(regexpStr, suffix) {
+				pinEnd = true
+				break
+			}
+		}
+		if !pinStart || !pinEnd {
+			return fmt.Errorf("regular expression does not pin to start/end of host in the origin header")
+		}
+		return nil
+	}
+
+	for _, regexp := range corsAllowedOriginList {
+		if len(regexp) == 0 {
+			allErrors = append(allErrors, fmt.Errorf("empty value in --cors-allowed-origins, help: %s", corsAllowedOriginsHelpText))
+			continue
+		}
+
+		if err := validateRegexFn(regexp); err != nil {
+			err = fmt.Errorf("--cors-allowed-origins has an invalid regular expression: %v, help: %s", err, corsAllowedOriginsHelpText)
+			allErrors = append(allErrors, err)
+		}
+	}
+	return errors.NewAggregate(allErrors)
+}
+
 // AddUniversalFlags adds flags for a specific APIServer to the specified FlagSet
 func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
 	// Note: the weird ""+ in below lines seems to be the only way to get gofmt to
@@ -193,9 +259,7 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
 		"will be used. If --bind-address is unspecified, the host's default interface will "+
 		"be used.")
 
-	fs.StringSliceVar(&s.CorsAllowedOriginList, "cors-allowed-origins", s.CorsAllowedOriginList, ""+
-		"List of allowed origins for CORS, comma separated.  An allowed origin can be a regular "+
-		"expression to support subdomain matching. If this list is empty CORS will not be enabled.")
+	fs.StringSliceVar(&s.CorsAllowedOriginList, "cors-allowed-origins", s.CorsAllowedOriginList, corsAllowedOriginsHelpText)
 
 	fs.StringSliceVar(&s.HSTSDirectives, "strict-transport-security-directives", s.HSTSDirectives, ""+
 		"List of directives for HSTS, comma separated. If this list is empty, then HSTS directives will not "+

pkg/server/options/server_run_options_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package options
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -164,7 +165,7 @@ func TestServerRunOptionsValidate(t *testing.T) {
 			name: "Test when ServerRunOptions is valid",
 			testOptions: &ServerRunOptions{
 				AdvertiseAddress:            netutils.ParseIPSloppy("192.168.10.10"),
-				CorsAllowedOriginList:       []string{"10.10.10.100", "10.10.10.200"},
+				CorsAllowedOriginList:       []string{"^10.10.10.100$", "^10.10.10.200$"},
 				HSTSDirectives:              []string{"max-age=31536000", "includeSubDomains", "preload"},
 				MaxRequestsInFlight:         400,
 				MaxMutatingRequestsInFlight: 200,
@@ -189,3 +190,74 @@ func TestServerRunOptionsValidate(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateCorsAllowedOriginList(t *testing.T) {
+	tests := []struct {
+		regexp           [][]string
+		errShouldContain string
+	}{
+		{
+			regexp: [][]string{
+				{}, // empty list, the cluster operator wants to disable CORS
+				{`^http://foo.com$`},
+				{`^http://foo.com`}, // valid, because we relaxed the validation
+				{`://foo.com$`},
+				{`//foo.com$`},
+				{`^http://foo.com(:|$)`},
+				{`://foo.com(:|$)`},
+				{`//foo.com(:|$)`},
+				{`(^foo.com$)`},
+				{`^http://foo.com$`, `//bar.com(:|$)`},
+			},
+			errShouldContain: "",
+		},
+		{
+			// empty string, indicates that the cluster operator
+			// specified --cors-allowed-origins=""
+			regexp: [][]string{
+				{`^http://foo.com$`, ``},
+			},
+			errShouldContain: "empty value in --cors-allowed-origins",
+		},
+		{
+			regexp: [][]string{
+				{`^foo.com`},
+				{`//foo.com`},
+				{`foo.com$`},
+				{`foo.com(:|$)`},
+			},
+			errShouldContain: "regular expression does not pin to start/end of host in the origin header",
+		},
+		{
+			regexp: [][]string{
+				{`^http://foo.com$`, `^foo.com`}, // one good followed by a bad one
+			},
+			errShouldContain: "regular expression does not pin to start/end of host in the origin header",
+		},
+	}
+
+	for _, test := range tests {
+		for _, regexp := range test.regexp {
+			t.Run(fmt.Sprintf("regexp/%s", regexp), func(t *testing.T) {
+				options := NewServerRunOptions()
+				if errs := options.Validate(); len(errs) != 0 {
+					t.Fatalf("wrong test setup: %#v", errs)
+				}
+
+				options.CorsAllowedOriginList = regexp
+				errsGot := options.Validate()
+				switch {
+				case len(test.errShouldContain) == 0:
+					if len(errsGot) != 0 {
+						t.Errorf("expected no error, but got: %v", errsGot)
+					}
+				default:
+					if len(errsGot) == 0 ||
+						!strings.Contains(utilerrors.NewAggregate(errsGot).Error(), test.errShouldContain) {
+						t.Errorf("expected error to contain: %s, but got: %v", test.errShouldContain, errsGot)
+					}
+				}
+			})
+		}
+	}
+}