[PHP-DEV] Let parse_str() parse more than max_input_vars args

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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,

On Wed, Mar 14, 2012 at 8:42 PM, Rasmus Lerdorf <[email protected]> wrote:
> 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>



--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 01:32 PM, Pierre Joye wrote:
> 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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

> But Pierre, you understand that by the time you ini_set() it in the code
> it can only ever affect parse_str() calls.

Well, wouldn't INI_ALL would allow .htaccess files to override that
statement, and hence open the vulnerability?

Anthony

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Wed, Mar 14, 2012 at 10:38 PM, Rasmus Lerdorf <[email protected]> wrote:
> On 03/14/2012 01:32 PM, Pierre Joye wrote:
>> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 02:46 PM, Anthony Ferrara wrote:
>> But Pierre, you understand that by the time you ini_set() it in the code
>> it can only ever affect parse_str() calls.
>
> Well, wouldn't INI_ALL would allow .htaccess files to override that
> statement, and hence open the vulnerability?

No because it is already PERDIR.

-Rasmus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Hi!

> 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.

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 14/03/12 20:42, Rasmus Lerdorf wrote:
> 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
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.


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 03:11 PM, Stas Malyshev wrote:
> Hi!
>
>> 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.
>
> 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.

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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?

On Wed, Mar 14, 2012 at 3:42 PM, Rasmus Lerdorf <[email protected]> wrote:
> 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 04:27 PM, Ilia Alshanetsky wrote:
> 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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf <[email protected]> wrote:
>
> 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.
>

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.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 07:34 PM, Tjerk Anne Meesters wrote:
> On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf <[email protected]> wrote:
>>
>> 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.
>>
>
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

>> 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 ;-)

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Rasmus Lerdorf wrote:
> 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.

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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 03/14/2012 02:40 PM, Ryan McCue wrote:
> Rasmus Lerdorf wrote:
>> 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.
>
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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 that parse_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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On Thu, Mar 15, 2012 at 6:01 PM, Ryan McCue <[email protected]> wrote:
>>> 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 :)

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On Thu, March 15, 2012 5:01 am, Ryan McCue wrote:
> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php