Skip to content

Commit e6716b1

Browse files
Gackotabbysable
Gacko
and
tabbysable
authored
Controller: Several security fixes. (#13070)
Co-authored-by: Tabitha Sable <tabitha.c.sable@gmail.com>
1 parent cd854d9 commit e6716b1

File tree

7 files changed

+36
-15
lines changed

7 files changed

+36
-15
lines changed

‎internal/ingress/annotations/auth/main.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package auth
1919
import (
2020
"fmt"
2121
"os"
22+
"path/filepath"
2223
"regexp"
2324
"strings"
2425

@@ -203,16 +204,24 @@ func (a auth) Parse(ing *networking.Ingress) (interface{}, error) {
203204
return nil, err
204205
}
205206

206-
passFilename := fmt.Sprintf("%v/%v-%v-%v.passwd", a.authDirectory, ing.GetNamespace(), ing.UID, secret.UID)
207+
passFileName := fmt.Sprintf("%v-%v-%v.passwd", ing.GetNamespace(), ing.UID, secret.UID)
208+
passFilePath := filepath.Join(a.authDirectory, passFileName)
209+
210+
// Ensure password file name does not contain any path traversal characters.
211+
if a.authDirectory != filepath.Dir(passFilePath) || passFileName != filepath.Base(passFilePath) {
212+
return nil, ing_errors.LocationDeniedError{
213+
Reason: fmt.Errorf("invalid password file name: %s", passFileName),
214+
}
215+
}
207216

208217
switch secretType {
209218
case fileAuth:
210-
err = dumpSecretAuthFile(passFilename, secret)
219+
err = dumpSecretAuthFile(passFilePath, secret)
211220
if err != nil {
212221
return nil, err
213222
}
214223
case mapAuth:
215-
err = dumpSecretAuthMap(passFilename, secret)
224+
err = dumpSecretAuthMap(passFilePath, secret)
216225
if err != nil {
217226
return nil, err
218227
}
@@ -225,9 +234,9 @@ func (a auth) Parse(ing *networking.Ingress) (interface{}, error) {
225234
return &Config{
226235
Type: at,
227236
Realm: realm,
228-
File: passFilename,
237+
File: passFilePath,
229238
Secured: true,
230-
FileSHA: file.SHA1(passFilename),
239+
FileSHA: file.SHA1(passFilePath),
231240
Secret: name,
232241
SecretType: secretType,
233242
}, nil

‎internal/ingress/controller/controller.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,11 +421,15 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
421421
return err
422422
}
423423

424+
/* Deactivated to mitigate CVE-2025-1974
425+
// TODO: Implement sandboxing so this test can be done safely
424426
err = n.testTemplate(content)
425427
if err != nil {
426428
n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
427429
return err
428430
}
431+
*/
432+
429433
n.metricCollector.IncCheckCount(ing.ObjectMeta.Namespace, ing.Name)
430434
endCheck := time.Now().UnixNano() / 1000000
431435
n.metricCollector.SetAdmissionMetrics(

‎internal/ingress/controller/controller_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,8 @@ func TestCheckIngress(t *testing.T) {
250250
}
251251
})
252252

253+
/* Deactivated to mitigate CVE-2025-1974
254+
// TODO: Implement sandboxing so this test can be done safely
253255
t.Run("When nginx test returns an error", func(t *testing.T) {
254256
nginx.command = testNginxTestCommand{
255257
t: t,
@@ -261,6 +263,7 @@ func TestCheckIngress(t *testing.T) {
261263
t.Errorf("with a new ingress with an error, an error should be returned")
262264
}
263265
})
266+
*/
264267

265268
t.Run("When the default annotation prefix is used despite an override", func(t *testing.T) {
266269
defer func() {

‎internal/ingress/controller/template/template.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1645,11 +1645,11 @@ func buildMirrorLocations(locs []*ingress.Location) string {
16451645
mapped.Insert(loc.Mirror.Source)
16461646
buffer.WriteString(fmt.Sprintf(`location = %v {
16471647
internal;
1648-
proxy_set_header Host "%v";
1649-
proxy_pass "%v";
1648+
proxy_set_header Host %v;
1649+
proxy_pass %v;
16501650
}
16511651
1652-
`, loc.Mirror.Source, loc.Mirror.Host, loc.Mirror.Target))
1652+
`, strconv.Quote(loc.Mirror.Source), strconv.Quote(loc.Mirror.Host), strconv.Quote(loc.Mirror.Target)))
16531653
}
16541654

16551655
return buffer.String()

‎rootfs/etc/nginx/template/nginx.tmpl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,7 +1013,7 @@ stream {
10131013

10141014
{{ if not ( empty $server.CertificateAuth.MatchCN ) }}
10151015
{{ if gt (len $server.CertificateAuth.MatchCN) 0 }}
1016-
if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN }} ) {
1016+
if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN | quote }} ) {
10171017
return 403 "client certificate unauthorized";
10181018
}
10191019
{{ end }}
@@ -1219,7 +1219,7 @@ stream {
12191219
set $target {{ changeHostPort $externalAuth.URL $authUpstreamName }};
12201220
{{ else }}
12211221
proxy_http_version {{ $location.Proxy.ProxyHTTPVersion }};
1222-
set $target {{ $externalAuth.URL }};
1222+
set $target {{ $externalAuth.URL | quote }};
12231223
{{ end }}
12241224
proxy_pass $target;
12251225
}
@@ -1258,7 +1258,7 @@ stream {
12581258
{{ buildOpentelemetryForLocation $all.Cfg.EnableOpentelemetry $all.Cfg.OpentelemetryTrustIncomingSpan $location }}
12591259

12601260
{{ if $location.Mirror.Source }}
1261-
mirror {{ $location.Mirror.Source }};
1261+
mirror {{ $location.Mirror.Source | quote }};
12621262
mirror_request_body {{ $location.Mirror.RequestBody }};
12631263
{{ end }}
12641264

‎test/e2e/admission/admission.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626

2727
"github.com/onsi/ginkgo/v2"
2828
"github.com/stretchr/testify/assert"
29-
apierrors "k8s.io/apimachinery/pkg/api/errors"
3029
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3130

3231
"k8s.io/ingress-nginx/test/e2e/framework"
@@ -126,6 +125,8 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
126125
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid path should return an error")
127126
})
128127

128+
/* Deactivated to mitigate CVE-2025-1974
129+
// TODO: Implement sandboxing so this test can be done safely
129130
ginkgo.It("should return an error if there is an error validating the ingress definition", func() {
130131
f.SetNginxConfigMapData(map[string]string{
131132
"allow-snippet-annotations": "true",
@@ -145,6 +146,7 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
145146
_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
146147
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
147148
})
149+
*/
148150

149151
ginkgo.It("should return an error if there is an invalid value in some annotation", func() {
150152
host := admissionTestHost
@@ -240,6 +242,8 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
240242
Status(http.StatusOK)
241243
})
242244

245+
/* Deactivated to mitigate CVE-2025-1974
246+
// TODO: Implement sandboxing so this test can be done safely
243247
ginkgo.It("should return an error if the Ingress V1 definition contains invalid annotations", func() {
244248
f.SetNginxConfigMapData(map[string]string{
245249
"allow-snippet-annotations": "true",
@@ -259,6 +263,7 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
259263
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
260264
}
261265
})
266+
*/
262267

263268
ginkgo.It("should not return an error for an invalid Ingress when it has unknown class", func() {
264269
f.SetNginxConfigMapData(map[string]string{

‎test/e2e/annotations/mirror.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ var _ = framework.DescribeAnnotation("mirror-*", func() {
4343

4444
f.WaitForNginxServer(host,
4545
func(server string) bool {
46-
return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) &&
46+
return strings.Contains(server, fmt.Sprintf("mirror \"/_mirror-%v\";", ing.UID)) &&
4747
strings.Contains(server, "mirror_request_body on;")
4848
})
4949
})
@@ -58,7 +58,7 @@ var _ = framework.DescribeAnnotation("mirror-*", func() {
5858

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

7676
f.WaitForNginxServer(host,
7777
func(server string) bool {
78-
return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) &&
78+
return strings.Contains(server, fmt.Sprintf("mirror \"/_mirror-%v\";", ing.UID)) &&
7979
strings.Contains(server, "mirror_request_body off;")
8080
})
8181
})

0 commit comments

Comments
 (0)