It is somewhat unintuitive that parse_str() is subject to the
max_input_vars limitation and there are sites that use parse_str() to
parse things that aren't directly coming from user query args.
There arr two ways to solve this. We could add an optional max_vars arg
something along these lines:
https://gist.github.com/2038870
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let people ini_set() their way around the limit.
The one drawback with the optional arg approach is that since
parse_str() is a thin layer on top of the query string parser, the error
if you exceed the passed max_vars talks about the ini setting which in
this case wouldn't really be correct and fixing that would be complicated.
-Rasmus
hi Rasmus,
As the ini_all option sounds appealing, I can imagine ISPs willing to
do not allow their users to change this value, and that's something I
would not allow random users either.
I'd to go with the optional argument, adding a clear in the
documentation about the confusing error message.
Cheers,
It is somewhat unintuitive that
parse_str()is subject to the
max_input_vars limitation and there are sites that useparse_str()to
parse things that aren't directly coming from user query args.
There arr two ways to solve this. We could add an optional max_vars arg
something along these lines:https://gist.github.com/2038870
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.The one drawback with the optional arg approach is that since
parse_str()is a thin layer on top of the query string parser, the error
if you exceed the passed max_vars talks about the ini setting which in
this case wouldn't really be correct and fixing that would be complicated.-Rasmus
--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
hi Rasmus,
As the ini_all option sounds appealing, I can imagine ISPs willing to
do not allow their users to change this value, and that's something I
would not allow random users either.I'd to go with the optional argument, adding a clear in the
documentation about the confusing error message.
But Pierre, you understand that by the time you ini_set() it in the code
it can only ever affect parse_str() calls. Normal GPC parsing is done
prior to the PHP script running so there is no way for a userspace
script to ini_set() themselves to a state where they will be insecure to
a remote attack. They would have to go out of their way to specifically
write code to do that and that is something they can obviously do anyway
by simply building a big hash from some external source. So I don't
really think this is a valid concern. If this was a real concern I would
think you would have objected to the current INI_PERDIR. This is where a
user can make his scripts unsafe by disabling max_input_vars in a
.htaccess file.
-Rasmus
But Pierre, you understand that by the time you
ini_set()it in the code
it can only ever affectparse_str()calls.
Well, wouldn't INI_ALL would allow .htaccess files to override that
statement, and hence open the vulnerability?
Anthony
But Pierre, you understand that by the time you
ini_set()it in the code
it can only ever affectparse_str()calls.Well, wouldn't
INI_ALLwould allow .htaccess files to override that
statement, and hence open the vulnerability?
No because it is already PERDIR.
-Rasmus
hi,
hi Rasmus,
As the ini_all option sounds appealing, I can imagine ISPs willing to
do not allow their users to change this value, and that's something I
would not allow random users either.I'd to go with the optional argument, adding a clear in the
documentation about the confusing error message.I would think you would have objected to the current INI_PERDIR. This is where a
user can make his scripts unsafe by disabling max_input_vars in a
.htaccess file.
I was sure it was system and not per dir.
The ini all option can't hurt more then.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Rasmus Lerdorf wrote:
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.
That's probably going to confuse people if it doesn't change the HTTP
request parsing limit too, IMHO. And I'm not sure that's something we
necessarily want.
--
Ryan McCue
<http://ryanmccue.info/
Hi!
That's probably going to confuse people if it doesn't change the HTTP
request parsing limit too, IMHO. And I'm not sure that's something we
necessarily want.
Err, how you can change it after HTTP request has already been parsed?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Stas Malyshev wrote:
Hi!
That's probably going to confuse people if it doesn't change the HTTP
request parsing limit too, IMHO. And I'm not sure that's something we
necessarily want.Err, how you can change it after HTTP request has already been parsed?
I'm not arguing that it should, I'm saying that in the INI it refers to
the HTTP arguments, while in the code (via ini_set) it would not affect
this. I think that could be confusing for users who don't realise the
script is only loaded after parsing the request.
Rasmus Lerdorf wrote:
I think that is a documentation issue. We already have plenty of
confusion here because it isn't documented thatparse_str()is affected
by the max_input_vars setting.
I think that is definitely a documentation issue, but the extra argument
is the way to go in terms of being the least confusing option.
--
Ryan McCue
<http://ryanmccue.info/
That's probably going to confuse people if it doesn't change the HTTP
request parsing limit too, IMHO. And I'm not sure that's something we
necessarily want.Err, how you can change it after HTTP request has already been parsed?
I'm not arguing that it should, I'm saying that in the INI it refers to the
HTTP arguments, while in the code (via ini_set) it would not affect this. I
think that could be confusing for users who don't realise the script is only
loaded after parsing the request.
That's something I hadn't even considered, that by turning the setting
into INI_ALL the expected behaviour is in fact broken when using
ini_set(); it doesn't do the same thing any more.
I think that is definitely a documentation issue, but the extra argument is
the way to go in terms of being the least confusing option.
After looking back at my previous comment I realized that, between the
two suboptimal fixes, adding the extra argument is actually the better
one; I see now that it makes a clearer statement that changing this
ini setting can only be done inside the more privileged code of
parse_str().
I had reasoned before that by having to use ini_set() the developer
would be more aware of the linkage between parse_str and
max_input_vars, but a good example in the parse_str() documentation
should be able to address that and the "strange" error message one
would see if the limits are exceeded :)
I'm not arguing that it should, I'm saying that in the INI it refers
to
the HTTP arguments, while in the code (via ini_set) it would not
affect
this. I think that could be confusing for users who don't realise the
script is only loaded after parsing the request.
If they don't know it by the time they need parse_str, it's time for
them to learn it... :-)
Not saying it won't cause confusion: But they have to make some effort
to figure out how PHP works.
Maybe a link in the error message to something that documents the
workflow clearly enough that they "get it" just by clicking on the
link and reading?
--
brain cancer update:
http://richardlynch.blogspot.com/search/label/brain%20tumor
Donate:
https://www.paypal.com/cgi-bin/webscr?cmd=_s-xclick&hosted_button_id=FS9NLTNEEKWBE
Rasmus Lerdorf wrote:
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.That's probably going to confuse people if it doesn't change the HTTP
request parsing limit too, IMHO. And I'm not sure that's something we
necessarily want.
I think that is a documentation issue. We already have plenty of
confusion here because it isn't documented that parse_str() is affected
by the max_input_vars setting.
There is no perfect solution to this one. We need the least
destabilizing fix for the inadvertent breakage we caused in 5.3. I think
we were all under the impression that it was really unlikely that a
default limit of 1000 input vars would cause a problem, and even in the
rare case where it did, people could increase it. What we didn't take
into account was that there were backend pieces affected by this
frontend restriction and we didn't provide a way to decouple the two.
Making max_input_vars PHP_INI_ALL is the least intrusive way to remedy
this in the stable 5.3.x tree.
-Rasmus
Hi!
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.
I think making it PHP_INI_ALL is OK. If you have access to a way to
change INI_ALL vars, that means you can run code on that system, then
DoS protection is meaningless on this stage.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.I think making it PHP_INI_ALL is OK. If you have access to a way to
changeINI_ALLvars, that means you can run code on that system, then
DoS protection is meaningless on this stage.
I ran into this in some existing code that broke upgrading to 5.3.10. It
was a backend call to an API where the API result was being fed to
parse_str(). The API itself is trusted, so no risk of a HashDoS from it.
Other than replacing the call to parse_str() with a similar userspace
implementation there was no way to fix this safely. I could do a
.htaccess for just that URI, but that would open up the frontend of this
particular request to a HashDoS.
I'll make the INI_ALL change for the next release.
-Rasmus
It is somewhat unintuitive that
parse_str()is subject to the
max_input_vars limitation and there are sites that useparse_str()to
parse things that aren't directly coming from user query args.
There arr two ways to solve this. We could add an optional max_vars arg
something along these lines:https://gist.github.com/2038870
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.The one drawback with the optional arg approach is that since
parse_str()is a thin layer on top of the query string parser, the error
if you exceed the passed max_vars talks about the ini setting which in
this case wouldn't really be correct and fixing that would be complicated.-Rasmus
Configuring it through ini_set would be a hack.
+1 for doing it by a new str_parse() parameter. I'm not really keen of
implementing
that setting and restoring PG(max_input_vars), but as a fast fix, it's ok.
Sounds like a least dangerous way of solving the problem to me. The
only issue I can see with this fix is what would happen is if after
the "PG(max_input_vars) = max_vars; "
call the request got interrupted in persistent environment such as
Apache (mod_php). Wouldn't that means that for subsequent requests the
value would not be equal to the one set by the user?
It is somewhat unintuitive that
parse_str()is subject to the
max_input_vars limitation and there are sites that useparse_str()to
parse things that aren't directly coming from user query args.
There arr two ways to solve this. We could add an optional max_vars arg
something along these lines:https://gist.github.com/2038870
The other way to solve this would be to make max_input_vars PHP_INI_ALL
and then just let peopleini_set()their way around the limit.The one drawback with the optional arg approach is that since
parse_str()is a thin layer on top of the query string parser, the error
if you exceed the passed max_vars talks about the ini setting which in
this case wouldn't really be correct and fixing that would be complicated.-Rasmus
Sounds like a least dangerous way of solving the problem to me. The
only issue I can see with this fix is what would happen is if after
the "PG(max_input_vars) = max_vars; "
call the request got interrupted in persistent environment such as
Apache (mod_php). Wouldn't that means that for subsequent requests the
value would not be equal to the one set by the user?
Yes, it would need a zend_alter_ini_entry_ex() call there. The patch
wasn't complete, just a quick hack. But it would still have an
out-of-context error message when you exceed it. At least with a
userspace ini_set() the error would make sense.
-Rasmus
Yes, it would need a zend_alter_ini_entry_ex() call there. The patch
wasn't complete, just a quick hack. But it would still have an
out-of-context error message when you exceed it. At least with a
userspaceini_set()the error would make sense.
As mentioned on IRC, a function signature of "array
parse_urlencoded(string $s)" would be useful too; the separated logic
would allow for avoiding max_input_vars altogether and not having to
worry about parameter name mangling (variable name rules). The nasty
part is that much of the treat_data code would have to be duplicated
(I think).
Besides that, applying the hash randomisation patch to only userland
arrays would make the max_input_vars less critical and at the same
time avoid breaking opcode caches and other low-level dependencies.
Yes, it would need a zend_alter_ini_entry_ex() call there. The patch
wasn't complete, just a quick hack. But it would still have an
out-of-context error message when you exceed it. At least with a
userspaceini_set()the error would make sense.As mentioned on IRC, a function signature of "array
parse_urlencoded(string $s)" would be useful too; the separated logic
would allow for avoiding max_input_vars altogether and not having to
worry about parameter name mangling (variable name rules). The nasty
part is that much of the treat_data code would have to be duplicated
(I think).Besides that, applying the hash randomisation patch to only userland
arrays would make the max_input_vars less critical and at the same
time avoid breaking opcode caches and other low-level dependencies.
Sure, but this is a longer-term fix. Right now I am more concerned about
the fact that we broke code that worked fine in PHP 5.3.8 without any
way to make it work safely.
-Rasmus
As mentioned on IRC, a function signature of "array
parse_urlencoded(string $s)" would be useful too; the separated logic
would allow for avoiding max_input_vars altogether and not having to
worry about parameter name mangling (variable name rules). The nasty
part is that much of the treat_data code would have to be duplicated
(I think).Besides that, applying the hash randomisation patch to only userland
arrays would make the max_input_vars less critical and at the same
time avoid breaking opcode caches and other low-level dependencies.Sure, but this is a longer-term fix. Right now I am more concerned about
the fact that we broke code that worked fine in PHP 5.3.8 without any
way to make it work safely.
I guess this looks acceptable then:
ini_set('max_input_vars', 5000);
parse_str($s, $arr);
ini_restore('max_input_vars');
Although, arguably the last statement would not be needed, since all
input has already been processed ;-)