Path Traversal

PoC (demonstrates edge-case path traversal gated by rewrite targets): 1) Configure Traefik with a rewrite target that uses a capture group, e.g.: - Regex: ^/foo(.*)$ - Replacement: "/$1" 2) Send a request that attempts dot-segment traversal within the capture: curl -sS http://localhost:8080/foo../bar 3) Expected outcome after this fix: Traefik responds with 400 Bad Request because the normalized path after applying JoinPath() no longer matches the original path ("/foo../bar" vs. the normalized "/../bar"). 4) Prior to this fix (without the normalization guard), the rewritten path might reach the upstream as a dot-segment traversal (e.g., "//../bar" or similar), potentially exposing restricted resources depending on upstream handling. This PoC assumes a backend that would treat the rewritten path in a way that dot-segment traversal could affect security if not blocked at the edge. Notes: - The exact backend behavior depends on the upstream service; the security fix ensures Traefik rejects such inputs at the edge instead of forwarding a potentially dangerous path. - The tests in the commit explicitly cover these traversal scenarios to validate the fix.

diff --git a/cmd/traefik/traefik.go b/cmd/traefik/traefik.go index 11c8c3a1e70..5226ea53aab 100644 --- a/cmd/traefik/traefik.go +++ b/cmd/traefik/traefik.go @@ -386,7 +386,7 @@ func setupServer(staticConfiguration *static.Configuration) (*server.Server, err } if _, ok := resolverNames[rt.TLS.CertResolver]; !ok { - log.Error().Err(err).Str(logs.RouterName, rtName).Str("certificateResolver", rt.TLS.CertResolver). + log.Error().Str(logs.RouterName, rtName).Str("certificateResolver", rt.TLS.CertResolver). Msg("Router uses a nonexistent certificate resolver") } } diff --git a/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target.go b/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target.go index 577b7ee58db..1cb6da4d361 100644 --- a/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target.go +++ b/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target.go @@ -152,6 +152,20 @@ func (rt *rewriteTarget) ServeHTTP(rw http.ResponseWriter, req *http.Request) { return } + // Here we are sanitizing the URL when the path is not empty, + // as the JoinPath method is adding a leading slash if the path is empty. + path := req.URL.Path + if path != "" { + req.URL = req.URL.JoinPath() + } + + // Stop here if the normalization of the path produces a different path. + if path != req.URL.Path { + logger.Debug().Msgf("Rejecting request, sanitized path: %q is not equivalent to stripped path: %q", path, req.URL.Path) + http.Error(rw, http.StatusText(http.StatusBadRequest), http.StatusBadRequest) + return + } + req.RequestURI = req.URL.RequestURI() rt.next.ServeHTTP(rw, req) diff --git a/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target_test.go b/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target_test.go index 1b5a18c2c5b..3f1e016cfe3 100644 --- a/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target_test.go +++ b/pkg/middlewares/ingressnginx/rewritetarget/rewrite_target_test.go @@ -43,8 +43,7 @@ func TestRewriteTarget(t *testing.T) { config: dynamic.RewriteTarget{ Replacement: "/replacement", }, - expectedPath: "/replacement", - expectedRawPath: "/replacement", + expectedPath: "/replacement", }, { desc: "plain replacement with escaped char in replacement", @@ -63,7 +62,6 @@ func TestRewriteTarget(t *testing.T) { XForwardedPrefix: "/foo", }, expectedPath: "/replacement", - expectedRawPath: "/replacement", expectedXForwardedPrefix: "/foo", }, { @@ -73,8 +71,7 @@ func TestRewriteTarget(t *testing.T) { Regex: `^/foo/(.*)`, Replacement: "/new/$1", }, - expectedPath: "/new/bar", - expectedRawPath: "/new/bar", + expectedPath: "/new/bar", }, { desc: "regex with multiple capture groups", @@ -83,8 +80,7 @@ func TestRewriteTarget(t *testing.T) { Regex: `^(?i)/downloads/([^/]+)/([^/]+)$`, Replacement: "/downloads/$1-$2", }, - expectedPath: "/downloads/src-source.go", - expectedRawPath: "/downloads/src-source.go", + expectedPath: "/downloads/src-source.go", }, { desc: "regex with escaped char in replacement", @@ -115,7 +111,6 @@ func TestRewriteTarget(t *testing.T) { XForwardedPrefix: "$1", }, expectedPath: "/bar", - expectedRawPath: "/bar", expectedXForwardedPrefix: "/foo", }, { @@ -127,7 +122,6 @@ func TestRewriteTarget(t *testing.T) { XForwardedPrefix: "/$1/$2", }, expectedPath: "/endpoint", - expectedRawPath: "/endpoint", expectedXForwardedPrefix: "/prefix/sub", }, { @@ -178,9 +172,7 @@ func TestRewriteTarget(t *testing.T) { Regex: `^/prefix/(.*)`, Replacement: "$1", }, - expectedPath: "http://evil.com/malicious", - expectedRawPath: "http://evil.com/malicious", - expectedStatusCode: http.StatusOK, + expectedStatusCode: http.StatusBadRequest, }, { desc: "regex with full URL replacement with no capture groups", @@ -220,6 +212,24 @@ func TestRewriteTarget(t *testing.T) { expectedStatusCode: http.StatusFound, expectedRedirectURL: "https://portal.example.org/site/foo/bar?tenant=acme&id=42", }, + { + desc: "path with ..", + path: "/foo../bar", + config: dynamic.RewriteTarget{ + Regex: `^/foo(.*)`, + Replacement: "/$1", + }, + expectedStatusCode: http.StatusBadRequest, + }, + { + desc: "capture group without path separator introducing dot-segment traversal", + path: "/api../admin", + config: dynamic.RewriteTarget{ + Regex: `^/api(.*)`, + Replacement: "/$1", + }, + expectedStatusCode: http.StatusBadRequest, + }, } for _, test := range testCases { diff --git a/pkg/middlewares/ingressnginx/snippet/action.go b/pkg/middlewares/ingressnginx/snippet/action.go index 8b2a9df1e7e..bbdee886dff 100644 --- a/pkg/middlewares/ingressnginx/snippet/action.go +++ b/pkg/middlewares/ingressnginx/snippet/action.go @@ -795,6 +795,18 @@ func createRewriteAction(d config.IDirective) (action, error) { // Otherwise, keep the original query string. } + // Here we are sanitizing the URL when the path is not empty, + // as the JoinPath method is adding a leading slash if the path is empty. + rewrittenPath := req.URL.Path + if rewrittenPath != "" { + req.URL = req.URL.JoinPath() + } + // Stop here if the normalization of the path produces a different path. + if rewrittenPath != req.URL.Path { + ctx.statusCode = http.StatusBadRequest + return true, nil + } + req.RequestURI = req.URL.RequestURI() // In NGINX, last restarts location matching while break stays in the diff --git a/pkg/middlewares/ingressnginx/snippet/snippet_test.go b/pkg/middlewares/ingressnginx/snippet/snippet_test.go index b325c38a719..5f8c6707fdd 100644 --- a/pkg/middlewares/ingressnginx/snippet/snippet_test.go +++ b/pkg/middlewares/ingressnginx/snippet/snippet_test.go @@ -1141,6 +1141,30 @@ rewrite ^/(.*)$ "${uri}?" break; expectedPath: "/some/path", expectedQuery: "", }, + { + desc: "rewrite with dot-segment traversal in capture group is rejected", + configurationSnippet: ` +rewrite ^/foo(.*)$ /$1 break; +`, + path: "/foo../bar", + expectedStatusCode: http.StatusBadRequest, + }, + { + desc: "rewrite with capture group lacking path separator introducing dot-segment traversal is rejected", + configurationSnippet: ` +rewrite ^/api(.*)$ /$1 break; +`, + path: "/api../admin", + expectedStatusCode: http.StatusBadRequest, + }, + { + desc: "rewrite with percent-encoded character in capture group succeeds", + configurationSnippet: ` +rewrite ^/api/(.*)$ /v2/$1 break; +`, + path: "/api/foo%2Fbar", + expectedPath: "/v2/foo/bar", + }, // --- add_header always tests --- { desc: "add_header with always applies to 200 status", diff --git a/pkg/middlewares/retry/retry.go b/pkg/middlewares/retry/retry.go index 9d8ca2bfcdc..cd712edfbf3 100644 --- a/pkg/middlewares/retry/retry.go +++ b/pkg/middlewares/retry/retry.go @@ -230,8 +230,9 @@ func (r *retry) ServeHTTP(rw http.ResponseWriter, req *http.Request) { r.next.ServeHTTP(retryResponseWriter, retryReq) - // End the operation as soon as the client received something. - if retryResponseWriter.written { + // End the operation as soon as the client received something + // or when the request is hijacked. + if retryResponseWriter.written || retryResponseWriter.hijacked { return nil } @@ -298,6 +299,7 @@ type responseWriter struct { proxyReached atomic.Bool wroteRequest atomic.Bool + hijacked bool written bool shouldNotWrite bool } @@ -371,6 +373,11 @@ func (r *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { if !ok { return nil, nil, fmt.Errorf("%T is not a http.Hijacker", r.responseWriter) } + + // When the connection is Hijacked when using Websockets, we don't want to retry anymore, + // so we set the hijacked flag to true. + r.hijacked = true + return hijacker.Hijack() } diff --git a/pkg/middlewares/retry/retry_test.go b/pkg/middlewares/retry/retry_test.go index 14751496425..a20d8781ffb 100644 --- a/pkg/middlewares/retry/retry_test.go +++ b/pkg/middlewares/retry/retry_test.go @@ -7,8 +7,11 @@ import ( "net/http" "net/http/httptest" "net/http/httptrace" + "net/http/httputil" "net/textproto" + "net/url" "strings" + "sync/atomic" "testing" "time" @@ -313,6 +316,20 @@ func TestRetryWebsocket(t *testing.T) { amountFaultyEndpoints: 2, expectedResponseStatus: http.StatusSwitchingProtocols, }, + { + desc: "Switching ok on the first attempt is not retried", + maxRequestAttempts: 3, + expectedRetryAttempts: 0, + amountFaultyEndpoints: 0, + expectedResponseStatus: http.StatusSwitchingProtocols, + }, + { + desc: "Switching ok on an intermediate attempt is not retried", + maxRequestAttempts: 3, + expectedRetryAttempts: 1, + amountFaultyEndpoints: 1, + expectedResponseStatus: http.StatusSwitchingProtocols, + }, { desc: "Switching failed", maxRequestAttempts: 2, @@ -858,3 +875,72 @@ func TestRetryDoesNotBufferBodyForNonIdempotentMethod(t *testing.T) { assert.Equal(t, http.StatusOK, recorder.Code) assert.Equal(t, 0, retryListener.timesCalled) } + +// TestRetryWebsocketDelayedFlush reproduces https://github.com/traefik/traefik/issues/13513. +// +// httputil.ReverseProxy serves a protocol upgrade by hijacking the connection and writing the 101 response directly to +// it, so WriteHeader is never called on the retry responseWriter and written stays false. +// When the client goes away, the retry middleware therefore replays the request, this time getting a regular +// Content-Length response, which makes copyResponse arm the maxLatencyWriter flush timer. +// The delayed flush then flushes an http.Response whose buffered writer has been released by Hijack. +// +// Until the retry middleware stops replaying hijacked requests, this does not report a test failure: +// it panics in a timer goroutine, which takes the whole test binary down with a SIGSEGV. +func TestRetryWebsocketDelayedFlush(t *testing.T) { + var backendCallCount atomic.Int32 + backend := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + if backendCallCount.Add(1) == 1 { + upgrader := websocket.Upgrader{} + conn, err := upgrader.Upgrade(rw, req, nil) + require.NoError(t, err) + + defer conn.Close() + + // Hold the stream until the client goes away. + _, _, _ = conn.ReadMessage() + + return + } + + // The replayed request no longer matches a live stream, and the backend answers with a regular response. + // A known Content-Length is what makes ReverseProxy arm the flush timer instead of flushing inline. + rw.Header().Set("Content-Length", "10") + rw.WriteHeader(http.StatusOK) + rw.(http.Flusher).Flush() + + // Give the flush timer time to fire before the body completes the copy. + time.Sleep(time.Second) + + _, _ = rw.Write([]byte("0123456789")) + })) + t.Cleanup(backend.Close) + + backendURL, err := url.Parse(backend.URL) + require.NoError(t, err) + + proxy := &httputil.ReverseProxy{ + FlushInterval: 100 * time.Millisecond, + Rewrite: func(pr *httputil.ProxyRequest) { + pr.Out.URL.Host = backendURL.Host + pr.Out.URL.Scheme = backendURL.Scheme + }, + } + + handler, err := New(t.Context(), WrapHandler(proxy), dynamic.Retry{Attempts: 3}, &countingRetryListener{}, "traefikTest") + require.NoError(t, err) + + retryServer := httptest.NewServer(handler) + t.Cleanup(retryServer.Close) + + conn, response, err := websocket.DefaultDialer.Dial(strings.Replace(retryServer.URL, "http", "ws", 1), nil) + require.NoError(t, err) + require.Equal(t, http.StatusSwitchingProtocols, response.StatusCode) + + // The client goes away: handleUpgradeResponse returns, and the retry middleware replays the request. + require.NoError(t, conn.Close()) + + // Leave the replayed attempt time to arm and fire the delayed flush. + time.Sleep(time.Second) + + assert.Equal(t, int32(1), backendCallCount.Load(), "an upgraded request must not be replayed") +}