Skip to content

test: extra Unicode regression vectors for splitPos (GHSA-g966-83w7-6w38 follow-up) - #2824

Open
sucloudflare wants to merge 3 commits into
php:mainfrom
sucloudflare:regression/unicode-splitpos-vectors
Open

sucloudflare wants to merge 3 commits into
php:mainfrom
sucloudflare:regression/unicode-splitpos-vectors

Conversation

@sucloudflare

Copy link
Copy Markdown
Contributor

Summary

While studying the fix for GHSA-g966-83w7-6w38 (CVE-2026-24895), I identified a residual edge case in the implementation released in v1.11.2 (commit 04fdc0c), and put together a set of regression vectors that pin the entire bug class against the current implementation. All tests in this PR pass on current main (v1.13.x) — this is not a bug report against the supported version; it's permanent coverage so the class cannot regress.

Background

Versions splitPos implementation Notes
< 1.11.2 strings.ToLower + strings.Index Byte-index desync — CVE-2026-24895 / GHSA-g966-83w7-6w38
1.11.2 – 1.12.3 ASCII fast path + x/text search.IgnoreCase (04fdc0c) Residual "bogus split" case (below); search.IgnoreCase itself was unsafe — CVE-2026-45062 / GHSA-3g8v-8r37-cgjm
1.13.0+ Strict byte-wise ASCII matching, match = false on >= utf8.RuneSelf, segment-boundary rule Resolves the full class

Related project: the pre-fix implementation was originally adapted from Caddy's reverseproxy/fastcgi module. Caddy's current master already carries the same hardened implementation ("Adapted from FrankenPHP's code", referencing GHSA-3g8v-8r37-cgjm / GHSA-v4h7-cj44-8fc8), so this PR only concerns FrankenPHP's regression suite.

The residual case in v1.11.2–v1.12.3 (fixed by the current rewrite)

In the 04fdc0c implementation, the inner matching loop treated a byte >= utf8.RuneSelf as a possible start of a case-folded match and fell back to splitSearchNonASCII.IndexString(path, split). When that fallback found nothing, the inner loop break — but match was still true from the ASCII prefix bytes already compared. The outer if match { return i + splitLen } then returned a bogus split index that sliced mid-UTF-8-rune, even though the split marker never occurred in the path:

```go
// path = "/x.phȺy" (bytes: 2f 78 2e 70 68 c8 ba 79 — no ".php" anywhere)
splitPos("/x.phȺy", []string{".php"})
// v1.11.2–v1.12.3: 6 -> scriptName "/x.ph\xC8" (invalid UTF-8), pathInfo "\xBAy"
// current main: -1 (correct)
```

The same happens with /upload.phȾnote (returns 11) and with raw lone bytes such as /x.ph%C8y. Practical impact was low (nonexistent script names, corrupted CGI variables), and it was incidentally resolved by the v1.13.0 rewrite that accompanied GHSA-3g8v-8r37-cgjm — hence a regression test rather than a security report.

What this PR adds

  • TestSplitPosMultiByteNeverMatches — bytes >= utf8.RuneSelf can never satisfy an ASCII-only split marker; bogus-prefix windows must yield -1 (and never produce invalid-UTF-8 SCRIPT_NAME).
  • TestSplitPosOtherExpansionChar — U+023E (Ⱦ → ⱦ) is the only codepoint besides U+023A (Ⱥ → ⱥ) whose simple lowercase mapping grows UTF-8 length (2B → 3B) in Go. The original fix's tests covered Ⱥ and U+0130 (İ); Ⱦ completes the expansion set.
  • TestSplitPosShrinkingChars — the shrink direction: a full scan of U+0080..U+10FFFF (Go simple-lowercase semantics) finds 23 codepoints whose lowercase is shorter in UTF-8, including U+1E9E (ẞ, 3B → 2B), U+212A (K, 3B → 1B k), U+2126 (Ω), U+212B (Å), U+0130 (İ) and the Latin Extended-C/D compatibility block.
  • TestSplitPosUnicodeSegmentBoundary — interaction between Unicode paths and the segment-boundary rule, including the exact advisory PoC path.

Validation

  • Vectors verified against a byte-exact standalone port of both the 04fdc0c and the current implementation.
  • go test -run 'TestSplitPos' -v ./... passes on current main — validated against the corresponding implementation during research; final CI run will confirm on this exact tree.

Credits & references

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants