Optimise path validation (nginxinc#3094)

* update path validation
coolbry95 · Nov 18, 2022 · 3406737 · 3406737
1 parent 88fb869
commit 3406737
Show file tree

Hide file tree

Showing 3 changed files with 250 additions and 3 deletions.
diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go
@@ -72,18 +72,15 @@ const (
 const (
 	commaDelimiter     = ","
 	annotationValueFmt = `([^"$\\]|\\[^$])*`
-	pathFmt            = `/[^\s{};\\]*`
 	jwtTokenValueFmt   = "\\$" + annotationValueFmt
 )
 
 const (
 	annotationValueFmtErrMsg = `a valid annotation value must have all '"' escaped and must not contain any '$' or end with an unescaped '\'`
-	pathErrMsg               = "must start with / and must not include any whitespace character, `{`, `}` or `;`"
 	jwtTokenValueFmtErrMsg   = `a valid annotation value must start with '$', have all '"' escaped, and must not contain any '$' or end with an unescaped '\'`
 )
 
 var (
-	pathRegexp                        = regexp.MustCompile("^" + pathFmt + "$")
 	validAnnotationValueRegex         = regexp.MustCompile("^" + annotationValueFmt + "$")
 	validJWTTokenAnnotationValueRegex = regexp.MustCompile("^" + jwtTokenValueFmt + "$")
 )
@@ -875,6 +872,13 @@ func validateBackend(backend *networking.IngressBackend, fieldPath *field.Path)
 	return allErrs
 }
 
+const (
+	pathFmt    = `/[^\s;]*`
+	pathErrMsg = "must start with / and must not include any whitespace character or `;`"
+)
+
+var pathRegexp = regexp.MustCompile("^" + pathFmt + "$")
+
 func validatePath(path string, fieldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
@@ -887,6 +891,80 @@ func validatePath(path string, fieldPath *field.Path) field.ErrorList {
 		return append(allErrs, field.Invalid(fieldPath, path, msg))
 	}
 
+	allErrs = append(allErrs, validateRegexPath(path, fieldPath)...)
+	allErrs = append(allErrs, validateCurlyBraces(path, fieldPath)...)
+	allErrs = append(allErrs, validateIllegalKeywords(path, fieldPath)...)
+
+	return allErrs
+}
+
+func validateRegexPath(path string, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if _, err := regexp.Compile(path); err != nil {
+		return append(allErrs, field.Invalid(fieldPath, path, fmt.Sprintf("must be a valid regular expression: %v", err)))
+	}
+
+	if err := ValidateEscapedString(path, "*.jpg", "^/images/image_*.png$"); err != nil {
+		return append(allErrs, field.Invalid(fieldPath, path, err.Error()))
+	}
+
+	return allErrs
+}
+
+const (
+	curlyBracesFmt = `\{(.*?)\}`
+	alphabetFmt    = `[A-Za-z]`
+	curlyBracesMsg = `must not include curly braces containing alphabetical characters`
+)
+
+var (
+	curlyBracesFmtRegexp = regexp.MustCompile(curlyBracesFmt)
+	alphabetFmtRegexp    = regexp.MustCompile(alphabetFmt)
+)
+
+func validateCurlyBraces(path string, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	bracesContents := curlyBracesFmtRegexp.FindAllStringSubmatch(path, -1)
+	for _, v := range bracesContents {
+		if alphabetFmtRegexp.MatchString(v[1]) {
+			return append(allErrs, field.Invalid(fieldPath, path, curlyBracesMsg))
+		}
+	}
+	return allErrs
+}
+
+const (
+	escapedStringsFmt    = `([^"\\]|\\.)*`
+	escapedStringsErrMsg = `must have all '"' (double quotes) escaped and must not end with an unescaped '\' (backslash)`
+)
+
+var escapedStringsFmtRegexp = regexp.MustCompile("^" + escapedStringsFmt + "$")
+
+// ValidateEscapedString validates an escaped string.
+func ValidateEscapedString(body string, examples ...string) error {
+	if !escapedStringsFmtRegexp.MatchString(body) {
+		msg := validation.RegexError(escapedStringsErrMsg, escapedStringsFmt, examples...)
+		return fmt.Errorf(msg)
+	}
+	return nil
+}
+
+const (
+	illegalKeywordFmt    = `/etc/|/root|/var|\\n|\\r`
+	illegalKeywordErrMsg = `must not contain invalid paths`
+)
+
+var illegalKeywordFmtRegexp = regexp.MustCompile("^" + illegalKeywordFmt + "$")
+
+func validateIllegalKeywords(path string, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if illegalKeywordFmtRegexp.MatchString(path) {
+		return append(allErrs, field.Invalid(fieldPath, path, illegalKeywordErrMsg))
+	}
+
 	return allErrs
 }
 

diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go
@@ -3335,3 +3335,166 @@ func TestGetSpecServices(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateRegexPath(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		regexPath string
+		msg       string
+	}{
+		{
+			regexPath: "/foo.*\\.jpg",
+			msg:       "case sensitive regexp",
+		},
+		{
+			regexPath: "/Bar.*\\.jpg",
+			msg:       "case insensitive regexp",
+		},
+		{
+			regexPath: `/f\"oo.*\\.jpg`,
+			msg:       "regexp with escaped double quotes",
+		},
+		{
+			regexPath: "/[0-9a-z]{4}[0-9]+",
+			msg:       "regexp with curly braces",
+		},
+	}
+
+	for _, test := range tests {
+		allErrs := validateRegexPath(test.regexPath, field.NewPath("path"))
+		if len(allErrs) != 0 {
+			t.Errorf("validateRegexPath(%v) returned errors for valid input for the case of %v", test.regexPath, test.msg)
+		}
+	}
+}
+
+func TestValidateRegexPathFails(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		regexPath string
+		msg       string
+	}{
+		{
+			regexPath: "[{",
+			msg:       "invalid regexp",
+		},
+		{
+			regexPath: `/foo"`,
+			msg:       "unescaped double quotes",
+		},
+		{
+			regexPath: `"`,
+			msg:       "empty regex",
+		},
+		{
+			regexPath: `/foo\`,
+			msg:       "ending in backslash",
+		},
+	}
+
+	for _, test := range tests {
+		allErrs := validateRegexPath(test.regexPath, field.NewPath("path"))
+		if len(allErrs) == 0 {
+			t.Errorf("validateRegexPath(%v) returned no errors for invalid input for the case of %v", test.regexPath, test.msg)
+		}
+	}
+}
+
+func TestValidatePath(t *testing.T) {
+	t.Parallel()
+
+	validPaths := []string{
+		"/",
+		"/path",
+		"/a-1/_A/",
+		"/[A-Za-z]{6}/[a-z]{1,2}",
+		"/[0-9a-z]{4}[0-9]",
+		"/foo.*\\.jpg",
+		"/Bar.*\\.jpg",
+		`/f\"oo.*\\.jpg`,
+		"/[0-9a-z]{4}[0-9]+",
+		"/[a-z]{1,2}",
+		"/[A-Z]{6}",
+		"/[A-Z]{6}/[a-z]{1,2}",
+		"/path",
+		"/abc}{abc",
+	}
+
+	for _, path := range validPaths {
+		allErrs := validatePath(path, field.NewPath("path"))
+		if len(allErrs) > 0 {
+			t.Errorf("validatePath(%q) returned errors %v for valid input", path, allErrs)
+		}
+	}
+
+	invalidPaths := []string{
+		"",
+		" /",
+		"/ ",
+		"/abc;",
+		`/path\`,
+		`/path\n`,
+		`/var/run/secrets`,
+		"/{autoindex on; root /var/run/secrets;}location /tea",
+		"/{root}",
+	}
+
+	for _, path := range invalidPaths {
+		allErrs := validatePath(path, field.NewPath("path"))
+		if len(allErrs) == 0 {
+			t.Errorf("validatePath(%q) returned no errors for invalid input", path)
+		}
+	}
+}
+
+func TestValidateCurlyBraces(t *testing.T) {
+	t.Parallel()
+
+	validPaths := []string{
+		"/[a-z]{1,2}",
+		"/[A-Z]{6}",
+		"/[A-Z]{6}/[a-z]{1,2}",
+		"/path",
+		"/abc}{abc",
+	}
+
+	for _, path := range validPaths {
+		allErrs := validateCurlyBraces(path, field.NewPath("path"))
+		if len(allErrs) > 0 {
+			t.Errorf("validatePath(%q) returned errors %v for valid input", path, allErrs)
+		}
+	}
+
+	invalidPaths := []string{
+		"/[A-Z]{a}",
+		"/{abc}abc",
+		"/abc{a1}",
+	}
+
+	for _, path := range invalidPaths {
+		allErrs := validateCurlyBraces(path, field.NewPath("path"))
+		if len(allErrs) == 0 {
+			t.Errorf("validateCurlyBraces(%q) returned no errors for invalid input", path)
+		}
+	}
+}
+
+func TestValidateIllegalKeywords(t *testing.T) {
+	t.Parallel()
+
+	invalidPaths := []string{
+		"/root",
+		"/etc/nginx/secrets",
+		"/etc/passwd",
+		"/var/run/secrets",
+		`\n`,
+		`\r`,
+	}
+
+	for _, path := range invalidPaths {
+		allErrs := validateIllegalKeywords(path, field.NewPath("path"))
+		if len(allErrs) == 0 {
+			t.Errorf("validateCurlyBraces(%q) returned no errors for invalid input", path)
+		}
+	}
+}
diff --git a/pkg/apis/configuration/validation/virtualserver_test.go b/pkg/apis/configuration/validation/virtualserver_test.go
@@ -1465,6 +1465,10 @@ func TestValidateRegexPath(t *testing.T) {
 			regexPath: `~ ^/f\"oo.*\\.jpg`,
 			msg:       "regexp with escaped double quotes",
 		},
+		{
+			regexPath: "~ [0-9a-z]{4}[0-9]+",
+			msg:       "regexp with curly braces",
+		},
 	}
 
 	for _, test := range tests {
@@ -1526,6 +1530,8 @@ func TestValidateRoutePath(t *testing.T) {
 	invalidPaths := []string{
 		"",
 		"invalid",
+		// regex without preceding "~*" modifier
+		"^/foo.*\\.jpg",
 	}
 
 	for _, path := range invalidPaths {