Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:89481
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Thomas Hruska'" <thruska@cubiclesoft.com>,
	"'Yasuo Ohgaki'" <yohgaki@ohgaki.net>
Cc: "'PHP internals'" <internals@lists.php.net>
References: <CAF+90c_pVFVbzqThuPg1V3VDTV-7Ci4O6T6hNtY5XPgVb2Mt-Q@mail.gmail.com> <CANUQDChV=WRM6WQiNBq4-9JrvwxF_NY5YbGj3-tvgsPgQbc_pg@mail.gmail.com> <CAF+90c9GNzszV-nKfzD=1SGLpJoAfNtok45p7sSQz9A5TqdAdg@mail.gmail.com> <56588DBA.9070209@cubiclesoft.com> <CAGa2bXY1hR6rwi+hX3ubSpM3-H7ruK5FAQ=KZemKZPHYfw6FdA@mail.gmail.com> <5658E5C6.8030206@cubiclesoft.com>
In-Reply-To: <5658E5C6.8030206@cubiclesoft.com>
Date: Sat, 28 Nov 2015 15:54:38 +0100
Message-ID: <00cc01d129ec$b8177220$28465660$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQEkNhkZaWBBMPbsdhih7rI8XPIJCAJQZRZlAg4IbbMBdfRS9QJKgSvGAcbD1TSfvBQCIA==
Content-Language: en-us
Subject: RE: [PHP-DEV] HashDos protection
From: anatol.php@belski.net ("Anatol Belski")

Hi Thomas,

> -----Original Message-----
> From: Thomas Hruska [mailto:thruska@cubiclesoft.com]
> Sent: Saturday, November 28, 2015 12:23 AM
> To: Yasuo Ohgaki <yohgaki@ohgaki.net>
> Cc: PHP internals <internals@lists.php.net>
> Subject: Re: [PHP-DEV] HashDos protection
>=20
> On 11/27/2015 2:21 PM, Yasuo Ohgaki wrote:
> > Hi Thomas,
> >
> > In practice, we wouldn't have problems with max number of =
collisions.
>=20
> Is CLI going to be or can CLI be excluded from max collisions?  After =
thinking
> about it for a long while, that's my only area of concern here.
>   SAPI can (fatal) error to its heart's content and I'll find ways to =
live with it.  But
> CLI needs stability guarantees.  'max_input_vars'
> didn't affect CLI.  However, max collisions could negatively affect =
CLI because
> the change affects all arrays instead of just the superglobals.
>=20
> Real-world scenario:  About once every 4 to 6 months I will write a =
script to load
> up a single PHP array with a few million records.  Partly because I =
can and partly
> because I need to.  On CLI, especially for my one-off quick-n-dirty =
scripts, I don't
> care how much CPU, RAM, and other resources are used nor how much time =
it
> takes to complete.  I just care that the script finishes running =
successfully.  If the
> script reads in a record and attempts to add it to the array, the =
proposed max
> collisions might trigger a fatal error if it hits the max collision =
limit for arrays.
> My experience is that CLI is silent about 50% of the time when it =
encounters a
> fatal error.  So my script would drop back to the prompt after =
spending many
> minutes to hours loading the data, not having done any work, and not =
emit any
> error(s).  I would think that it had completed successfully until I =
went to look at
> the results and the results I would be expecting to see wouldn't be =
there.
>=20
> I abuse PHP arrays.  Especially on CLI.  Sorry.
>=20
The particular case being fixes is a vulnerability which can be =
specifically triggered. If you already used to write scripts creating =
arrays with millions of records, and those went through, means there was =
never enough collisions produced. If you have time, I'd ask you to =
please test this patch with one of your previous scripts of that kind.

The reproduce case from https://github.com/bk2204/php-hash-dos contains =
10^6  crafted keys, that renders the application to just hang as hash =
table will be busy resolving collisions and looking for free buckets. I =
was running that reproduce case and broke the CLI script after a hour as =
it doesn't make sense. Ofc, a cron running for hours or even days is ok, =
but it is clear that with invalid keys that cron will spend those days =
not in the actual job but resolving collisions. So indeed, if you would =
meet the malicious situation, your script wouldn't work. Using same =
approach for my test, I'm already able to produce an array with around =
2*10^6 11 byte long keys from 62 chars set (see the script linked here =
https://github.com/php/php-src/pull/1565#issuecomment-160231878 ). It is =
supposed to show, that the patch doesn't produce an issue with creating =
huge arrays.

I'd really plea for a bit less theoretical approach at this point - lets =
test it with any possible concerning cases and see. In the real life, =
the key length can vary, the quality of keys can vary, the keys can be =
explicitly made suitable for a huge data set actually just same way they =
can be crafted for a malicious attack. Say, use bigger set for keys and =
use longer keys, use keys generated by random_bytes(),  etc. As such =
scripts processing big datasets are usually only meant for cronjobs, =
taking special measures could affect their runtime, however - if a =
scripts runs for days, + or - a couple of hours won't be significant.

So after all, fixing the vulnerability could theoretically cost =
something for the justified  but unusual use cases like big datasets. =
However it won't make those unusual cases impossible, furthermore it =
doesn't really seem to have an impact in practice, and it won't affect =
common cases at all. At least that is what my current tests show. =
Please, let's fetch the patch and test it.

Regards

Anatol