From 3d005442e4ee1b1fd8c129ff47c74ae636438fdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Thu, 5 Nov 2026 00:02:42 +0100 Subject: [PATCH] fastcgi: reject paths whose escaped form selects another script (#8357) --- .../caddyhttp/reverseproxy/fastcgi/fastcgi.go | 24 +++++++++ .../reverseproxy/fastcgi/fastcgi_test.go | 51 ++++++++++++++----- .../fastcgi/fastcgi_windows_test.go | 23 ++++++--- 3 files changed, 80 insertions(+), 18 deletions(-) diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go index 63f45ca3e..3a4dd82ad 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go @@ -20,6 +20,7 @@ import ( "fmt" "net" "net/http" + "net/url" "path/filepath" "runtime" "strconv" @@ -322,6 +323,21 @@ func (t Transport) buildEnv(r *http.Request) (envVars, error) { return nil, caddyhttp.Error(http.StatusBadRequest, errors.New("illegal script path")) } + // Path matchers whose pattern contains "%" clean the escaped path, where "%2f" + // and "%2e%2e" are neither separators nor dot segments, so both forms must + // select the same script. On Windows, they read a backslash, escaped even + // without RawPath, as "%2f". + backslash := runtime.GOOS == "windows" && strings.Contains(r.URL.Path, `\`) + if r.URL.RawPath != "" || backslash { + escaped, err := url.PathUnescape(caddyhttp.NormalizedPath(r.URL.EscapedPath(), true)) + if backslash { + escaped = strings.ReplaceAll(escaped, `\`, "/") + } + if err != nil || t.scriptPart(escaped) != docURI { + return nil, caddyhttp.Error(http.StatusBadRequest, errors.New("escaped and decoded paths select different scripts")) + } + } + // Try to grab the path remainder from a file matcher // if we didn't get a split result here. // See https://github.com/caddyserver/caddy/issues/3718 @@ -526,6 +542,14 @@ func (t Transport) splitPos(path string) int { return -1 } +// scriptPart returns the part of path naming the script. +func (t Transport) scriptPart(path string) string { + if pos := t.splitPos(path); pos > -1 { + return path[:pos] + } + return path +} + type envVars map[string]string // loggableEnv is a simple type to allow for speeding up zap log encoding. diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go index 8938571c0..a65d94f6f 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go @@ -568,18 +568,6 @@ func TestBuildEnvDotSegmentsCannotSelectAnotherScript(t *testing.T) { expectedScriptName: "/other.php", expectedPathInfo: "", }, - { - name: "percent-encoded separators", - target: "/admin.php/..%2f..%2fother.php", - expectedScriptName: "/other.php", - expectedPathInfo: "", - }, - { - name: "percent-encoded dots", - target: "/admin.php/%2e%2e/other.php", - expectedScriptName: "/other.php", - expectedPathInfo: "", - }, { name: "dot segments before legitimate path info", target: "/admin.php/../other.php/extra/path", @@ -620,3 +608,42 @@ func TestBuildEnvDotSegmentsCannotSelectAnotherScript(t *testing.T) { }) } } + +// Path matchers whose pattern contains "%" clean the escaped path, so +// "/public/tool.php%2f..%2f..%2fadmin%2ftool.php" matches "path /public/%*" +// while the decoded path selects admin/tool.php. +func TestBuildEnvEscapedPathCannotSelectAnotherScript(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + target string + wantErr bool + expectedScriptName string + }{ + {target: "/public/tool.php%2f..%2f..%2fadmin%2ftool.php", wantErr: true}, + {target: "/admin.php/..%2f..%2fother.php", wantErr: true}, + {target: "/admin.php/%2e%2e/other.php", wantErr: true}, + {target: "/dir%2fscript.php/a", expectedScriptName: "/dir/script.php"}, + {target: "/index.php/a%2f..%2fb", expectedScriptName: "/index.php"}, + {target: "/index.php/a%2Fb", expectedScriptName: "/index.php"}, + } { + t.Run(tc.target, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, tc.target, nil) + ctx := context.WithValue(r.Context(), caddy.ReplacerCtxKey, caddy.NewReplacer()) + ctx = context.WithValue(ctx, caddyhttp.VarsCtxKey, map[string]any{}) + ctx = context.WithValue(ctx, caddyhttp.OriginalRequestCtxKey, *r) + + tr := Transport{Root: t.TempDir(), SplitPath: []string{".php"}} + env, err := tr.buildEnv(r.WithContext(ctx)) + if !tc.wantErr { + require.NoError(t, err) + assert.Equal(t, tc.expectedScriptName, env["SCRIPT_NAME"]) + return + } + + var herr caddyhttp.HandlerError + require.ErrorAs(t, err, &herr) + assert.Equal(t, http.StatusBadRequest, herr.StatusCode) + }) + } +} diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_windows_test.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_windows_test.go index 7566b6f53..130f8e244 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_windows_test.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_windows_test.go @@ -31,7 +31,8 @@ import ( // On Windows the path matchers additionally fold backslashes into separators and // trim per-component trailing dots and spaces (see caddyhttp.NormalizedPath), // so the transport must do the same or the dot-segment bypass simply reappears -// spelled with backslashes. +// spelled with backslashes. Backslash dot segments are rejected outright because +// the "%" path matchers read them as "%2f..%2f", which they do not resolve. func TestBuildEnvWindowsSeparatorsCannotSelectAnotherScript(t *testing.T) { t.Parallel() @@ -46,21 +47,31 @@ func TestBuildEnvWindowsSeparatorsCannotSelectAnotherScript(t *testing.T) { for _, tc := range []struct { name string target string + wantErr bool expectedScriptName string expectedPathInfo string }{ - {"backslash dot segments", `/admin.php\..\other.php`, "/other.php", ""}, - {"backslash, two levels", `/admin.php\..\..\other.php`, "/other.php", ""}, - {"mixed separators", `/admin.php\../other.php`, "/other.php", ""}, + {"backslash dot segments", `/admin.php\..\other.php`, true, "", ""}, + {"backslash, two levels", `/admin.php\..\..\other.php`, true, "", ""}, + {"mixed separators", `/admin.php\../other.php`, true, "", ""}, + {"escaped backslash dot segments", "/admin.php%5C..%5Cother.php", true, "", ""}, + {"backslash separator", `/dir\other.php\a`, false, "/dir/other.php", "/a"}, + {"escaped backslash separator", "/dir%5Cother.php%5Ca", false, "/dir/other.php", "/a"}, // Per-component trailing dot/space folding is Windows-only and is not // something CleanPath does, so this case fails unless the transport // applies the same normalization the matchers do. - {"trailing dot in a path-info component", "/other.php/a./b", "/other.php", "/a/b"}, - {"trailing space in a path-info component", "/other.php/a%20/b", "/other.php", "/a/b"}, + {"trailing dot in a path-info component", "/other.php/a./b", false, "/other.php", "/a/b"}, + {"trailing space in a path-info component", "/other.php/a%20/b", false, "/other.php", "/a/b"}, } { t.Run(tc.name, func(t *testing.T) { tr := Transport{Root: `C:\srv`, SplitPath: []string{".php"}} env, err := tr.buildEnv(newRequest(tc.target)) + if tc.wantErr { + var herr caddyhttp.HandlerError + require.ErrorAs(t, err, &herr) + assert.Equal(t, http.StatusBadRequest, herr.StatusCode) + return + } require.NoError(t, err) assert.Equal(t, tc.expectedScriptName, env["SCRIPT_NAME"],