Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121924
MIME-Version: 1.0
References: <CAMZNyFfiP5y8QAcNBCo235xG30Ra-PgCKcYfsZVuvgwy9Re-8w@mail.gmail.com>
 <CAFPFaMJeLD1WVzczjfXa4iKg0QtgKxhH0e3mzOMjsa9POf=C9w@mail.gmail.com>
 <CACsECNcbe+D=8WMa=LG-ts6_Rq1W91hneb43i07L58xC5WUt3w@mail.gmail.com>
 <CAMZNyFe0YywB5WGt3ga6s1SFOytNLFfF3=eEfMhnpWTDh24C9w@mail.gmail.com> <CACsECNf_+WHxFwR02n631pk5hR52eAjN1QZAw-7j+g8RQzoi3g@mail.gmail.com>
In-Reply-To: <CACsECNf_+WHxFwR02n631pk5hR52eAjN1QZAw-7j+g8RQzoi3g@mail.gmail.com>
Reply-To: Stefan Schiller <stefan.schiller@sonarsource.com>
Date: Tue, 5 Dec 2023 09:43:23 +0100
Message-ID: <CAMZNyFd79C1TqR=K9_2tnP8aakzx5vnsOy=DFAkqZ0wxc341tw@mail.gmail.com>
To: Alex <alexinbeijing@gmail.com>
Cc: "G. P. B." <george.banyard@gmail.com>, internals@lists.php.net, 
	youkidearitai@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Inconsistency mbstring functions
From: internals@lists.php.net ("Stefan Schiller via internals")

On Mon, Dec 4, 2023 at 8:45=E2=80=AFPM Alex <alexinbeijing@gmail.com> wrote=
:
>
> Stefan,
>
>>
>> My biggest concern is that this quirk can cause security issues in
>> user code. I came across this in the first place when discovering an
>> exploitable security vulnerability in an application. From my point of
>> view, this is not only about inconsistent behavior but also violates
>> the documentation for specific functions like mb_strstr. I agree that
>> a lot of mbstring operations should not be used on invalid strings,
>> and an exception seems to be an appropriate answer despite the huge BC
>> impact.
>
>
> 1) I'm sure the vulnerable application is proprietary, but can you at lea=
st tell us which mbstring functions were directly involved in this vulnerab=
ility, give an overview of the mechanism by which the exploit worked (scrub=
bed of any specific details about the application), and let us know the gen=
eral nature of the resulting compromise? (i.e. denial-of-service, informati=
on disclosure, allowing users to impersonate other users, etc)
>
> Real-life examples of actual security impact provide far more compelling =
grounds for a BC break than hypothetical security impact.

The case I am referring to involves mb_strpos, which is used to
determine the index of a specific character, and mb_substr, which is
used to extract the substring until the first occurrence of this
character. For this specific case, this results in a Cross-Site
Scripting (XSS) vulnerability, which would allow an attacker to
perform actions on behalf of an authenticated user. The actual code is
more complex, but the following lines should be a suitable
representation of the issue:

$data =3D $_GET['data'];
$pos =3D mb_strpos($data, "<");
$out =3D $pos ? mb_substr($data, 0, $pos) : $data;
echo $out;

The developer's assumption here is that $out should never contain an
opening HTML tag, and it is thus safe to echo it back in the response.
Despite the fact that e.g. htmlspecialchars should be used to encode
the output, this assumption seems reasonable to me. However, due to
the inconsistent behavior, an attacker can break this assumption. If
$data, e.g., contains the sequence "\xf0AAA<b>", str_pos will consider
the index of "<" to be 4. However, mb_substr assumes the full sequence
to have a length of 4 (1 x 4-byte Unicode character + 3 x 1-byte
Unicode characters). Accordingly, the full sequence, including the
"<b>" tag, is reflected in the response.

>
> 2) Your point about documentation is appreciated. However, you should con=
sider the context. We are dealing with a 20+-year old library, with a large=
 installed base, which has (historically) always had inconsistent, poorly d=
efined semantics in edge cases, and which (historically) has always had poo=
r documentation. Don't forget Hyrum's Law, Stefan. Never forget Hyrum's Law=
.
>
> In view of all this, we are usually much more likely to amend the mbstrin=
g documentation to match behavior than to modify behavior to match document=
ation. Now, that's not to say we can never modify mbstring behavior to matc=
h documentation (I have done it myself several times), but there should alw=
ays be good reasons to prefer the new behavior (not just "because that's wh=
at the documentation says").

I can fully understand your concerns, especially taking into account
the long-standing history of the library and Hyrum's Law. While the
above-mentioned example might even be arguable since two different
functions are involved, the behavior of, e.g., mb_strstr just feels
wrong to me:

$data =3D "\xf0start";
var_dump(mb_strstr($data, "start")); // string(2) "rt"

>
> 3) Modifying mbstring behavior to make the handling of invalid strings mo=
re consistent is a smaller BC break than raising an exception. However, com=
pletely refusing to operate on invalid strings (by raising an exception) wi=
ll be more effective in guarding against potential security vulnerabilities=
.

Considering BC, completely refusing to operate on invalid strings
seems like a huge break. From a
preventing-security-issues-in-user-code point of view, either of these
solutions seems appropriate.

>
> 4) Hamada-san is absolutely correct that all mbstring users should check =
strings derived from user input using mb_check_encoding() before operating =
on them. Unfortunately, "should" doesn't count for much. Most users will no=
t do what they "should".
>
> 5) Amending the documentation to call out these dangers more clearly woul=
d be a positive step, though probably not enough.
>

In general, I like the idea of warning against pitfalls like this in
the documentation, although I estimate the effect, especially on
existing applications to be low.

Best Regards,
Stefan Schiller