# another fix for max_input_vars. - Thread: https://externals.io/message/57174 - Original archive: https://news-web.php.net/php.internals/57174 - Started by Laruence on 2012-01-04T06:59:36+00:00 - Messages: 49 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/57174.md?page=2 - 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 --- ## #57174 — Laruence — 2012-01-04T06:59:36+00:00 URL: https://externals.io/message/57174#57174 Hi dmitry: it seems you have fix the issue that error in register_variable will cause php process exit. here is a fix I made before: http://pastebin.com/7BLAVaWr , I think maybe this is a lighter fix. could you review this? if you think this is okey, I will commit it. thanks very much. -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #57175 — Stas Malyshev — 2012-01-04T07:08:23+00:00 URL: https://externals.io/message/57174#57175 · In reply to: #57174 Hi! > it seems you have fix the issue that error in register_variable > will cause php process exit. > > here is a fix I made before: http://pastebin.com/7BLAVaWr , I > think maybe this is a lighter fix. > > could you review this? if you think this is okey, I will commit it. Could you please explain what exactly causes process exit and how it can be reproduced? I.e., what is the problem in the patch that is committed now? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57176 — Laruence — 2012-01-04T07:13:24+00:00 URL: https://externals.io/message/57174#57176 · In reply to: #57175 Hi: sorry, I didn't put that clearly, I mean, in trigger `E_ERROR` in php_register_variable will cause php-cgi process exit. and dmitry has fixed that by change the `E_ERROR` to E_WARNING, my patch is doing the same thing but in another way thanks > Hi! > > > > […] > > > Could you please explain what exactly causes process exit and how it can be > reproduced? I.e., what is the problem in the patch that is committed now? > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #57180 — Laruence — 2012-01-04T09:59:48+00:00 URL: https://externals.io/message/57174#57180 · In reply to: #57174 > Hi dmitry: > >    it seems you have fix the issue that error in register_variable > will cause php process exit. > >    here is a fix I made before: http://pastebin.com/7BLAVaWr ,  I > think maybe this is a lighter fix. > >    could you review this?   if you think this is okey,  I will commit it. Hmm, after a deep thought, this patch will not work in case of sub arrays in POST .. thanks > >    thanks very much. > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #57181 — Laruence — 2012-01-04T11:34:24+00:00 URL: https://externals.io/message/57174#57181 · In reply to: #57180 Hi: I have updated the patch, make it works in case of sub arrays. http://pastebin.com/yPTUZuNe thanks > > > […] > Hmm, after a deep thought, this patch will not work in case of  sub > arrays in POST .. > > thanks > > […] > > > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #57182 — Laruence — 2012-01-04T11:52:52+00:00 URL: https://externals.io/message/57174#57182 · In reply to: #57181 > Hi: >   I have updated the patch, make it works in case of sub arrays. > >  http://pastebin.com/yPTUZuNe this patch only restrict the post variables number, since GET and Cookie all have their length limit. and it's also easy to restrict the get or request too(add the samilar logic in php_default_treat_data), I just think that is no-needed :) thanks > > thanks > > > > […] > > > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #57183 — jpauli — 2012-01-04T13:30:07+00:00 URL: https://externals.io/message/57174#57183 · In reply to: #57182 > > > […] > > this patch only restrict the post variables number, since GET and > Cookie all have their length limit. > > and it's also easy to restrict the get or request too(add the samilar > logic in php_default_treat_data), I just think that is no-needed :) > > thanks > I don't think adding one more .ini option is a good idea. That will lead to people confused, and regarding security parameters, that is never a good idea. For example, people would ask what is the difference between max_input_vars and max_post_vars ? Julien.Pauli > > […] > it. > > […] > > > > -- > Laruence Xinchen Hui > http://www.laruence.com/ --- ## #57184 — Pierre Joye — 2012-01-04T13:59:09+00:00 URL: https://externals.io/message/57174#57184 · In reply to: #57183 hi, There is no other option. This value is used before a script even get the hand. So we have to set a value by default, but we cannot force it, that's why we have to use an ini setting. Cheers, > > > > […] > > I don't think adding one more .ini option is a good idea. > That will lead to people confused, and regarding security parameters, that > is never a good idea. > > For example, people would ask what is the difference between max_input_vars > and max_post_vars ? > > Julien.Pauli > > > > […] -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #57190 — Stas Malyshev — 2012-01-04T17:18:59+00:00 URL: https://externals.io/message/57174#57190 · In reply to: #57181 Hi! > Hi: > I have updated the patch, make it works in case of sub arrays. > > http://pastebin.com/yPTUZuNe I'm sorry, I'm not sure still I understand - what is this patch fixing? I.e. what is the problem with the current PHP that needs this patch? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57191 — Ferenc Kovacs — 2012-01-04T18:55:02+00:00 URL: https://externals.io/message/57174#57191 · In reply to: #57190 On Wed, Jan 4, 2012 at 6:18 PM, Stas Malyshev wrote: > Hi! > > > Hi: > > […] > > I'm sorry, I'm not sure still I understand - what is this patch fixing? > I.e. what is the problem with the current PHP that needs this patch? > > the max_input_vars was introduced to fix http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-4885 My understanding is that the original patch was too intrusive, from the comment of Laruence, it seems that throwing an `E_ERROR` in that phase means that you can kill the cgi workers if you pass the malicious input, which has still some DOS potential, this seems to be backed as Dmitry changed the original patch, to only raise a warning and ignore the vars over the set limits: http://svn.php.net/viewvc?view=revision&revision=321335 From the comments by Laruence it seems that he thinks that we only need to limit post, as get and cookie has external limits. I disagree with this, for two reasons: - we also have post_max_size to limit the length of the Post Content-Length, so by the same logic, we wouldn't need an additional ini setting for limiting the number of variables. - we also have max_input_nesting_level, so having a max_input_vars seems to be more consistent, than max_post_vars I would also like to point out, that when we added max_input_nesting_level we later polished that a little bit: http://svn.php.net/viewvc/php/php-src/trunk/main/php_variables.c?r1=233940&r2=236894 http://svn.php.net/viewvc/php/php-src/trunk/main/php_variables.c?r1=236898&r2=239877 And I guess at least the information disclosure part would be needed here also ( https://twitter.com/#!/i0n1c/status/152356767601393665 ) -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #57192 — Stas Malyshev — 2012-01-04T18:58:58+00:00 URL: https://externals.io/message/57174#57192 · In reply to: #57191 Hi! > From the comments by Laruence it seems that he thinks that we only need > to limit post, as get and cookie has external limits. I don't think it's a big problem if we limit all of them. It's not like anybody needs a million cookies in their http request. > And I guess at least the information disclosure part would be needed > here also ( https://twitter.com/#!/i0n1c/status/152356767601393665 ) Could you please elaborate on that part - where is the disclosure and what exactly is being disclosed? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57193 — Ferenc Kovacs — 2012-01-04T19:35:06+00:00 URL: https://externals.io/message/57174#57193 · In reply to: #57192 > And I guess at least the information disclosure part would be needed > > […] > > Could you please elaborate on that part - where is the disclosure and what > exactly is being disclosed? I would guess that the value of that said limit. (it is the only variable in the error message). -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #57194 — Stas Malyshev — 2012-01-04T19:37:50+00:00 URL: https://externals.io/message/57174#57194 · In reply to: #57193 Hi! > Could you please elaborate on that part - where is the disclosure > and what exactly is being disclosed? > > > I would guess that the value of that said limit. (it is the only > variable in the error message). This is an error message, it's not visible to anybody. Even if it were, I don't see a problem with it. Usually people mean different thing by information disclosure, but without proper report of course it is meaningless to talk about it. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57195 — Ferenc Kovacs — 2012-01-04T19:46:07+00:00 URL: https://externals.io/message/57174#57195 · In reply to: #57194 On Wed, Jan 4, 2012 at 8:37 PM, Stas Malyshev wrote: > Hi! > > > Could you please elaborate on that part - where is the disclosure > > […] > > This is an error message, it's not visible to anybody. Even if it were, I > don't see a problem with it. Usually people mean different thing by > information disclosure, but without proper report of course it is > meaningless to talk about it. /* do not output the error message to the screen, this helps us to to avoid "information disclosure" */ I don't think that it is a high importance, but with display_errors enabled, it does leak otherwise unobtainable (if you don't have publicly available phpinfo files, which most person with enabled display_errors does) info. So while I don't feel strongly about it, I wanted to mention it. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #57196 — Rasmus Lerdorf — 2012-01-04T19:50:05+00:00 URL: https://externals.io/message/57174#57196 · In reply to: #57195 > On Wed, Jan 4, 2012 at 8:37 PM, Stas Malyshev wrote: > > > […] > > > /* do not output the error message to the screen, > this helps us to to avoid "information disclosure" */ > > I don't think that it is a high importance, but with display_errors > enabled, it does leak otherwise unobtainable (if you don't have publicly > available phpinfo files, which most person with enabled display_errors > does) info. > > So while I don't feel strongly about it, I wanted to mention it. Since it is one of these remotely-triggered errors that you can't program around, it should probably be suppressed when display_errors is on. There is another precedence for this, but I am drawing a blank on where else we did this right now. -Rasmus --- ## #57197 — Nikita Popov — 2012-01-04T19:54:09+00:00 URL: https://externals.io/message/57174#57197 · In reply to: #57196 > Since it is one of these remotely-triggered errors that you can't > program around, it should probably be suppressed when display_errors is > on. There is another precedence for this, but I am drawing a blank on > where else we did this right now. > > -Rasmus You mean like with `htmlspecialchars()` before PHP 5.4? Please don't. It's really non-obvious to start hiding errors as soon as you enable error display. Nikita --- ## #57198 — Rasmus Lerdorf — 2012-01-04T20:02:57+00:00 URL: https://externals.io/message/57174#57198 · In reply to: #57197 > > > […] > You mean like with `htmlspecialchars()` before PHP 5.4? Please don't. > It's really non-obvious to start hiding errors as soon as you enable > error display. But there is a very valid security concern here. People can usually run safely with display_errors enabled if their code is well-written. They can check for potential errors and avoid them. This one can't be checked for and you could easily write a scanner that scoured the Net for sites with display_errors enabled by sending a relatively short POST request to each one and checking for this error. And there is absolutely nothing people could do about this short of turning off display_errors which we know from experience a lot of people just don't do no matter how much we suggest it. The alternative is to just not have any error message at all. That avoids the discrepancy between the error messages with display_errors on and off. -Rasmus --- ## #57199 — Paul Dragoonis — 2012-01-04T20:07:23+00:00 URL: https://externals.io/message/57174#57199 · In reply to: #57198 Inline. > > > […] > > But there is a very valid security concern here. People can usually run > safely with display_errors enabled if their code is well-written. They > can check for potential errors and avoid them. This one can't be checked > for and you could easily write a scanner that scoured the Net for sites > with display_errors enabled by sending a relatively short POST request > to each one and checking for this error. And there is absolutely nothing > people could do about this short of turning off display_errors which we > know from experience a lot of people just don't do no matter how much we > suggest it. I agree with Rasmus here. A lot of people keep display_errors on, even when they shouldn't. It log_errors is on, it should go to the error_log, but with display_errors it should never be sent back to the browser. - Paul Dragoonis. > > The alternative is to just not have any error message at all. That > avoids the discrepancy between the error messages with display_errors on > and off. > > -Rasmus --- ## #57201 — Reindl Harald — 2012-01-04T20:15:51+00:00 URL: https://externals.io/message/57174#57201 · In reply to: #57198 Am 04.01.2012 21:02, schrieb Rasmus Lerdorf: > But there is a very valid security concern here. People can usually run > safely with display_errors enabled if their code is well-written. if it is well written there would be nor errors displayed but you miss - in production you MUST NOT dispaly errors > They can check for potential errors and avoid them. This one can't be checked > for and you could easily write a scanner that scoured the Net for sites > with display_errors enabled by sending a relatively short POST request > to each one and checking for this error. does not matter if display_errors is on DISPLAY it if it is off do NOT there is nothing between every try to make exceptions here is simply a bad style and should not be done - where do you stop? you can't decide - only the admin or developer with `ini_set()` has to decide and nobody else --- ## #57202 — Nikita Popov — 2012-01-04T20:17:57+00:00 URL: https://externals.io/message/57174#57202 · In reply to: #57199 > Inline. > > > > […] > > I agree with Rasmus here. A lot of people keep display_errors on, even > when they shouldn't. > It log_errors is on, it should go to the error_log, but with > display_errors it should never be sent back to the browser. I simply fear that we are facing the same problem with this as we did with `htmlspecialchars()`. Sure, the behavior was meant nice in the first place, but in reality it basically made debugging hard for the good guys (you know, with display_errors=Off on production and =On on development), because they wouldn't see the error while developing. I can imagine the same with this: If you develop some application using lots and lots of POST variables (1000+) [for whatever reason] and you notice that some of those vars just don't submit you'll have a really hard time debugging this. With the warning the issue on the other hand would be obvious. I really don't like the idea to punish normal developers with hard debugging only to make life easier for terrible server admins. I understand that as this is a security issue things are a bit more complicated, but I still don't really think this is a good idea. By the way, an attacker could also find out the limit by measuring page load times with various POST data sizes quite easily (I would think so at least), so I wouldn't say it's "otherwise unobtainable". Nikita --- ## #57203 — Reindl Harald — 2012-01-04T20:19:25+00:00 URL: https://externals.io/message/57174#57203 · In reply to: #57199 Am 04.01.2012 21:07, schrieb Paul Dragoonis: > I agree with Rasmus here. A lot of people keep display_errors on, even > when they shouldn't. it is not the job of a programming language stop admins from beeing stupid - the defaults have to be sane and this is display_error OFF, if somebody decides for whateever reason to turn it on it is not yours or anybody others decision to ignore the setting here, and there and there also but there not > It log_errors is on, it should go to the error_log, but with > display_errors it should never be sent back to the browser. damned this sort of decisions MUST NOT happen if i debug a webiste on a local network and enable display_errors you have to display them without any fuzzy logic --- ## #57204 — Stas Malyshev — 2012-01-04T20:29:16+00:00 URL: https://externals.io/message/57174#57204 · In reply to: #57198 Hi! > But there is a very valid security concern here. People can usually run > safely with display_errors enabled if their code is well-written. They Oh no. Nobody should or can safely run production with display_errors. Everybody thinks their code is well-written, but display_errors should never be enabled in production, however high is your opinion of the code. I'm afraid people now will start quoting this saying "ok, yeah, if you're a bad programmer, disable display_errors, but I'm a good programmer, my code is solid, I even have a dozen of unit tests, so I just go ahead and enable display_errors" and then we have this sad state of affairs where sites spill out error messages that are never supposed to be seen by clients because developers thought it can never happen. > The alternative is to just not have any error message at all. That > avoids the discrepancy between the error messages with display_errors on > and off. I don't think it's a good idea to add such magic, as correctly noted, it will make people that are working properly - display off in production, on in development - work harder and have trouble, all in the name of cuddling people that run misconfigured servers and ignore the advice that is being repeated for years by now. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57205 — johannes@schlueters.de — 2012-01-04T20:46:49+00:00 URL: https://externals.io/message/57174#57205 · In reply to: #57204 > Hi! > > > […] > > Oh no. Nobody should or can safely run production with display_errors. > Everybody thinks their code is well-written, but display_errors should > never be enabled in production, however high is your opinion of the code. > I'm afraid people now will start quoting this saying "ok, yeah, if > you're a bad programmer, disable display_errors, but I'm a good > programmer, my code is solid, I even have a dozen of unit tests, so I > just go ahead and enable display_errors" and then we have this sad state > of affairs where sites spill out error messages that are never supposed > to be seen by clients because developers thought it can never happen. On shared hosts display_errors typically is on, but the application can do ini_set('display_errors', 0) or such ... johannes --- ## #57206 — Jannik Zschiesche — 2012-01-04T20:49:21+00:00 URL: https://externals.io/message/57174#57206 · In reply to: #57205 Hi Johannes, as far as I understood the issue, this error would be triggered before the application's code is executed, so that would not solve this issue. Cheers Jannik Am 04.01.2012 um 21:46 schrieb Johannes Schlüter: > > > […] > > On shared hosts display_errors typically is on, but the application can > do ini_set('display_errors', 0) or such ... > > johannes --- ## #57207 — johannes@schlueters.de — 2012-01-04T20:52:28+00:00 URL: https://externals.io/message/57174#57207 · In reply to: #57206 > Hi Johannes, > > as far as I understood the issue, this error would be triggered before > the application's code is executed, so that would not solve this > issue. > That's the point. Thanks for making it clear. :-) johannes > > Cheers > Jannik --- Next page: https://externals.io/message/57174.md?page=2