Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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-xclickhosted_button_id=FS9NLTNEEKWBE -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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 ras...@lerdorf.com 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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
hi, On Wed, Mar 14, 2012 at 10:38 PM, Rasmus Lerdorf ras...@lerdorf.com 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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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 ras...@lerdorf.com 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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf ras...@lerdorf.com 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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
On 03/14/2012 07:34 PM, Tjerk Anne Meesters wrote: On Thu, Mar 15, 2012 at 7:38 AM, Rasmus Lerdorf ras...@lerdorf.com 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
Re: [PHP-DEV] Let parse_str() parse more than max_input_vars args
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
