Hi Thomas, > -----Original Message----- > From: Thomas Hruska [mailto:thru...@cubiclesoft.com] > Sent: Saturday, November 28, 2015 12:23 AM > To: Yasuo Ohgaki <yohg...@ohgaki.net> > Cc: PHP internals <internals@lists.php.net> > Subject: Re: [PHP-DEV] HashDos protection > > On 11/27/2015 2:21 PM, Yasuo Ohgaki wrote: > > Hi Thomas, > > > > In practice, we wouldn't have problems with max number of collisions. > > 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. > > 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. > > I abuse PHP arrays. Especially on CLI. Sorry. > 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
