Skip to content

Controller: Several security fixes. #13070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions internal/ingress/annotations/auth/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package auth
import (
"fmt"
"os"
"path/filepath"
"regexp"
"strings"

Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,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(
Expand Down
3 changes: 3 additions & 0 deletions internal/ingress/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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() {
Expand Down
6 changes: 3 additions & 3 deletions internal/ingress/controller/template/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -1645,11 +1645,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()
Expand Down
6 changes: 3 additions & 3 deletions rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,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 }}
Expand Down Expand Up @@ -1219,7 +1219,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;
}
Expand Down Expand Up @@ -1258,7 +1258,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 }}

Expand Down
7 changes: 6 additions & 1 deletion test/e2e/admission/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -126,6 +125,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() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "true",
Expand All @@ -145,6 +146,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
Expand Down Expand Up @@ -240,6 +242,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() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "true",
Expand All @@ -259,6 +263,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() {
f.SetNginxConfigMapData(map[string]string{
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/annotations/mirror.go
Original file line number Diff line number Diff line change
Expand Up @@ -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;")
})
})
Expand All @@ -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";`)
})
Expand All @@ -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;")
})
})
Expand Down