diff --git a/cgi.go b/cgi.go index 7f31c36293..4863285078 100644 --- a/cgi.go +++ b/cgi.go @@ -18,6 +18,7 @@ import ( "crypto/tls" "net" "net/http" + "path" "path/filepath" "strings" "unicode/utf8" @@ -338,17 +339,25 @@ func sanitizedPathJoin(root, reqPath string) string { root = "." } - path := filepath.Join(root, filepath.Clean("/"+reqPath)) + // reqPath is an HTTP request path: always "/"-separated, regardless of + // host OS. It must be cleaned with the "path" package (POSIX-only), + // not "path/filepath": on Windows, filepath.Clean does not treat a + // driveless "/"-rooted path as absolute, so a leading ".." isn't + // collapsed at the root the way it is on POSIX - it survives into the + // joined path instead, escaping root. + cleanedReqPath := filepath.FromSlash(path.Clean("/" + reqPath)) + + joined := filepath.Join(root, cleanedReqPath) // filepath.Join also cleans the path, and cleaning strips // the trailing slash, so we need to re-add it afterward. // if the length is 1, then it's a path to the root, // and that should return ".", so we don't append the separator. if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 { - path += separator + joined += separator } - return path + return joined } // splitRemoteAddr splits "host:port" leniently: a missing port is accepted. diff --git a/cgi_test.go b/cgi_test.go index 3feffa3b54..c89a1f6662 100644 --- a/cgi_test.go +++ b/cgi_test.go @@ -1,6 +1,7 @@ package frankenphp import ( + "path/filepath" "strings" "testing" @@ -330,3 +331,65 @@ func TestSplitPosSecurityRegressionUnicodeBypass(t *testing.T) { assert.Equalf(t, -1, splitPos(p, split), "payload %q must not be detected as .php", p) } } + +// FuzzSplitPos guards the byte/rune-boundary arithmetic behind the Unicode +// case-folding bypasses above (GHSA-3g8v-8r37-cgjm, GHSA-v4h7-cj44-8fc8): +// splitPos must never return an out-of-bounds position, whatever bytes are +// thrown at it. +func FuzzSplitPos(f *testing.F) { + f.Add("/path/to/script.php", ".php") + f.Add("/path/to/script.php/some/path", ".php") + f.Add("/ȺȺȺȺshell.php.txt.php", ".php") + f.Add("/shell﹒php", ".php") + f.Add("", "") + + f.Fuzz(func(t *testing.T, path, splitMarker string) { + pos := splitPos(path, []string{splitMarker}) + if pos < -1 || pos > len(path) { + t.Fatalf("splitPos(%q, %q) returned out-of-bounds position %d for a %d-byte path", path, splitMarker, pos, len(path)) + } + }) +} + +// FuzzSanitizedPathJoin checks that the request path can never escape root, +// however it's mangled. +func FuzzSanitizedPathJoin(f *testing.F) { + f.Add("/var/www/html", "/index.php") + f.Add("/var/www/html", "../../etc/passwd") + f.Add("/var/www/html", "..\\..\\windows\\win.ini") + f.Add("", "/../../../etc/passwd") + f.Add("/var/www/html", "") + + f.Fuzz(func(t *testing.T, root, reqPath string) { + result := sanitizedPathJoin(root, reqPath) + + cleanRoot := root + if cleanRoot == "" { + cleanRoot = "." + } + rel, err := filepath.Rel(cleanRoot, result) + if err != nil { + // Different volumes on Windows and the like: not a traversal, just + // an unrelated path, but it must still not happen for a plain root. + return + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("sanitizedPathJoin(%q, %q) = %q escapes root", root, reqPath, result) + } + }) +} + +// FuzzSplitRemoteAddr guards against panics: it's called from a cgo +// callback, so a panic here would crash the process. +func FuzzSplitRemoteAddr(f *testing.F) { + f.Add("1.2.3.4:5") + f.Add("[::1]:443") + f.Add("[fe80::1%eth0]:443") + f.Add("[") + f.Add("[:9000") + f.Add("") + + f.Fuzz(func(t *testing.T, remoteAddr string) { + splitRemoteAddr(remoteAddr) + }) +} diff --git a/frankenphp.c b/frankenphp.c index b15507f69d..2378ac8ff6 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -1056,8 +1056,9 @@ PHP_FUNCTION(frankenphp_test_persist_roundtrip) { if (!persistent_zval_validate(input)) { zend_throw_exception(spl_ce_LogicException, - "persistent_zval: value type not supported " - "(only scalars, arrays, and enums are allowed)", + "persistent_zval: value not supported (only " + "scalars, arrays, and enums are allowed, nested " + "no deeper than PERSISTENT_ZVAL_MAX_DEPTH)", 0); RETURN_THROWS(); } diff --git a/frankenphp_test.go b/frankenphp_test.go index 6a9d71685a..20910cc94d 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -7,6 +7,8 @@ package frankenphp_test import ( "bytes" "context" + "encoding/base64" + "encoding/json" "errors" "flag" "fmt" @@ -100,8 +102,10 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), * wg.Add(opts.nbParallelRequests) for i := 0; i < opts.nbParallelRequests; i++ { go func(i int) { + // Deferred so a t.Skip/t.Fatalf from a non-main goroutine (which + // triggers runtime.Goexit) still decrements the WaitGroup. + defer wg.Done() test(handler, ts, i) - wg.Done() }(i) } @@ -1189,6 +1193,60 @@ func FuzzRequest(f *testing.F) { }) } +// FuzzResponseHeaders exercises add_response_header (frankenphp.c), FrankenPHP's +// own copy of the response header list into a PHP array. The header line is +// base64-encoded so arbitrary bytes reach it unmangled by HTTP transport. +func FuzzResponseHeaders(f *testing.F) { + f.Add("X-Foo: bar") + f.Add("X-Foo:bar") + f.Add("X-Foo : bar ") + f.Add(":no-name") + f.Add("no-colon-at-all") + f.Add("X-Foo: ") + f.Add("") + f.Add(strings.Repeat("X-Foo: bar", 1000)) + + f.Fuzz(func(t *testing.T, headerLine string) { + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) { + encoded := base64.StdEncoding.EncodeToString([]byte(headerLine)) + req := httptest.NewRequest("GET", "http://example.com/fuzz-response-header.php?h="+url.QueryEscape(encoded), nil) + body, resp := testRequest(req, handler, t) + + assert.Equal(t, 200, resp.StatusCode) + assert.True(t, json.Valid([]byte(body)), "frankenphp_response_headers() must always return valid JSON, got: %s", body) + }, nil) + }) +} + +// FuzzPersistZvalRoundtrip exercises zval.h's persistent_zval_persist/ +// _to_request/_free recursive tree walk: FrankenPHP's own mechanism for +// carrying values across the request/persistent memory boundary (used by +// worker state), not php-src itself. Nesting depth and width are +// fuzzer-controlled, since unbounded native recursion (no depth guard) is +// the interesting bug class here, not the value shapes themselves. +func FuzzPersistZvalRoundtrip(f *testing.F) { + f.Add(0, 1) + f.Add(1, 1) + f.Add(10, 2) + f.Add(100, 1) + f.Add(1000, 1) + f.Add(-1, -1) + + f.Fuzz(func(t *testing.T, depth, width int) { + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) { + req := httptest.NewRequest("GET", fmt.Sprintf("http://example.com/fuzz-persist-roundtrip.php?depth=%d&width=%d", depth, width), nil) + body, resp := testRequest(req, handler, t) + + if body == "SKIP" { + t.Skip("FRANKENPHP_TEST not set; skipping persistent_zval roundtrip fuzzing") + } + + assert.Equal(t, 200, resp.StatusCode) + assert.NotContains(t, body, "MISMATCH", "roundtrip changed the value for depth=%d width=%d", depth, width) + }, nil) + }) +} + func TestSessionHandlerReset_worker(t *testing.T) { runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { // Request 1: Set a custom session handler and start session diff --git a/testdata/fuzz-persist-roundtrip.php b/testdata/fuzz-persist-roundtrip.php new file mode 100644 index 0000000000..1b52b53dd6 --- /dev/null +++ b/testdata/fuzz-persist-roundtrip.php @@ -0,0 +1,43 @@ + +/* Conservative on purpose: comfortably below the depth that overflows the + * native stack even under sanitizer builds (larger per-frame redzones), far + * above any depth a legitimate config/state value would ever need. Matches + * the same order of magnitude as PHP's own default nesting caps (e.g. + * json_decode()'s default $depth of 512, Xdebug's max_nesting_level of + * 256). */ +#define PERSISTENT_ZVAL_MAX_DEPTH 256 + /* Enum payload stored in persistent memory: the class name + case name * are kept as persistent zend_strings and the case object is re-resolved * via zend_lookup_class + zend_enum_get_case_cstr on each read. */ @@ -26,8 +40,13 @@ typedef struct { /* Whitelist check: only scalars, arrays of allowed values, and enum * instances pass. Returns false for objects other than enums, resources, - * closures, references, etc. */ -static bool persistent_zval_validate(zval *z) { + * closures, references, etc. Also enforces PERSISTENT_ZVAL_MAX_DEPTH, + * bailing out before recursing further once hit - see the file header for + * why this is the only place that's safe to do so. */ +static bool persistent_zval_validate_depth(zval *z, int depth) { + if (depth > PERSISTENT_ZVAL_MAX_DEPTH) { + return false; + } switch (Z_TYPE_P(z)) { case IS_NULL: case IS_FALSE: @@ -47,7 +66,7 @@ static bool persistent_zval_validate(zval *z) { return true; zval *val; ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(z), val) { - if (!persistent_zval_validate(val)) + if (!persistent_zval_validate_depth(val, depth + 1)) return false; } ZEND_HASH_FOREACH_END(); @@ -58,6 +77,10 @@ static bool persistent_zval_validate(zval *z) { } } +static bool persistent_zval_validate(zval *z) { + return persistent_zval_validate_depth(z, 0); +} + /* Deep-copy a zval from request memory into persistent (pemalloc) memory. * Callers must have already passed persistent_zval_validate on src. *