Controller: Several security fixes. (#13068)

Co-authored-by: Tabitha Sable <[email protected]>

Author: Gacko

internal/ingress/annotations/auth/main.go

@@ -19,6 +19,7 @@ package auth
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strings"
 
@@ -203,16 +204,24 @@ func (a auth) Parse(ing *networking.Ingress) (interface{}, error) {
 		return nil, err
 	}
 
-	passFilename := fmt.Sprintf("%v/%v-%v-%v.passwd", a.authDirectory, ing.GetNamespace(), ing.UID, secret.UID)
+	passFileName := fmt.Sprintf("%v-%v-%v.passwd", ing.GetNamespace(), ing.UID, secret.UID)
+	passFilePath := filepath.Join(a.authDirectory, passFileName)
+
+	// Ensure password file name does not contain any path traversal characters.
+	if a.authDirectory != filepath.Dir(passFilePath) || passFileName != filepath.Base(passFilePath) {
+		return nil, ing_errors.LocationDeniedError{
+			Reason: fmt.Errorf("invalid password file name: %s", passFileName),
+		}
+	}
 
 	switch secretType {
 	case fileAuth:
-		err = dumpSecretAuthFile(passFilename, secret)
+		err = dumpSecretAuthFile(passFilePath, secret)
 		if err != nil {
 			return nil, err
 		}
 	case mapAuth:
-		err = dumpSecretAuthMap(passFilename, secret)
+		err = dumpSecretAuthMap(passFilePath, secret)
 		if err != nil {
 			return nil, err
 		}
@@ -225,9 +234,9 @@ func (a auth) Parse(ing *networking.Ingress) (interface{}, error) {
 	return &Config{
 		Type:       at,
 		Realm:      realm,
-		File:       passFilename,
+		File:       passFilePath,
 		Secured:    true,
-		FileSHA:    file.SHA1(passFilename),
+		FileSHA:    file.SHA1(passFilePath),
 		Secret:     name,
 		SecretType: secretType,
 	}, nil

internal/ingress/controller/controller.go

@@ -420,11 +420,15 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 		return err
 	}
 
+	/* Deactivated to mitigate CVE-2025-1974
+	// TODO: Implement sandboxing so this test can be done safely
 	err = n.testTemplate(content)
 	if err != nil {
 		n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
 		return err
 	}
+	*/
+
 	n.metricCollector.IncCheckCount(ing.ObjectMeta.Namespace, ing.Name)
 	endCheck := time.Now().UnixNano() / 1000000
 	n.metricCollector.SetAdmissionMetrics(

internal/ingress/controller/controller_test.go

@@ -250,6 +250,8 @@ func TestCheckIngress(t *testing.T) {
 			}
 		})
 
+		/* Deactivated to mitigate CVE-2025-1974
+		// TODO: Implement sandboxing so this test can be done safely
 		t.Run("When nginx test returns an error", func(t *testing.T) {
 			nginx.command = testNginxTestCommand{
 				t:        t,
@@ -261,6 +263,7 @@ func TestCheckIngress(t *testing.T) {
 				t.Errorf("with a new ingress with an error, an error should be returned")
 			}
 		})
+		*/
 
 		t.Run("When the default annotation prefix is used despite an override", func(t *testing.T) {
 			defer func() {

internal/ingress/controller/template/template.go

@@ -1622,11 +1622,11 @@ func buildMirrorLocations(locs []*ingress.Location) string {
 		mapped.Insert(loc.Mirror.Source)
 		buffer.WriteString(fmt.Sprintf(`location = %v {
 internal;
-proxy_set_header Host "%v";
-proxy_pass "%v";
+proxy_set_header Host %v;
+proxy_pass %v;
 }
 
-`, loc.Mirror.Source, loc.Mirror.Host, loc.Mirror.Target))
+`, strconv.Quote(loc.Mirror.Source), strconv.Quote(loc.Mirror.Host), strconv.Quote(loc.Mirror.Target)))
 	}
 
 	return buffer.String()

rootfs/etc/nginx/template/nginx.tmpl

@@ -879,7 +879,7 @@ stream {
 
         {{ if not ( empty $server.CertificateAuth.MatchCN ) }}
         {{ if gt (len $server.CertificateAuth.MatchCN) 0 }}
-        if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN }} ) {
+        if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN | quote }} ) {
             return 403 "client certificate unauthorized";
         }
         {{ end }}
@@ -1082,7 +1082,7 @@ stream {
             set $target {{ changeHostPort $externalAuth.URL $authUpstreamName }};
             {{ else }}
             proxy_http_version {{ $location.Proxy.ProxyHTTPVersion }};
-            set $target {{ $externalAuth.URL }};
+            set $target {{ $externalAuth.URL | quote }};
             {{ end }}
             proxy_pass $target;
         }
@@ -1120,7 +1120,7 @@ stream {
             {{ buildOpentelemetryForLocation $all.Cfg.EnableOpentelemetry $all.Cfg.OpentelemetryTrustIncomingSpan $location }}
 
             {{ if $location.Mirror.Source }}
-            mirror {{ $location.Mirror.Source }};
+            mirror {{ $location.Mirror.Source | quote }};
             mirror_request_body {{ $location.Mirror.RequestBody }};
             {{ end }}
 

test/e2e/admission/admission.go

@@ -26,7 +26,6 @@ import (
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/assert"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/ingress-nginx/test/e2e/framework"
@@ -99,6 +98,8 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
 		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid path should return an error")
 	})
 
+	/* Deactivated to mitigate CVE-2025-1974
+	// TODO: Implement sandboxing so this test can be done safely
 	ginkgo.It("should return an error if there is an error validating the ingress definition", func() {
 		disableSnippet := f.AllowSnippetConfiguration()
 		defer disableSnippet()
@@ -112,6 +113,7 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
 		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
 		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 := admissionTestHost
@@ -207,6 +209,8 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
 			Status(http.StatusOK)
 	})
 
+	/* Deactivated to mitigate CVE-2025-1974
+	// TODO: Implement sandboxing so this test can be done safely
 	ginkgo.It("should return an error if the Ingress V1 definition contains invalid annotations", func() {
 		disableSnippet := f.AllowSnippetConfiguration()
 		defer disableSnippet()
@@ -220,6 +224,7 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
 			assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
 		}
 	})
+	*/
 
 	ginkgo.It("should not return an error for an invalid Ingress when it has unknown class", func() {
 		disableSnippet := f.AllowSnippetConfiguration()

test/e2e/annotations/mirror.go

@@ -43,7 +43,7 @@ var _ = framework.DescribeAnnotation("mirror-*", func() {
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) &&
+				return strings.Contains(server, fmt.Sprintf("mirror \"/_mirror-%v\";", ing.UID)) &&
 					strings.Contains(server, "mirror_request_body on;")
 			})
 	})
@@ -58,7 +58,7 @@ var _ = framework.DescribeAnnotation("mirror-*", func() {
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) &&
+				return strings.Contains(server, fmt.Sprintf("mirror \"/_mirror-%v\";", ing.UID)) &&
 					strings.Contains(server, "mirror_request_body on;") &&
 					strings.Contains(server, `proxy_pass "https://test.env.com/$request_uri";`)
 			})
@@ -75,7 +75,7 @@ var _ = framework.DescribeAnnotation("mirror-*", func() {
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) &&
+				return strings.Contains(server, fmt.Sprintf("mirror \"/_mirror-%v\";", ing.UID)) &&
 					strings.Contains(server, "mirror_request_body off;")
 			})
 	})