From 04fdc0c1e8fde94e2c1ad86217e962c88d27c53e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 26 Jan 2026 17:21:34 +0100 Subject: [PATCH] fix: path confusion via unicode casing in CGI path splitting --- caddy/mercure.go | 3 +- caddy/module.go | 59 +++++--------- cgi.go | 59 ++++++++++++-- cgi_test.go | 177 ++++++++++++++++++++++++++++++++++++++++ request_options.go | 30 +++++++ request_options_test.go | 69 ++++++++++++++++ 6 files changed, 350 insertions(+), 47 deletions(-) create mode 100644 request_options_test.go diff --git a/caddy/mercure.go b/caddy/mercure.go index 081a6d34..9624f50e 100644 --- a/caddy/mercure.go +++ b/caddy/mercure.go @@ -22,8 +22,7 @@ func (f *FrankenPHPModule) assignMercureHub(ctx caddy.Context) { return } - opt := frankenphp.WithMercureHub(f.mercureHub) - f.mercureHubRequestOption = &opt + f.requestOptions = append(f.requestOptions, frankenphp.WithMercureHub(f.mercureHub)) for i, wc := range f.Workers { wc.mercureHub = f.mercureHub diff --git a/caddy/module.go b/caddy/module.go index 64163626..d1ab43a2 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -51,7 +51,7 @@ type FrankenPHPModule struct { preparedEnv frankenphp.PreparedEnv preparedEnvNeedsReplacement bool logger *slog.Logger - mercureHubRequestOption *frankenphp.RequestOption + requestOptions []frankenphp.RequestOption } // CaddyModule returns the Caddy module information. @@ -118,6 +118,7 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { if len(f.SplitPath) == 0 { f.SplitPath = []string{".php"} } + f.requestOptions = append(f.requestOptions, frankenphp.WithRequestSplitPath(f.SplitPath)) if f.ResolveRootSymlink == nil { rrs := true @@ -148,6 +149,8 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { f.Workers[i].FileName = resolvedPath } } + + f.requestOptions = append(f.requestOptions, frankenphp.WithRequestResolvedDocumentRoot(f.resolvedDocumentRoot)) } if f.preparedEnv == nil { @@ -162,6 +165,10 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { } } + if !f.preparedEnvNeedsReplacement { + f.requestOptions = append(f.requestOptions, frankenphp.WithRequestPreparedEnv(f.preparedEnv)) + } + if err := f.configureHotReload(fapp); err != nil { return err } @@ -180,31 +187,26 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c origReq := ctx.Value(caddyhttp.OriginalRequestCtxKey).(http.Request) repl := ctx.Value(caddy.ReplacerCtxKey).(*caddy.Replacer) - var ( - documentRootOption frankenphp.RequestOption - documentRoot string - ) - - if f.resolvedDocumentRoot == "" { + documentRoot := f.resolvedDocumentRoot + if documentRoot == "" { documentRoot = repl.ReplaceKnown(f.Root, "") if documentRoot == "" && frankenphp.EmbeddedAppPath != "" { documentRoot = frankenphp.EmbeddedAppPath } + // If we do not have a resolved document root, then we cannot resolve the symlink of our cwd because it may // resolve to a different directory than the one we are currently in. // This is especially important if there are workers running. - documentRootOption = frankenphp.WithRequestDocumentRoot(documentRoot, false) - } else { - documentRoot = f.resolvedDocumentRoot - documentRootOption = frankenphp.WithRequestResolvedDocumentRoot(documentRoot) + f.requestOptions = append(f.requestOptions, frankenphp.WithRequestDocumentRoot(documentRoot, false)) } - env := f.preparedEnv if f.preparedEnvNeedsReplacement { - env = make(frankenphp.PreparedEnv, len(f.Env)) + env := make(frankenphp.PreparedEnv, len(f.Env)) for k, v := range f.preparedEnv { env[k] = repl.ReplaceKnown(v, "") } + + f.requestOptions = append(f.requestOptions, frankenphp.WithRequestPreparedEnv(env)) } workerName := "" @@ -215,32 +217,15 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c } } - var ( - err error - fr *http.Request + fr, err := frankenphp.NewRequestWithContext( + r, + append( + f.requestOptions, + frankenphp.WithOriginalRequest(&origReq), + frankenphp.WithWorkerName(workerName), + )..., ) - if f.mercureHubRequestOption == nil { - fr, err = frankenphp.NewRequestWithContext( - r, - documentRootOption, - frankenphp.WithRequestSplitPath(f.SplitPath), - frankenphp.WithRequestPreparedEnv(env), - frankenphp.WithOriginalRequest(&origReq), - frankenphp.WithWorkerName(workerName), - ) - } else { - fr, err = frankenphp.NewRequestWithContext( - r, - documentRootOption, - frankenphp.WithRequestSplitPath(f.SplitPath), - frankenphp.WithRequestPreparedEnv(env), - frankenphp.WithOriginalRequest(&origReq), - frankenphp.WithWorkerName(workerName), - *f.mercureHubRequestOption, - ) - } - if err != nil { return caddyhttp.Error(http.StatusInternalServerError, err) } diff --git a/cgi.go b/cgi.go index c79a4160..2411d2e7 100644 --- a/cgi.go +++ b/cgi.go @@ -18,9 +18,12 @@ import ( "net/http" "path/filepath" "strings" + "unicode/utf8" "unsafe" "github.com/dunglas/frankenphp/internal/phpheaders" + "golang.org/x/text/language" + "golang.org/x/text/search" ) // Protocol versions, in Apache mod_ssl format: https://httpd.apache.org/docs/current/mod/mod_ssl.html @@ -252,24 +255,64 @@ func splitCgiPath(fc *frankenPHPContext) { fc.worker = getWorkerByPath(fc.scriptFilename) } -// splitPos returns the index where path should -// be split based on SplitPath. +var splitSearchNonASCII = search.New(language.Und, search.IgnoreCase) + +// splitPos returns the index where path should be split based on splitPath. // example: if splitPath is [".php"] // "/path/to/script.php/some/path": ("/path/to/script.php", "/some/path") -// -// Adapted from https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go -// Copyright 2015 Matthew Holt and The Caddy Authors func splitPos(path string, splitPath []string) int { if len(splitPath) == 0 { return 0 } - lowerPath := strings.ToLower(path) + pathLen := len(path) + + // We are sure that split strings are all ASCII-only and lower-case because of validation and normalization in WithRequestSplitPath for _, split := range splitPath { - if idx := strings.Index(lowerPath, strings.ToLower(split)); idx > -1 { - return idx + len(split) + splitLen := len(split) + + for i := 0; i < pathLen; i++ { + if path[i] >= utf8.RuneSelf { + if _, end := splitSearchNonASCII.IndexString(path, split); end > -1 { + return end + } + + break + } + + if i+splitLen > pathLen { + continue + } + + match := true + for j := 0; j < splitLen; j++ { + c := path[i+j] + + if c >= utf8.RuneSelf { + if _, end := splitSearchNonASCII.IndexString(path, split); end > -1 { + return end + } + + break + } + + if 'A' <= c && c <= 'Z' { + c += 'a' - 'A' + } + + if c != split[j] { + match = false + + break + } + } + + if match { + return i + splitLen + } } } + return -1 } diff --git a/cgi_test.go b/cgi_test.go index a3288633..d7c0e854 100644 --- a/cgi_test.go +++ b/cgi_test.go @@ -1,6 +1,7 @@ package frankenphp import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -31,3 +32,179 @@ func TestEnsureLeadingSlash(t *testing.T) { }) } } + +func TestSplitPos(t *testing.T) { + tests := []struct { + name string + path string + splitPath []string + wantPos int + }{ + { + name: "simple php extension", + path: "/path/to/script.php", + splitPath: []string{".php"}, + wantPos: 19, + }, + { + name: "php extension with path info", + path: "/path/to/script.php/some/path", + splitPath: []string{".php"}, + wantPos: 19, + }, + { + name: "case insensitive match", + path: "/path/to/script.PHP", + splitPath: []string{".php"}, + wantPos: 19, + }, + { + name: "mixed case match", + path: "/path/to/script.PhP/info", + splitPath: []string{".php"}, + wantPos: 19, + }, + { + name: "no match", + path: "/path/to/script.txt", + splitPath: []string{".php"}, + wantPos: -1, + }, + { + name: "empty split path", + path: "/path/to/script.php", + splitPath: []string{}, + wantPos: 0, + }, + { + name: "multiple split paths first match", + path: "/path/to/script.php", + splitPath: []string{".php", ".phtml"}, + wantPos: 19, + }, + { + name: "multiple split paths second match", + path: "/path/to/script.phtml", + splitPath: []string{".php", ".phtml"}, + wantPos: 21, + }, + // Unicode case-folding tests (security fix for GHSA-g966-83w7-6w38) + // U+023A (Ⱥ) lowercases to U+2C65 (ⱥ), which has different UTF-8 byte length + // Ⱥ: 2 bytes (C8 BA), ⱥ: 3 bytes (E2 B1 A5) + { + name: "unicode path with case-folding length expansion", + path: "/ȺȺȺȺshell.php", + splitPath: []string{".php"}, + wantPos: 18, // correct position in original string + }, + { + name: "unicode path with extension after expansion chars", + path: "/ȺȺȺȺshell.php/path/info", + splitPath: []string{".php"}, + wantPos: 18, + }, + { + name: "unicode in filename with multiple php occurrences", + path: "/ȺȺȺȺshell.php.txt.php", + splitPath: []string{".php"}, + wantPos: 18, // should match first .php, not be confused by byte offset shift + }, + { + name: "unicode case insensitive extension", + path: "/ȺȺȺȺshell.PHP", + splitPath: []string{".php"}, + wantPos: 18, + }, + { + name: "unicode in middle of path", + path: "/path/Ⱥtest/script.php", + splitPath: []string{".php"}, + wantPos: 23, // Ⱥ is 2 bytes, so path is 23 bytes total, .php ends at byte 23 + }, + { + name: "unicode only in directory not filename", + path: "/Ⱥ/script.php", + splitPath: []string{".php"}, + wantPos: 14, + }, + // Additional Unicode characters that expand when lowercased + // U+0130 (İ - Turkish capital I with dot) lowercases to U+0069 + U+0307 + { + name: "turkish capital I with dot", + path: "/İtest.php", + splitPath: []string{".php"}, + wantPos: 11, + }, + // Ensure standard ASCII still works correctly + { + name: "ascii only path with case variation", + path: "/PATH/TO/SCRIPT.PHP/INFO", + splitPath: []string{".php"}, + wantPos: 19, + }, + { + name: "path at root", + path: "/index.php", + splitPath: []string{".php"}, + wantPos: 10, + }, + { + name: "extension in middle of filename", + path: "/test.php.bak", + splitPath: []string{".php"}, + wantPos: 9, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotPos := splitPos(tt.path, tt.splitPath) + assert.Equal(t, tt.wantPos, gotPos, "splitPos(%q, %v)", tt.path, tt.splitPath) + + // Verify that the split produces valid substrings + if gotPos > 0 && gotPos <= len(tt.path) { + scriptName := tt.path[:gotPos] + pathInfo := tt.path[gotPos:] + + // The script name should end with one of the split extensions (case-insensitive) + hasValidEnding := false + for _, split := range tt.splitPath { + if strings.HasSuffix(strings.ToLower(scriptName), split) { + hasValidEnding = true + + break + } + } + assert.True(t, hasValidEnding, "script name %q should end with one of %v", scriptName, tt.splitPath) + + // Original path should be reconstructable + assert.Equal(t, tt.path, scriptName+pathInfo, "path should be reconstructable from split parts") + } + }) + } +} + +// TestSplitPosUnicodeSecurityRegression specifically tests the vulnerability +// described in GHSA-g966-83w7-6w38 where Unicode case-folding caused +// incorrect SCRIPT_NAME/PATH_INFO splitting +func TestSplitPosUnicodeSecurityRegression(t *testing.T) { + // U+023A: Ⱥ (UTF-8: C8 BA). Lowercase is ⱥ (UTF-8: E2 B1 A5), longer in bytes. + path := "/ȺȺȺȺshell.php.txt.php" + split := []string{".php"} + + pos := splitPos(path, split) + + // The vulnerable code would return 22 (computed on lowercased string) + // The correct code should return 18 (position in original string) + expectedPos := strings.Index(path, ".php") + len(".php") + assert.Equal(t, expectedPos, pos, "split position should match first .php in original string") + assert.Equal(t, 18, pos, "split position should be 18, not 22") + + if pos > 0 && pos <= len(path) { + scriptName := path[:pos] + pathInfo := path[pos:] + + assert.Equal(t, "/ȺȺȺȺshell.php", scriptName, "script name should be the path up to first .php") + assert.Equal(t, ".txt.php", pathInfo, "path info should be the remainder after first .php") + } +} diff --git a/request_options.go b/request_options.go index 2227b06e..7f1e6b4f 100644 --- a/request_options.go +++ b/request_options.go @@ -1,11 +1,14 @@ package frankenphp import ( + "errors" "log/slog" "net/http" "path/filepath" + "strings" "sync" "sync/atomic" + "unicode/utf8" "github.com/dunglas/frankenphp/internal/fastabs" ) @@ -14,6 +17,8 @@ import ( type RequestOption func(h *frankenPHPContext) error var ( + ErrInvalidSplitPath = errors.New("split path contains non-ASCII characters") + documentRootCache sync.Map documentRootCacheLen atomic.Uint32 ) @@ -71,11 +76,36 @@ func WithRequestResolvedDocumentRoot(documentRoot string) RequestOption { // actual resource (CGI script) name, and the second piece will be set to // PATH_INFO for the CGI script to use. // +// Split paths can only contain ASCII characters. +// Comparison is case-insensitive. +// // Future enhancements should be careful to avoid CVE-2019-11043, // which can be mitigated with use of a try_files-like behavior // that 404s if the FastCGI path info is not found. func WithRequestSplitPath(splitPath []string) RequestOption { return func(o *frankenPHPContext) error { + var b strings.Builder + + for i, split := range splitPath { + b.Grow(len(split)) + + for j := 0; j < len(split); j++ { + c := split[j] + if c >= utf8.RuneSelf { + return ErrInvalidSplitPath + } + + if 'A' <= c && c <= 'Z' { + b.WriteByte(c + 'a' - 'A') + } else { + b.WriteByte(c) + } + } + + splitPath[i] = b.String() + b.Reset() + } + o.splitPath = splitPath return nil diff --git a/request_options_test.go b/request_options_test.go new file mode 100644 index 00000000..9da421f7 --- /dev/null +++ b/request_options_test.go @@ -0,0 +1,69 @@ +package frankenphp + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWithRequestSplitPath(t *testing.T) { + tests := []struct { + name string + splitPath []string + wantErr error + wantSplitPath []string + }{ + { + name: "valid lowercase split path", + splitPath: []string{".php"}, + wantErr: nil, + wantSplitPath: []string{".php"}, + }, + { + name: "valid uppercase split path normalized", + splitPath: []string{".PHP"}, + wantErr: nil, + wantSplitPath: []string{".php"}, + }, + { + name: "valid mixed case split path normalized", + splitPath: []string{".PhP", ".PHTML"}, + wantErr: nil, + wantSplitPath: []string{".php", ".phtml"}, + }, + { + name: "empty split path", + splitPath: []string{}, + wantErr: nil, + wantSplitPath: []string{}, + }, + { + name: "non-ASCII character in split path rejected", + splitPath: []string{".php", ".Ⱥphp"}, + wantErr: ErrInvalidSplitPath, + }, + { + name: "unicode character in split path rejected", + splitPath: []string{".phpⱥ"}, + wantErr: ErrInvalidSplitPath, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := &frankenPHPContext{} + opt := WithRequestSplitPath(tt.splitPath) + err := opt(ctx) + + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantSplitPath, ctx.splitPath) + }) + } +}