Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121921
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>
In-Reply-To: <CAMZNyFe0YywB5WGt3ga6s1SFOytNLFfF3=eEfMhnpWTDh24C9w@mail.gmail.com>
Date: Mon, 4 Dec 2023 14:25:36 +0100
Message-ID: <CAPzBOBNDhMkF1_qMHpSeZF=1+Gb8ZoobKnU7rLRuHN-9t3BGkQ@mail.gmail.com>
To: Stefan Schiller <stefan.schiller@sonarsource.com>
Cc: Alex <alexinbeijing@gmail.com>, "G. P. B." <george.banyard@gmail.com>, 
	internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Inconsistency mbstring functions
From: landers.robert@gmail.com (Robert Landers)

On Mon, Dec 4, 2023 at 1:51=E2=80=AFPM Stefan Schiller via internals
<internals@lists.php.net> wrote:
>
> On Sat, Dec 2, 2023 at 6:13=E2=80=AFAM Alex <alexinbeijing@gmail.com> wro=
te:
> >
> > Dear Stefan, and Dear Gina,
> >
> > Thanks for the message. Yes, Stefan has rediscovered an interesting qui=
rk of the mbstring library. I have been aware of this for a long time, and =
other mbstring developers have too. It dates back to the origin of the libr=
ary; actually, even before the origin of mbstring, since mbstring was based=
 on another library called libmbfl, and this behavior originates from libmb=
fl.
> >
> > Pull your chair up around the fire and let me tell you the tale of libm=
bfl. Once upon a time, there was a text-processing library called libmbfl. =
libmbfl was based on a collection of text-decoding routines (which converte=
d bytes to codepoints) and text-encoding routines (which converted codepoin=
ts to bytes). Each such routine was structured as a stateful "filter". Thes=
e filters could be assembled into "chains", whereby the output values gener=
ated by one routine would automatically be passed to the next. libmbfl coul=
d perform many wonderful text-processing tasks by substituting a different =
final filter at the end of the chain.
> >
> > But all was not well. Since libmbfl's filters processed text only one b=
yte or codepoint at a time, and each routine had to save its state before r=
eturning, and restore its state upon entry, libmbfl was slow. Slow as a tur=
tle, slow as a snail, slow as whatever-slowly-moving-thing-you-can-think-of=
. Oh, what was libmbfl to do? A clever plan was hatched: give libmbfl a 256=
-entry table called a "mblen_table" for each supported text encoding with t=
he property that the byte length of a character can be determined from its =
first byte. Then, text-processing tasks which were not dependent on the act=
ual content of a string, but only on the number of codepoints, could be per=
formed without ever invoking those wonderful, but painfully slow filters! l=
ibmbfl could skip through a string while just examining the first byte of e=
ach character. (Of course, this only worked for text encodings with an mble=
n_table.) For valid strings, the new method worked identically to the previ=
ous one. For invalid strings, there were significant differences in behavio=
r, but libmbfl tried to ignore these and bravely pressed on.
> >
> > The story ends with an ironic twist. Many years later, I became interes=
ted in mbstring and reimplemented its internals, replacing the libmbfl code=
 with fresh new code which ran many times faster. The new code was so much =
faster that in some cases, the mblen_table optimization actually became a p=
essimization! In other cases, the mblen_table-based code is still faster, b=
ut not by a large amount. But now mbstring was haunted by the spectre of Hy=
rum's Law (https://www.hyrumslaw.com/). With a huge body of legacy code rel=
ying on mbstring, almost any observable behavior change runs a significant =
risk of breaking someone's code. And when this happens, they will not hesit=
ate to vent their rage on the hapless maintainers.
> >
> > Notwithstanding the rage of the users, about a year ago, I did remove t=
he mblen_table-based code in one place where benchmarks clearly showed it w=
as acting as a pessimization. I don't remember which mbstring function was =
affected and would need to check the commit log to confirm.
>
> Hi Alex,
>
> Thank you very much for sharing this background context.
>
> >
> > Personally, I think the real issue here is not the inconsistency betwee=
n mbstring functions which are based on the mblen_tables and those which ar=
e not. I think a lot of mbstring operations should not be used on invalid s=
trings at all, and that for such operations, mbstring would do well to thro=
w an exception if it receives invalid input. (Like mb_strpos; how do you de=
fine the "position of a UTF-8 substring" when the parent string is not UTF-=
8 at all?) But that would be a huge BC break.
> >
>
> 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.

I think it is only a security issue when people accidentally think
mb_* functions should be used if it is available. I've seen people do
mb_strlen() on binary data, for example, not realizing the differences
between mb_strlen and strlen. Or using mb_* functions and then passing
them off to cryptographic functions.

Robert Landers
Software Engineer
Utrecht NL