Skip to content

Fix GH-23106: mb_strpos() reads past the end of a haystack ending in a truncated UTF-8 sequence - #25121

Closed
VishnuSrivatsava wants to merge 1 commit into
php:PHP-8.3from
VishnuSrivatsava:fix/gh23106-mb-strpos-8.3
Closed

VishnuSrivatsava wants to merge 1 commit into
php:PHP-8.3from
VishnuSrivatsava:fix/gh23106-mb-strpos-8.3

Conversation

@VishnuSrivatsava

Copy link
Copy Markdown

Fixes #25025 and GH-23106 for the PHP-8.3 branch.

Backport of master commit cc22056 (PR #23107, credited to Lazizbek Ergashev), which shipped in 8.4.27 / 8.5.12 but never reached the stable branches. The forward offset walk in offset_to_pointer_utf8() can step past the end of the haystack when it lands on a truncated multi-byte lead byte, and the returned pointer then violates zend_memnstr()'s end >= p invariant (assertion failure in debug builds). #25025 additionally documents overshoots on the reverse path — the same clamp fixes those as well, since both paths share offset_to_pointer_utf8().

Changes:

Verification on this branch (macOS arm64, --enable-debug + AddressSanitizer):

  • Pre-fix: mb_strpos("AA\xf0\x90", "xyz", 3) aborts with Assertion failed: (end >= p), function zend_memnstr, file zend_operators.h, line 182.
  • Post-fix: assertion gone, regression test passes, full ext/mbstring suite passes (343 passed, 0 failed, 82 environment skips).

Not touching NEWS, following the branch convention where release managers add entries at packaging time.

For PHP-8.2: per the analysis in #25025, the fix there has to be rewritten against mbfl_find_offset_utf8() in libmbfl (different function and file, const unsigned char * pointers), so I have not attempted a straight backport; happy to prepare it if release managers want it while the branch is in security support.

…in a truncated UTF-8 sequence

Backport of master commit cc22056 (PR php#23107), requested in phpGH-25025.
offset_to_pointer_utf8() can step past the end of the haystack when the
forward offset walk lands on a truncated multi-byte sequence, and the
returned pointer is then used by zend_memnstr() despite violating its
end >= pointer invariant. Clamp the pointer to the end of the haystack.
@youkidearitai

Copy link
Copy Markdown
Contributor

Hmm, 8.3 and 8.2 is security fixes only. So I think we can't merge in 8.3 and 8.2

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants