Add option to sanitize annotation inputs

internal/ingress/annotations/parser/main_test.go

@@ -116,6 +116,12 @@ rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/serv
 			}
 			continue
 		}
+		if !test.expErr {
+			if err != nil {
+				t.Errorf("%v: didn't expected error but error was returned: %v", test.name, err)
+			}
+			continue
+		}
 		if s != test.exp {
 			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s)
 		}

internal/ingress/controller/config/config.go

@@ -97,6 +97,14 @@ type Configuration struct {
 	// If disabled, only snippets added via ConfigMap are added to ingress.
 	AllowSnippetAnnotations bool `json:"allow-snippet-annotations"`
 
+	// AnnotationValueCharBlocklist defines characters that should not be part of an user annotation value
+	// (can be used to escape strings, for example) and that should be dropped
+	AnnotationValueCharBlocklist []string `json:"annotation-value-char-blocklist"`
+
+	// AnnotationValueWordBlocklist defines words that should not be part of an user annotation value
+	// (can be used to run arbitrary code or configs, for example) and that should be dropped
+	AnnotationValueWordBlocklist []string `json:"annotation-value-word-blocklist"`
+
 	// Sets the name of the configmap that contains the headers to pass to the client
 	AddHeaders string `json:"add-headers,omitempty"`
 
@@ -754,6 +762,9 @@ func NewDefault() Configuration {
 	defNginxStatusIpv6Whitelist := make([]string, 0)
 	defResponseHeaders := make([]string, 0)
 
+	defAnnotationValueWordBlocklist := []string{"load_module", "lua_package", "_by_lua", "location", "root"}
+	defAnnotationValueCharBlocklist := []string{"{", "}", "'", "\"", "\\"}
+
 	defIPCIDR = append(defIPCIDR, "0.0.0.0/0")
 	defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1")
 	defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1")
@@ -764,6 +775,8 @@ func NewDefault() Configuration {
 
 		AllowSnippetAnnotations:          true,
 		AllowBackendServerHeader:         false,
+		AnnotationValueWordBlocklist:     defAnnotationValueWordBlocklist,
+		AnnotationValueCharBlocklist:     defAnnotationValueCharBlocklist,
 		AccessLogPath:                    "/var/log/nginx/access.log",
 		AccessLogParams:                  "",
 		EnableAccessLogForDefaultBackend: false,

internal/ingress/controller/controller.go

@@ -237,12 +237,25 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 	cfg := n.store.GetBackendConfiguration()
 	cfg.Resolver = n.resolver
 
-	for key := range ing.ObjectMeta.GetAnnotations() {
+	for key, value := range ing.ObjectMeta.GetAnnotations() {
+
 		if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
 			if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
 				return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix)
 			}
 		}
+		if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
+			for _, forbiddenvalue := range cfg.AnnotationValueWordBlocklist {
+				if strings.Contains(value, forbiddenvalue) {
+					return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
+				}
+			}
+			for _, invalidchar := range cfg.AnnotationValueCharBlocklist {
+				if strings.ContainsAny(value, invalidchar) {
+					return fmt.Errorf("%s annotation contains invalid character %s", key, invalidchar)
+				}
+			}
+		}
 
 		if !cfg.AllowSnippetAnnotations && strings.HasSuffix(key, "-snippet") {
 			return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key)

internal/ingress/controller/controller_test.go

@@ -268,6 +268,40 @@ func TestCheckIngress(t *testing.T) {
 			}
 		})
 
+		t.Run("When invalid directives are used in annotation values", func(t *testing.T) {
+			nginx.store = fakeIngressStore{
+				ingresses: []*ingress.Ingress{},
+				configuration: ngx_config.Configuration{
+					AnnotationValueWordBlocklist: []string{"invalid_directive"},
+				},
+			}
+			nginx.command = testNginxTestCommand{
+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "invalid_directive"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with an invalid value in annotation the ingress should be rejected")
+			}
+		})
+
+		t.Run("When invalid chars are used in annotation values", func(t *testing.T) {
+			nginx.store = fakeIngressStore{
+				ingresses: []*ingress.Ingress{},
+				configuration: ngx_config.Configuration{
+					AnnotationValueCharBlocklist: []string{"{", "}"},
+				},
+			}
+			nginx.command = testNginxTestCommand{
+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "{differentheader"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with an invalid chars in annotation the ingress should be rejected")
+			}
+		})
+
 		t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) {
 			backendBefore := ing.Spec.DefaultBackend
 			disableCatchAllBefore := nginx.cfg.DisableCatchAll

internal/ingress/controller/store/store.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"reflect"
 	"sort"
+	"strings"
 	"sync"
 	"time"
 
@@ -734,6 +735,24 @@ func hasCatchAllIngressRule(spec networkingv1.IngressSpec) bool {
 	return spec.DefaultBackend != nil
 }
 
+func checkBadAnnotationValue(annotations map[string]string, badwords, badchars []string) error {
+	for annotation, value := range annotations {
+		if strings.HasPrefix(annotation, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
+			for _, forbiddenvalue := range badwords {
+				if strings.Contains(value, forbiddenvalue) {
+					return fmt.Errorf("%s annotation contains invalid word %s", annotation, forbiddenvalue)
+				}
+			}
+			for _, invalidchar := range badchars {
+				if strings.ContainsAny(value, invalidchar) {
+					return fmt.Errorf("%s annotation contains invalid character %s", annotation, invalidchar)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 // syncIngress parses ingress annotations converting the value of the
 // annotation to a go struct
 func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
@@ -742,6 +761,12 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
 
 	copyIng := &networkingv1.Ingress{}
 	ing.ObjectMeta.DeepCopyInto(&copyIng.ObjectMeta)
+
+	if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist, s.backendConfig.AnnotationValueCharBlocklist); err != nil {
+		klog.Error("skipping ingress %v: %s", err)
+		return
+	}
+
 	ing.Spec.DeepCopyInto(&copyIng.Spec)
 	ing.Status.DeepCopyInto(&copyIng.Status)
 

test/e2e/admission/admission.go

@@ -121,6 +121,28 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
 		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
 	})
 
+	ginkgo.It("should return an error if there is an invalid value in some annotation", func() {
+		host := "admission-test"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/connection-proxy-header": "a;}",
+		}
+		firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error")
+	})
+
+	ginkgo.It("should return an error if there is a forbidden value in some annotation", func() {
+		host := "admission-test"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/connection-proxy-header": "set_by_lua",
+		}
+		firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error")
+	})
+
 	ginkgo.It("should not return an error if the Ingress V1 definition is valid with Ingress Class", func() {
 		err := createIngress(f.Namespace, validV1Ingress)
 		assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress using kubectl")

test/e2e/framework/util.go

@@ -42,7 +42,7 @@ const (
 	Poll = 2 * time.Second
 
 	// DefaultTimeout time to wait for operations to complete
-	DefaultTimeout = 5 * time.Minute
+	DefaultTimeout = 30 * time.Second
 )
 
 func nowStamp() string {

test/e2e/settings/badannotationvalues.go

@@ -0,0 +1,129 @@
+/*
+Copyright 2021 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 settings
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/onsi/ginkgo"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
+
+var _ = framework.DescribeAnnotation("Bad annotation values", func() {
+	f := framework.NewDefaultFramework("bad-annotation")
+
+	ginkgo.BeforeEach(func() {
+		f.NewEchoDeployment()
+	})
+
+	ginkgo.It("should drop an ingress if there is an invalid character in some annotation", func() {
+		host := "invalid-value-test"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/configuration-snippet": `
+			# abc { }`,
+		}
+
+		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+			})
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, "# abc { }")
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusNotFound)
+	})
+
+	ginkgo.It("should drop an ingress if there is a forbidden word in some annotation", func() {
+		host := "forbidden-value-test"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/configuration-snippet": `
+			  default_type text/plain;
+              content_by_lua_block {
+                ngx.say("Hello World")
+            }`,
+		}
+
+		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+			})
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, `ngx.say("Hello World")`)
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusNotFound)
+	})
+
+	ginkgo.It("should drop an ingress if there is a custom blocklisted word in some annotation", func() {
+		host := "custom-forbidden-value-test"
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/configuration-snippet": `
+			# something_forbidden`,
+		}
+
+		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+			})
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, "# something_forbidden")
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusNotFound)
+	})
+})