FAQ
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

Search Discussions

  • Pierre Joye at Mar 14, 2012 at 8:32 pm
    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 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
  • Rasmus Lerdorf at Mar 14, 2012 at 9:38 pm

    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
  • Anthony Ferrara at Mar 14, 2012 at 9:46 pm

    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
  • Rasmus Lerdorf at Mar 14, 2012 at 10:06 pm

    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
  • Pierre Joye at Mar 14, 2012 at 10:05 pm
    hi,
    On Wed, Mar 14, 2012 at 10:38 PM, Rasmus Lerdorf 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,
  • Stas Malyshev at Mar 14, 2012 at 10:11 pm
    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
  • Rasmus Lerdorf at Mar 14, 2012 at 11:04 pm

    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
  • Ángel González at Mar 14, 2012 at 10:48 pm

    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.
  • Ilia Alshanetsky at Mar 14, 2012 at 11:27 pm
    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 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
  • Rasmus Lerdorf at Mar 14, 2012 at 11:38 pm

    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
  • Tjerk Anne Meesters at Mar 15, 2012 at 2:34 am

    On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf 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.
  • Rasmus Lerdorf at Mar 15, 2012 at 4:06 am

    On 03/14/2012 07:34 PM, Tjerk Anne Meesters wrote:
    On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf 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
  • Tjerk Anne Meesters at Mar 15, 2012 at 4:25 am

    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 ;-)
  • Ryan McCue at Mar 15, 2012 at 6:15 am

    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.
  • Stas Malyshev at Mar 15, 2012 at 6:41 am
    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
  • Ryan McCue at Mar 15, 2012 at 10:01 am

    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.
  • Tjerk Anne Meesters at Mar 15, 2012 at 11:06 am

    On Thu, Mar 15, 2012 at 6:01 PM, Ryan McCue 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 :)
  • Richard Lynch at Mar 15, 2012 at 3:07 pm

    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?
  • Rasmus Lerdorf at Mar 15, 2012 at 7:01 am

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMar 14, '12 at 7:42p
activeMar 15, '12 at 3:07p
posts20
users9
websitephp.net

People

Translate

site design / logo © 2018 Grokbase