fuzz: expand coverage to cgi.go parsing, response headers, and zval persistence - #2606
Open
dunglas wants to merge 5 commits into
Open
fuzz: expand coverage to cgi.go parsing, response headers, and zval persistence#2606dunglas wants to merge 5 commits into
dunglas wants to merge 5 commits into
Conversation
t.Skip/t.Fatalf trigger runtime.Goexit on the calling goroutine, which skips everything after it, including a non-deferred wg.Done() below the test() call. Any runTest caller whose callback skips or fails deadlocks the whole WaitGroup instead of failing cleanly. Same fix already applied to the autoscale tests for the same reason (#2413); this is the one shared helper both call into.
splitPos and sanitizedPathJoin sit behind past CVEs (Unicode case-folding bypasses in the .php split point, path traversal in PATH_TRANSLATED); splitRemoteAddr is called from a cgo callback where a panic would crash the process rather than fail one request. All three are plain Go with no libphp involved, so they run at native fuzzing speed.
Exercises FrankenPHP's own copy of the response header list into a PHP array (frankenphp.c), reached only through frankenphp_response_headers(), not php-src's own header() validation. The header line is base64-encoded in the query string so arbitrary bytes reach it unmangled by HTTP transport; json_encode() needs JSON_INVALID_UTF8_SUBSTITUTE since header values may legitimately contain non-UTF-8 bytes and would otherwise return false (and echo nothing) on those, which isn't a bug.
persistent_zval_persist/_to_request/_free (zval.h) recurse once per nesting level with no depth guard. A plain linear chain of nested single-element arrays crashes the process (SIGBUS, native stack overflow) around depth ~700 on a local debug build; sanitizer builds, with much larger per-frame redzones, would hit it shallower still. Not reachable today - the only caller is the FRANKENPHP_TEST-only roundtrip hook, and zval.h itself is only compiled in under that guard, pending the first real caller (background workers, per the comment at its include site) - but it's a live landmine for whenever that lands: a native stack overflow there kills the whole process, not just one request. persistent_zval_validate is the one gate every caller already runs before persist/free/to_request, so it's the only safe place to reject excess depth: rejecting there means persist never starts, so there's no partially-persisted tree to unwind on the error path. Picked 256 as the cap, the same order of magnitude as PHP's own defaults (json_decode()'s $depth, Xdebug's max_nesting_level). The new fuzz target's own builder script needed a fix too: growing every slot at every level makes the tree size width**depth, which blows past available memory (and hangs the fuzzer) well under the depth needed to threaten the stack; only the first slot per level now recurses, so total size is depth*width instead.
Caught by FuzzSanitizedPathJoin on Windows CI: sanitizedPathJoin("",
"/../../../etc/passwd") returned "..\..\etc\passwd" - the traversal
escaped root instead of being neutralized.
reqPath is an HTTP request path (always "/"-separated, regardless of
host OS), but the code cleaned it with filepath.Clean, which uses
native-separator, native-OS rules. 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 of being dropped.
Fixed by cleaning reqPath with the "path" package (POSIX-only, no
OS-dependent branching) before handing it to filepath.Join for the
native-separator join onto root. path.Clean deterministically produces
the same traversal-free result on every platform, so there's no
leftover ".." left for filepath's OS-specific rules to mishandle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds fuzz targets covering FrankenPHP's own code (not php-src/libphp, which has its own upstream fuzzing infrastructure already), found via a codebase survey for untrusted-input parsing that only one existing target (
FuzzRequest) currently covers:FuzzSplitPos/FuzzSanitizedPathJoin/FuzzSplitRemoteAddr(cgi.go):splitPossits behind two past Unicode-folding CVEs (GHSA-3g8v-8r37-cgjm, GHSA-v4h7-cj44-8fc8);sanitizedPathJoinbuildsPATH_TRANSLATED(path-traversal surface);splitRemoteAddris called from a cgo callback where a panic crashes the process. All three are plain Go, no libphp needed, so they run at native fuzzing speed.FuzzResponseHeaders: exercisesadd_response_header(frankenphp.c) throughfrankenphp_response_headers()— FrankenPHP's own copy of the response header list into a PHP array, not php-src'sheader()validation itself.FuzzPersistZvalRoundtrip: exerciseszval.h's persist/free/to_request recursive tree walk, FrankenPHP's own mechanism for carrying values across the request/persistent-memory boundary (used by worker state).A bug this surfaced, fixed here
persistent_zval_persist/_to_request/_free(zval.h) recurse once per nesting level with no depth guard. A plain linear chain of nested single-element arrays crashed the process (SIGBUS, native stack overflow) around depth ~700 on a local debug build; sanitizer builds, with much larger per-frame redzones, would hit it shallower still.Not reachable today: the only caller is the
FRANKENPHP_TEST-only roundtrip hook added a few PRs back, andzval.hitself is only compiled in under that same guard, pending the first real caller (background workers, per the comment at its include site). But it's a live landmine for whenever that lands — a native stack overflow there kills the whole process, not just one request.Fixed by capping depth in
persistent_zval_validate, the one gate every caller already runs before persist/free/to_request: rejecting there means persist never starts, so there's no partially-persisted tree to unwind on the error path. Picked 256 as the cap, the same order of magnitude as PHP's own defaults (json_decode()'s$depth, Xdebug'smax_nesting_level).Also fixed: a pre-existing
runTestbug this exposedt.Skip/t.Fatalftriggerruntime.Goexiton the calling goroutine, which skips everything after it — including the non-deferredwg.Done()below thetest()call inrunTest's parallel-goroutine loop. Any caller whose callback skips or fails deadlocks the wholeWaitGroupinstead of failing cleanly. The autoscale tests hit this same class of bug before (#2413);runTestitself just hadn't needed a skip/fail path yet untilFuzzPersistZvalRoundtrip's "skip whenFRANKENPHP_TESTisn't set" branch did.Test plan
./go.sh test ./...passes, both with and without-DFRANKENPHP_TESTgo test -fuzz, no crashes after the depth-guard fixLogicExceptioninstead