mb_str{i,}pos does not match illegal byte sequences against occurrences of mb_substitute_char

In GitHub issue 9613, it was reported that mb_strpos wrongly matches the
character '?' again

mb_str{i,}pos does not match illegal byte sequences against occurrences of mb_substitute_char

In GitHub issue 9613, it was reported that mb_strpos wrongly matches the
character '?' against any invalid string, even when the character '?'
clearly does not appear in the invalid string. This behavior has existed
at least since PHP 5.2.

The reason for the behavior is that mb_strpos internally converts the
haystack and needle to UTF-8 before performing a search. When converting
to UTF-8, regardless of the setting of mb_substitute_character, libmbfl
would use '?' as an error marker for invalid byte sequences. Once those
invalid input sequences were replaced with '?', then naturally, they
would match against occurrences of the actual character '?' (when it
appeared as a 'normal' character, not as an error marker). This would
happen regardless of whether the error was in the haystack and '?' was
used in the needle, or whether the error was in the needle and '?' was
used in the haystack.

Why would libmbfl use '?' rather than the mb_substitute_character set
by the user? Remember that libmbfl was originally a separate library
which was imported into the PHP codebase. mb_substitute_character is an
mbstring API function, not something built into libmbfl. When mbstring
would call into libmbfl, it would provide the error replacement
character to libmbfl as a parameter. However, when libmbfl would perform
conversion operations internally, and not because of a direct call from
mbstring, it would use its own error replacement character.

Example:

<?php
$questionMark = "\x00?";
$badUTF16 = "\xDB\x00"; // half of a surrogate pair
echo mb_strpos($questionMark, $badUTF16, 0, 'UTF-16BE'), "\n";
echo mb_strpos($badUTF16, $questionMark, 0, 'UTF-16BE'), "\n";

Incidentally, this behavior does not occur if the text encoding is
UTF-8, because no conversion is needed in that case.

mb_stripos had a similar issue, but instead of always using '?' as an
error marker internally, it would use the selected
mb_substitute_character. So, for example, if the mb_substitute_character
was '%', then occurrences of '%' in the haystack would match invalid
bytes in the needle, and vice versa.

Example:

<?php
mb_substitute_character(0x25); // '%'
$percent = "\x00%";
$badUTF16 = "\xDB\x00"; // half of a surrogate pair
echo mb_stripos($percent, $badUTF16, 0, 'UTF-16BE'), "\n";
echo mb_stripos($badUTF16, $percent, 0, 'UTF-16BE'), "\n";

This behavior (of mb_stripos) still occurs even if the text encoding is
UTF-8, because case folding is still needed to make the search
case-insensitive.

It is not hard to think of scenarios where these strange and unintuitive
behaviors could cause security vulnerabilities. In the discussion on
GH issue 9613, Christoph Becker suggested that mb_str{i,}pos should
simply refuse to operate on invalid strings. However, this would almost
certainly break existing production code.

This commit mitigates the problem in a less intrusive way: it ensures
that while invalid haystacks can match invalid needles (even if the
specific invalid bytes are different), invalid bytes in the haystack
will never match '?' OR occurrences of the mb_substitute_character in
the needle, and vice versa.

This does represent a backwards compatibility break, but a small one.
Since it mitigates a potential security problem, I believe this is
appropriate.

Closes GH-9613.

show more ...

Speed boost for mb_stripos (when not using UTF-8)

Instead of case-folding a string and then converting it to UTF-8 as a
separate operation, why not convert it to UTF-8 at the same time a

Speed boost for mb_stripos (when not using UTF-8)

Instead of case-folding a string and then converting it to UTF-8 as a
separate operation, why not convert it to UTF-8 at the same time as
we fold case?

For non-UTF-8 encodings, this typically makes mb_stripos about 2x
faster.

show more ...

Migrate more SKIPIF -> EXTENSIONS (#7139)

This is a mix of more automated and manual migration. It should remove all applicable extension_loaded() checks outside of skipif.inc files.

Replace EXPECTF when possible

Closes GH-5779

Convert warnings to ValueError in mb_strpos function family.

Closes GH-5109

Clean DONE tags from tests

Remove most of the `===DONE===` tags and its variations.
Keep `===DONE===` if the test output otherwise becomes empty.

Closes GH-4872.

Adjust tests for zpp TypeError change

Remove custom error handler in mbstring tests

To make it more obvious what is tested and what the error messages
are.

Trim trailing whitespace in tests

Remove the b prefix from literals on unrelated tests

mb_strpos()/mb_stripos(): Add support for negative offset

It's a tradition return NULL on error of args parsing

MBString tests: checked on PHP 5.2.6, 5.3 and 6.0 (Windows, Linux and Linux 64 bit).

- MFH: Fix a typo in mb_stripos() that caused segfaults (noticed by bs@php.net, thanks).