# Let parse_str() parse more than max_input_vars args - Thread: https://externals.io/message/58933 - Original archive: https://news-web.php.net/php.internals/58933 - Started by Rasmus Lerdorf on 2012-03-14T19:42:41+00:00 - Messages: 20 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #58933 — Rasmus Lerdorf — 2012-03-14T19:42:41+00:00 URL: https://externals.io/message/58933#58933 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 --- ## #58936 — Pierre Joye — 2012-03-14T20:32:05+00:00 URL: https://externals.io/message/58933#58936 · In reply to: #58933 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 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 > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #58938 — Rasmus Lerdorf — 2012-03-14T21:38:01+00:00 URL: https://externals.io/message/58933#58938 · In reply to: #58936 > 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 --- ## #58951 — Ryan McCue — 2012-03-14T21:40:21+00:00 URL: https://externals.io/message/58933#58951 · In reply to: #58933 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 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 --- ## #58940 — Pierre Joye — 2012-03-14T22:05:01+00:00 URL: https://externals.io/message/58933#58940 · In reply to: #58938 hi, > > > […] > > 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 --- ## #58941 — Rasmus Lerdorf — 2012-03-14T22:06:13+00:00 URL: https://externals.io/message/58933#58941 · In reply to: #58939 > > […] > > 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 --- ## #58942 — Stas Malyshev — 2012-03-14T22:11:54+00:00 URL: https://externals.io/message/58933#58942 · In reply to: #58933 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 --- ## #58943 — keisial@gmail.com — 2012-03-14T22:53:11+00:00 URL: https://externals.io/message/58933#58943 · In reply to: #58933 > 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. --- ## #58944 — Rasmus Lerdorf — 2012-03-14T23:04:29+00:00 URL: https://externals.io/message/58933#58944 · In reply to: #58942 > Hi! > > > […] > > 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 --- ## #58945 — Ilia Alshanetsky — 2012-03-14T23:27:03+00:00 URL: https://externals.io/message/58933#58945 · In reply to: #58933 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 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 --- ## #58947 — Rasmus Lerdorf — 2012-03-14T23:38:30+00:00 URL: https://externals.io/message/58933#58947 · In reply to: #58945 > 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 --- ## #58948 — Tjerk Anne Meesters — 2012-03-15T02:34:15+00:00 URL: https://externals.io/message/58933#58948 · In reply to: #58947 > > 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. --- ## #58949 — Rasmus Lerdorf — 2012-03-15T04:06:00+00:00 URL: https://externals.io/message/58933#58949 · In reply to: #58948 > > > […] > > 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 --- ## #58950 — Tjerk Anne Meesters — 2012-03-15T04:25:40+00:00 URL: https://externals.io/message/58933#58950 · In reply to: #58949 > > […] > > 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 ;-) --- ## #58952 — Stas Malyshev — 2012-03-15T06:41:52+00:00 URL: https://externals.io/message/58933#58952 · In reply to: #58951 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 --- ## #58953 — Rasmus Lerdorf — 2012-03-15T07:00:56+00:00 URL: https://externals.io/message/58933#58953 · In reply to: #58951 > Rasmus Lerdorf 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. 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 --- ## #58954 — Ryan McCue — 2012-03-15T10:01:45+00:00 URL: https://externals.io/message/58933#58954 · In reply to: #58952 Stas Malyshev wrote: > Hi! > > > […] > > 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 > […] > > 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 :) --- ## #58961 — Richard Lynch — 2012-03-15T15:07:02+00:00 URL: https://externals.io/message/58933#58961 · In reply to: #58954 > 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