From c1413e6079b0e642dcc923642b0df1c5baa29df9 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Thu, 17 Nov 2022 09:24:40 -0300 Subject: [PATCH] Validate ingress path fields (#9309) * Validate characters in path fields * Add e2e tests for path validation * Fix review comments --- internal/ingress/controller/store/store.go | 5 + pkg/util/ingress/ingress.go | 25 ++++ pkg/util/ingress/ingress_test.go | 85 +++++++++++++ test/e2e/security/invalid_paths.go | 134 +++++++++++++++++++++ 4 files changed, 249 insertions(+) create mode 100644 test/e2e/security/invalid_paths.go diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 7913eb0de..7f493bd8a 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -59,6 +59,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/resolver" "k8s.io/ingress-nginx/internal/k8s" "k8s.io/ingress-nginx/pkg/apis/ingress" + ingressutils "k8s.io/ingress-nginx/pkg/util/ingress" ) // IngressFilterFunc decides if an Ingress should be omitted or not @@ -861,6 +862,10 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { if path.Path == "" { copyIng.Spec.Rules[ri].HTTP.Paths[pi].Path = "/" } + if !ingressutils.IsSafePath(copyIng, path.Path) { + klog.Warningf("ingress %s contains invalid path %s", key, path.Path) + return + } } } diff --git a/pkg/util/ingress/ingress.go b/pkg/util/ingress/ingress.go index 7df2cc114..5fb3ee7b9 100644 --- a/pkg/util/ingress/ingress.go +++ b/pkg/util/ingress/ingress.go @@ -18,15 +18,30 @@ package ingress import ( "fmt" + "regexp" "strings" + networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/k8s" "k8s.io/ingress-nginx/internal/net/ssl" "k8s.io/ingress-nginx/pkg/apis/ingress" "k8s.io/klog/v2" ) +const ( + alphaNumericChars = `\-\.\_\~a-zA-Z0-9/` + regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+` +) + +var ( + // pathAlphaNumeric is a regex validation of something like "^/[a-zA-Z]+$" on path + pathAlphaNumeric = regexp.MustCompile("^/[" + alphaNumericChars + "]*$").MatchString + // pathRegexEnabled is a regex validation of paths that may contain regex. + pathRegexEnabled = regexp.MustCompile("^/[" + alphaNumericChars + regexEnabledChars + "]*$").MatchString +) + func GetRemovedHosts(rucfg, newcfg *ingress.Configuration) []string { oldSet := sets.NewString() newSet := sets.NewString() @@ -231,3 +246,13 @@ func BuildRedirects(servers []*ingress.Server) []*redirect { return redirectServers } + +// IsSafePath verifies if the path used in ingress object contains only valid characters. +// It will behave differently if regex is enabled or not +func IsSafePath(copyIng *networkingv1.Ingress, path string) bool { + isRegex, _ := parser.GetBoolAnnotation("use-regex", copyIng) + if isRegex { + return pathRegexEnabled(path) + } + return pathAlphaNumeric(path) +} diff --git a/pkg/util/ingress/ingress_test.go b/pkg/util/ingress/ingress_test.go index 24597fb6e..d829a57f1 100644 --- a/pkg/util/ingress/ingress_test.go +++ b/pkg/util/ingress/ingress_test.go @@ -17,8 +17,13 @@ limitations under the License. package ingress import ( + "fmt" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + networkingv1 "k8s.io/api/networking/v1" + "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/pkg/apis/ingress" ) @@ -130,3 +135,83 @@ func TestIsDynamicConfigurationEnough(t *testing.T) { t.Errorf("Expected new config to not change") } } + +func generateDumbIngressforPathTest(regexEnabled bool) *networkingv1.Ingress { + var annotations = make(map[string]string) + regexAnnotation := fmt.Sprintf("%s/use-regex", parser.AnnotationsPrefix) + if regexEnabled { + annotations[regexAnnotation] = "true" + } + return &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dumb", + Namespace: "default", + Annotations: annotations, + }, + } +} + +func TestIsSafePath(t *testing.T) { + tests := []struct { + name string + copyIng *networkingv1.Ingress + path string + want bool + }{ + { + name: "should accept valid path with regex disabled", + want: true, + copyIng: generateDumbIngressforPathTest(false), + path: "/xpto/~user/t-e_st.exe", + }, + { + name: "should accept valid path / with regex disabled", + want: true, + copyIng: generateDumbIngressforPathTest(false), + path: "/", + }, + { + name: "should reject invalid path with invalid chars", + want: false, + copyIng: generateDumbIngressforPathTest(false), + path: "/foo/bar/;xpto", + }, + { + name: "should reject regex path when regex is disabled", + want: false, + copyIng: generateDumbIngressforPathTest(false), + path: "/foo/bar/(.+)", + }, + { + name: "should accept valid path / with regex enabled", + want: true, + copyIng: generateDumbIngressforPathTest(true), + path: "/", + }, + { + name: "should accept regex path when regex is enabled", + want: true, + copyIng: generateDumbIngressforPathTest(true), + path: "/foo/bar/(.+)", + }, + { + name: "should reject regex path when regex is enabled but the path is invalid", + want: false, + copyIng: generateDumbIngressforPathTest(true), + path: "/foo/bar/;xpto", + }, + { + name: "should reject regex path when regex is enabled but the path is invalid", + want: false, + copyIng: generateDumbIngressforPathTest(true), + path: ";xpto", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsSafePath(tt.copyIng, tt.path); got != tt.want { + t.Errorf("IsSafePath() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/test/e2e/security/invalid_paths.go b/test/e2e/security/invalid_paths.go new file mode 100644 index 000000000..d75aefc2c --- /dev/null +++ b/test/e2e/security/invalid_paths.go @@ -0,0 +1,134 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package security + +import ( + "fmt" + "net/http" + "strings" + + "github.com/onsi/ginkgo/v2" + + "k8s.io/ingress-nginx/test/e2e/framework" +) + +const ( + validPath = "/xpto/~user/t-e_st.exe" + invalidPath = "/foo/bar/;xpto" + regexPath = "/foo/bar/(.+)" + host = "securitytest.com" +) + +var ( + annotationRegex = map[string]string{ + "nginx.ingress.kubernetes.io/use-regex": "true", + } +) + +var _ = framework.IngressNginxDescribe("[Security] validate path fields", func() { + f := framework.NewDefaultFramework("validate-path") + + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + }) + + ginkgo.It("should accept an ingress with valid path", func() { + + ing := framework.NewSingleIngress(host, validPath, host, f.Namespace, framework.EchoService, 80, nil) + + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.HTTPTestClient(). + GET(validPath). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + }) + + ginkgo.It("should drop an ingress with invalid path", func() { + + ing := framework.NewSingleIngress(host, invalidPath, host, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.HTTPTestClient(). + GET(invalidPath). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + }) + + ginkgo.It("should drop an ingress with regex path and regex disabled", func() { + + ing := framework.NewSingleIngress(host, regexPath, host, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.HTTPTestClient(). + GET("/foo/bar/lalala"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + }) + + ginkgo.It("should accept an ingress with regex path and regex enabled", func() { + + ing := framework.NewSingleIngress(host, regexPath, host, f.Namespace, framework.EchoService, 80, annotationRegex) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.HTTPTestClient(). + GET("/foo/bar/lalala"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + }) + + ginkgo.It("should reject an ingress with invalid path and regex enabled", func() { + + ing := framework.NewSingleIngress(host, invalidPath, host, f.Namespace, framework.EchoService, 80, annotationRegex) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.HTTPTestClient(). + GET(invalidPath). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + }) +})