[PHP-INTERNALS] another fix for max_input_vars.

Hi!
Could you please explain what exactly causes process exit and how it can
be reproduced? I.e., what is the problem in the patch that is committed now?

Hi:
sorry, I didn't put that clearly, I mean, in trigger E_ERROR in
php_register_variable will cause php-cgi process exit.

and dmitry has fixed that by change the E_ERROR to E_WARNING,

my patch is doing the same thing but in another way

thanks

Hmm, after a deep thought, this patch will not work in case of sub
arrays in POST ..

thanks

Hi:
I have updated the patch, make it works in case of sub arrays.

http://pastebin.com/yPTUZuNe

thanks

this patch only restrict the post variables number, since GET and
Cookie all have their length limit.

and it's also easy to restrict the get or request too(add the samilar
logic in php_default_treat_data), I just think that is no-needed :)

thanks

I don't think adding one more .ini option is a good idea.
That will lead to people confused, and regarding security parameters, that
is never a good idea.

For example, people would ask what is the difference between max_input_vars
and max_post_vars ?

Julien.Pauli

hi,

There is no other option.

This value is used before a script even get the hand. So we have to
set a value by default, but we cannot force it, that's why we have to
use an ini setting.

Cheers,

Hi!
I'm sorry, I'm not sure still I understand - what is this patch fixing?
I.e. what is the problem with the current PHP that needs this patch?

the max_input_vars was introduced to fix
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-4885

My understanding is that the original patch was too intrusive, from the
comment of Laruence, it seems that throwing an E_ERROR in that phase means
that you can kill the cgi workers if you pass the malicious input, which
has still some DOS potential, this seems to be backed as Dmitry changed the
original patch, to only raise a warning and ignore the vars over the set
limits: http://svn.php.net/viewvc?view=revision&revision=321335

From the comments by Laruence it seems that he thinks that we only need to
limit post, as get and cookie has external limits.
I disagree with this, for two reasons:
- we also have post_max_size to limit the length of the Post
Content-Length, so by the same logic, we wouldn't need an additional ini
setting for limiting the number of variables.
- we also have max_input_nesting_level, so having a max_input_vars seems to
be more consistent, than max_post_vars

I would also like to point out, that when we added max_input_nesting_level
we later polished that a little bit:
http://svn.php.net/viewvc/php/php-src/trunk/main/php_variables.c?r1=233940&r2=236894
http://svn.php.net/viewvc/php/php-src/trunk/main/php_variables.c?r1=236898&r2=239877

And I guess at least the information disclosure part would be needed here
also ( https://twitter.com/#!/i0n1c/status/152356767601393665 )

Hi!
I don't think it's a big problem if we limit all of them. It's not like
anybody needs a million cookies in their http request.
Could you please elaborate on that part - where is the disclosure and
what exactly is being disclosed?

I would guess that the value of that said limit. (it is the only variable
in the error message).

Hi!
This is an error message, it's not visible to anybody. Even if it were,
I don't see a problem with it. Usually people mean different thing by
information disclosure, but without proper report of course it is
meaningless to talk about it.

/* do not output the error message to the screen,
this helps us to to avoid "information disclosure" */

I don't think that it is a high importance, but with display_errors
enabled, it does leak otherwise unobtainable (if you don't have publicly
available phpinfo files, which most person with enabled display_errors
does) info.

So while I don't feel strongly about it, I wanted to mention it.

Since it is one of these remotely-triggered errors that you can't
program around, it should probably be suppressed when display_errors is
on. There is another precedence for this, but I am drawing a blank on
where else we did this right now.

-Rasmus

You mean like with htmlspecialchars() before PHP 5.4? Please don't.
It's really non-obvious to start hiding errors as soon as you enable
error display.

Nikita

But there is a very valid security concern here. People can usually run
safely with display_errors enabled if their code is well-written. They
can check for potential errors and avoid them. This one can't be checked
for and you could easily write a scanner that scoured the Net for sites
with display_errors enabled by sending a relatively short POST request
to each one and checking for this error. And there is absolutely nothing
people could do about this short of turning off display_errors which we
know from experience a lot of people just don't do no matter how much we
suggest it.

The alternative is to just not have any error message at all. That
avoids the discrepancy between the error messages with display_errors on
and off.

-Rasmus

Inline.
I agree with Rasmus here. A lot of people keep display_errors on, even
when they shouldn't.
It log_errors is on, it should go to the error_log, but with
display_errors it should never be sent back to the browser.

- Paul Dragoonis.

I simply fear that we are facing the same problem with this as we did
with htmlspecialchars(). Sure, the behavior was meant nice in the
first place, but in reality it basically made debugging hard for the
good guys (you know, with display_errors=Off on production and =On on
development), because they wouldn't see the error while developing.

I can imagine the same with this: If you develop some application
using lots and lots of POST variables (1000+) [for whatever reason]
and you notice that some of those vars just don't submit you'll have a
really hard time debugging this. With the warning the issue on the
other hand would be obvious.

I really don't like the idea to punish normal developers with hard
debugging only to make life easier for terrible server admins. I
understand that as this is a security issue things are a bit more
complicated, but I still don't really think this is a good idea.

By the way, an attacker could also find out the limit by measuring
page load times with various POST data sizes quite easily (I would
think so at least), so I wouldn't say it's "otherwise unobtainable".

Nikita

it is not the job of a programming language stop admins
from beeing stupid - the defaults have to be sane and this
is display_error OFF, if somebody decides for whateever
reason to turn it on it is not yours or anybody others
decision to ignore the setting here, and there and there also
but there not
damned this sort of decisions MUST NOT happen

if i debug a webiste on a local network and enable
display_errors you have to display them without any
fuzzy logic

Yes, but display_errors is not off by default, that is the problem. If
we could get away with turning display_errors off by default, then I
agree that we don't need this. As it is currently, the default setup,
if people don't do anything, will result in a security problem because
of this.

-Rasmus

I just got the tip that this error is only shown if display_startup_errors
is set to true, and because it is in the startup routine the file path in
the error message (which is the real infoleak) would only point to "unknown
0".
If somebody has the time to double check/test this, it would be nice.

Right, like I said in my previous message, if this is caught by
display_start_errors, I am ok with it. We need the default/no php.ini
file case to not leak information like this.

-Rasmus

Hi!
Just checked - it does not display error if display_startup_errors if
off, does display if it's on.

Right, that seems ok. The other thing is that we need to clarify that it
actually only limits the number of variables per nesting level. The
current name and the description doesn't make that clear. You can still
send 1M post vars in a single POST if you just nest them in a 1000x1000
2d array. Of course, this is likely going to hit the post_max_size
limit, although many sites that do file uploads will have cranked that
way up.

-Rasmus

Oh, and a final issue to address.

This code:

for($data=[],$i=0; $i<=999; $i++) $data[$i] = range(0,1001);
echo curl_post("http://localhost/index.php",['a'=>$data]);

will spew the warning 2000 times.

& php post.php | grep Warning | wc -l
2000

-Rasmus

could you try with this new patch:
https://bugs.php.net/patch-display.php?bug_id=60655&patch=max_input_vars.patch&revision=latest
?

this patch also restrict the json / serializer , all of them must
less than PG(max_input_vars).

and different with the fix which was commited now, this patch count
the num vars in a global scope, that means if there are 2 elements
which both have 500 elements in post, the restriction will also
affect,

and this patch will not affect the existsing called to json or
serializer, only affect the zif_json_decode and zif_serialize,
after patch, the serialize will have a sencode parameter :"$max_vars".

and the restriction can also be closed by set max_input_vars to 0.

thanks

Right, I don't think this is going to work. A simple 'make install'
after applying your patch fails with:

Warning: unserialize(): Unserialized variables exceeded 1000 in
phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php
on line 1145

Warning: unserialize(): Unserialized variables exceeded 1000 in
phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php
on line 1145

Warning: unserialize(): Unserialized variables exceeded 1000 in
phar:///home/rasmus/php-src/branches/PHP_5_4/pear/install-pear-nozlib.phar/PEAR/Registry.php
on line 1145
[PEAR] PEAR: pear.php.net/PEAR not installed

I really don't think this manual per-feature limiting is going to cut it
here. The real problem is the predictability of the hashing which we can
address by seeding it with a random value. That part is easy enough, the
hard part is figuring out where people may be storing these hashes
externally and providing some way of associating the seed with these
external caches so they will still work.

-Rasmus

Hi:
that will be a better fix, we have disscussed this before in irc, I
also can have a try in that way.

and for this fix(in such way), to be honest, yes, this is a little
ugly fix, but quick.

the default 1000 is a little insufficient when counting the elements
in a global scope.

IMO it should set to be 4096, then I think 99% of developers will
not see this warning at all.

thanks

Hi!
I think we'd need and API to access the seed value and to calculate hash
for given seed value. That would probably allow extensions that store
hashes with some additional work to do it properly. Though it needs more
detailed investigation.

Yes, but we still need an actual case to look at. Opcode caches
shouldn't be a problem unless they store some representation on disk
that live across server restarts. In the APC world, nobody does that. Is
there something in common use out there that actually needs this?

Let's do just the GPC fix (the Dmitry version) for 5.3 and turn on
ignore_repeated_errors just during startup and get it out there. That
takes care of the most obvious attack vector for existing users. Leaving
this in place for 5.4 is fine regardless of what we do in 5.4.

I think for 5.4 we should take a couple of days to dig into what would
actually break from seeding the hash. This seems like a much more
elegant solution compared to trying to add limits to all the other
places manually. This manual limit checking also wouldn't cover 3rd
party extensions or even userspace code that might be vulnerable to the
same thing. The only way to fix those cases is with a central hash fix.

Another alternative to seeding would be to use a different hashing
algorithm altogether. That would solve the cross-server issues, at the
likely cost of slower hashing.

-Rasmus

Hi!
It's not that simple. Take example of FastCGI setup where processes can
live and die independently (note that FastCGI does not mandate
single-parent fork and in fact on Windows it doesn't work that way). In
this setup opcode cache is shared between processes which may have
different hash value, unless we give means to the cache to set this
value on startup somehow.

Hi:
there is one way maybe is a good try.

when resize hashtable, we don't just dobule the size, instead, we
increase the hashtable size with a random delta

what do you think?

thanks

Sorry, you lost me. How does that help? The problem is when we collide
on a single hash key the resulting linked list traversion gets longer
and longer as more colliding keys are added to that hashtable. Whether
you double the size or grow it by some other factor doesn't change this.

-Rasmus

No, No,

if we increase the table size with a random delta(the nTableMask will
also be random), then the collision will not be predictable.

thanks

I have made a patch, plz try this mechanism,
https://bugs.php.net/patch-display.php?bug_id=60655&patch=rand_hash_resize.patch&revision=latest

I have tested , that works.

and the patch is still a rough draft, so maybe some compile warnings..

thanks

the key point is, increase the table size in a random delta (2 * size
+ (random_num & size_mask));

in the same time use mod(%) instead of and(&) while doing the index mapping.

then the attacker will be not able to predicate the collision ;)

thanks.

hrm, ok, yes for non single parent forks it is an issue. For APC there
is no such thing, although people have been asking for it.

With a clean API this doesn't sound insurmountable though.

get_hash_seed()/set_hash_seed() internal functions. When the cache is
created the first time call get_hash_seed() and store it somewhere. When
someone connects to an existing cache the first time, fetch the stored
hash seed and call set_hash_seed()

-Rasmus

bcompiler would need it

cheers,
Derick

Hi!
Why? The point of the limitation is to avoid hash collisions and related
performance problems, but if they are in different elements, what is the
point of limiting them?

Hi, this patch is aim at a quick/simple fix than before, that is why I
proposal this patch.

actually, there might be no attack even a array has more than 1000 elements,

I mean, this is a simple / quick fix but works the same.

thanks

if it is well written there would be nor errors displayed
but you miss - in production you MUST NOT dispaly errors
does not matter

if display_errors is on DISPLAY it

if it is off do NOT
there is nothing between

every try to make exceptions here is simply a bad style and should
not be done - where do you stop? you can't decide - only the
admin or developer with ini_set() has to decide and nobody else

Hi!
Oh no. Nobody should or can safely run production with display_errors.
Everybody thinks their code is well-written, but display_errors should
never be enabled in production, however high is your opinion of the code.
I'm afraid people now will start quoting this saying "ok, yeah, if
you're a bad programmer, disable display_errors, but I'm a good
programmer, my code is solid, I even have a dozen of unit tests, so I
just go ahead and enable display_errors" and then we have this sad state
of affairs where sites spill out error messages that are never supposed
to be seen by clients because developers thought it can never happen.
I don't think it's a good idea to add such magic, as correctly noted, it
will make people that are working properly - display off in production,
on in development - work harder and have trouble, all in the name of
cuddling people that run misconfigured servers and ignore the advice
that is being repeated for years by now.

On shared hosts display_errors typically is on, but the application can
do ini_set('display_errors', 0) or such ...

johannes

Hi Johannes,

as far as I understood the issue, this error would be triggered before the application's code is executed, so that would not solve this issue.



Cheers
Jannik

Am 04.01.2012 um 21:46 schrieb Johannes Schlüter:

That's the point. Thanks for making it clear. :-)

johannes

Hi!
Hmm... shared hosts. OK, let's make it the same way as the other input
message then. I don't like either of them but at least they'd do the
same thing (and when somebody finally has an idea how to properly fix it
it would be the same in both places).

Hi again,


Am 04.01.2012 um 21:52 schrieb Johannes Schlüter:

what could work nevertheless (if the server is an apache and htaccess is enabled) is setting display_errors to off with `php_flag display_errors off`, I guess.



Cheers
Jannik

But that is precisely why this is a special case. Even if you do
ini_set('display_errors', 0) in your code this message will still be
displayed. Although, display_startup_errors is off by default and
hopefully this one falls under that setting. I didn't test it, but if it
doesn't then we need to make sure it is suppressed when
display_startup_errors is off.

-Rasmus