Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [PATCH] Fix for bug #31440 (GLOBALS can be by G/P/C when register_globals=On) »

[PATCH] Fix for bug #31440 (GLOBALS can be by G/P/C when register_globals=On)

php.internals

Jani Taskinen

21 years ago
Patch to fix is here: http://www.php.net/~jani/patches/bug31440.php_4_3_patch http://www.php.net/~jani/patches/bug31440.php_HEAD_patch In PHP_4_3 you can overwrite GLOBALS with these queries: ?GLOBALS[foo]=err or ?GLOBALS[]=foo or ?GLOBALS=foo In HEAD you can overwrite GLOBALS with this only: ?GLOBALS=foo I didn't investigate WHY that is the only type of query that "works" in HEAD branch but the same patch fixed that too. None of super-globals can be overwritten like this, be it register_globals On or Off. IMNSHO, GLOBALS should be "protected". (I don't say that this hacky patch of mine is the way, but it does the job :) --Jani

Andi Gutmans

21 years ago
Jani, Thanks for bringing this up. Will look into this. Andi At 01:39 PM 2/15/2005 +0200, Jani Taskinen wrote:

Andi Gutmans

21 years ago
This behavior makes some sort of sense. It happens when register_globals is on which means you are supposed to be able to access $GLOBALS[] and it makes sense for it to stay in sync with the global variables. Maybe $GLOBALS[] and $GLOBALS direct access are edge cases but should we invest time and code to resolve this when we know it's a general problem anyway? Andi At 01:39 PM 2/15/2005 +0200, Jani Taskinen wrote:

Jani Taskinen

21 years ago
On Tue, 15 Feb 2005, Andi Gutmans wrote:
> This behavior makes some sort of sense. It happens when register_globals is on > which means you are supposed to be able to access $GLOBALS[] and it makes > sense for it to stay in sync with the global variables.
Yes, and register_globals=on is evil. That I do agree with. :) And that was the initial response I had in mind for this bug report too.
> Maybe $GLOBALS[] and $GLOBALS direct access are edge cases but should we > invest time and code to resolve this when we know it's a general problem > anyway?
Well, the patch is there and works with all cases I tried with and could think of..but I'm all for removing register_globals altogether though. :) Seriously, this is not consistent (you can't replace e.g. _GET like this) so why not protect against it? I cooked up a piece of PHP code to recreate $GLOBALS always when register_globals=On but come on..it's slow and won't give you exactly same result.. <?php /* * Protect GLOBALS by recreating it whenever register_globals = On */ if (ini_get('register_globals')) { $superglobals = array ( '_SERVER' => $_SERVER, '_ENV' => $_ENV, '_FILES' => $_FILES, '_COOKIE' => $_COOKIE, '_POST' => $_POST, '_GET' => $_GET ); if (isset($_SESSION)) { array_unshift($superglobals, array ('_SESSION' => $_SESSION)); } $GLOBALS = array(); foreach ($superglobals as $sg_name => $sg_value) { $GLOBALS[$sg_name] = $sg_value; foreach ($sg_value as $name => $value) { $GLOBALS[$name] = $value; } } } ?> It lacks the reference to itself of course but who needs that anyway? :) So, do I post that as resolution for the bug or what? --Jani

Ilia A.

21 years ago
IMO this is something that should be marked as Won't Fix and hope that by PHP 5.2 we can drop register_globals all together. Heck, perhaps we can do it for PHP 5.1. Ilia Andi Gutmans wrote:

Rasmus Lerdorf

21 years ago
Seems like a bad excuse. Being able to destroy a superglobal array like that is clearly a bug that should be fixed. -Rasmus Ilia Alshanetsky wrote:

Andi Gutmans

21 years ago
If we take the fixing approach then Jani's fix would work or at least it seems it would. I guess we could apply it but we'll be fixing just one problem for someone who's already using a conceptually broken feature. Probably the best way would be to define $GLOBALS last but I'm afraid that would probably require too many changes. Jani, think we should commit it? Andi At 07:15 PM 2/15/2005 -0800, Rasmus Lerdorf wrote:

Ilia A.

21 years ago
Here is a slightly shorter solution to the problem, which hopefully will prove to be a little bit faster as well. Ilia

Stanislav Malyshev

21 years ago
AG>>>Seems like a bad excuse. Being able to destroy a superglobal array like AG>>>that is clearly a bug that should be fixed. IMO there should be no possibility to destroy GLOBALS with ?GLOBALS=foo, but there should be possibility to write GLOBALS with ?GLOBALS[foo]=bar - why not, if we allow accessing globals anyway? I didn't check how hard it should be to separate these - if it's hard then restricting access to GLOBALS would be good.
-- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ +972-3-6139665 ext.115

Rasmus Lerdorf

21 years ago
Stanislav Malyshev wrote:
> AG>>>Seems like a bad excuse. Being able to destroy a superglobal array like > AG>>>that is clearly a bug that should be fixed. > > IMO there should be no possibility to destroy GLOBALS with ?GLOBALS=foo, > but there should be possibility to write GLOBALS with ?GLOBALS[foo]=bar - > why not, if we allow accessing globals anyway? I didn't check how hard > it should be to separate these - if it's hard then restricting access to > GLOBALS would be good.
Yup, I agree. GLOBALS[foo]=bar is perfectly fine since that doesn't destroy the array and prevent a $foo global locally defined from overriding it. The problem with the current situation is code like this: <?php $password_checked = true; ... if($GLOBALS['password_checked']) do_something; ?> This application can be hacked by simply sending it a ?GLOBALS=foo in the URL. -Rasmus

Jani Taskinen

21 years ago
On Wed, 16 Feb 2005, Stanislav Malyshev wrote:
> AG>>>Seems like a bad excuse. Being able to destroy a superglobal array like > AG>>>that is clearly a bug that should be fixed. > > IMO there should be no possibility to destroy GLOBALS with ?GLOBALS=foo,
Agreed 100%. Ilia's improved patch fixes this and the one below.
> but there should be possibility to write GLOBALS with ?GLOBALS[foo]=bar -
True. But that shouldn't overwrite the whole thing and ending up with GLOBALS containing only 'foo' => 'bar' entry? (that happens now..) --Jani

Steph

21 years ago
Can anyone give me a good reason we still have register_globals in PHP 5 at all? I mean, it's been marked 'deprecated' forever.... Just my 2 sheks, - Steph ----- Original Message ----- From: "Ilia Alshanetsky" <ilia@prohost.org> To: "Andi Gutmans" <andi@zend.com> Cc: "Jani Taskinen" <sniper@iki.fi>; <internals@lists.php.net> Sent: Wednesday, February 16, 2005 5:01 AM Subject: Re: [PHP-DEV] [PATCH] Fix for bug #31440 (GLOBALS can be by G/P/Cwhen register_globals=On)

Andi Gutmans

21 years ago
Yes because a lot of applications/code still use it. It's off by default but we should still enable people to move to PHP 5 whilst allowing old code to run. At 07:37 AM 2/16/2005 +0200, Steph wrote:

Steph

21 years ago
- I'd agree to that more if we didn't have barriers there already. But we do. Did you see the sitepoint blog today? - Steph ----- Original Message ----- From: "Andi Gutmans" <andi@zend.com> To: "Steph" <steph@zend.com>; "Ilia Alshanetsky" <ilia@prohost.org> Cc: "Jani Taskinen" <sniper@iki.fi>; <internals@lists.php.net> Sent: Wednesday, February 16, 2005 7:48 AM Subject: Re: [PHP-DEV] [PATCH] Fix for bug #31440 (GLOBALS can beby G/P/Cwhen register_globals=On)
> Yes because a lot of applications/code still use it. It's off by default > but we should still enable people to move to PHP 5 whilst allowing old
code
> to run. > > At 07:37 AM 2/16/2005 +0200, Steph wrote: > >Can anyone give me a good reason we still have register_globals in PHP 5
at
> >all? > > > >I mean, it's been marked 'deprecated' forever.... > > > >Just my 2 sheks, > > > >- Steph > > > >----- Original Message ----- > >From: "Ilia Alshanetsky" <ilia@prohost.org> > >To: "Andi Gutmans" <andi@zend.com> > >Cc: "Jani Taskinen" <sniper@iki.fi>; <internals@lists.php.net> > >Sent: Wednesday, February 16, 2005 5:01 AM > >Subject: Re: [PHP-DEV] [PATCH] Fix for bug #31440 (GLOBALS can be by > >G/P/Cwhen register_globals=On) > > > > > > > IMO this is something that should be marked as Won't Fix and hope that > > > by PHP 5.2 we can drop register_globals all together. Heck, perhaps we > > > can do it for PHP 5.1. > > > > > > Ilia > > > > > > Andi Gutmans wrote: > > > > This behavior makes some sort of sense. It happens when
register_globals
> > > > is on which means you are supposed to be able to access $GLOBALS[]
and
> > > > it makes sense for it to stay in sync with the global variables. > > > > Maybe $GLOBALS[] and $GLOBALS direct access are edge cases but
should we
> > > > invest time and code to resolve this when we know it's a general
problem
> > > > anyway? > > > > > > > > Andi > > > > > > > > At 01:39 PM 2/15/2005 +0200, Jani Taskinen wrote: > > > > > > > >> Patch to fix is here: > > > >> > > > >> http://www.php.net/~jani/patches/bug31440.php_4_3_patch > > > >> http://www.php.net/~jani/patches/bug31440.php_HEAD_patch > > > >> > > > >> In PHP_4_3 you can overwrite GLOBALS with these queries: > > > >> > > > >> ?GLOBALS[foo]=err or ?GLOBALS[]=foo or ?GLOBALS=foo > > > >> > > > >> In HEAD you can overwrite GLOBALS with this only: > > > >> > > > >> ?GLOBALS=foo > > > >> > > > >> I didn't investigate WHY that is the only type of query that > > > >> "works" in HEAD branch but the same patch fixed that too. > > > >> > > > >> None of super-globals can be overwritten like this, be it > > > >> register_globals On or Off. > > > >> > > > >> IMNSHO, GLOBALS should be "protected". > > > >> (I don't say that this hacky patch of mine is the way, but it
does

Derick Rethans

21 years ago
On Tue, 15 Feb 2005, Andi Gutmans wrote:
> Yes because a lot of applications/code still use it. It's off by default > but we should still enable people to move to PHP 5 whilst allowing old code > to run.
My experience is that most (OO) code doesn't work when moving anyway... so this is a bad excuse andi ;-) Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Christian Schneider

21 years ago
Derick Rethans wrote:
> My experience is that most (OO) code doesn't work when moving anyway... > so this is a bad excuse andi ;-)
If you turn zend.ze1_compatibility_mode on (and a lot of people have to do that) then the situation is a bit better. Which brings me to the point that all those E_STRICT messages in compatibility mode slow down execution of such old applications while not really helping, they give a warning on almost each and every assignement and object creation. That's one of the things stopping me from even trying to move one of the bigger applications here to PHP 5. Removing register_globals will not be possible for years. You might not like it but you'll have to accept it. Otherwise you'll either mess up a lot of people's life having to fix very subtle bugs or you'll stop a lot more people from using the new version altogether. Programming is like sex, one mistake and you have to support it for life :-) One thing I don't quite get about the proposed patch to overwriting superglobals is why int globals_check = (PG(register_globals) && (dest == (&EG(symbol_table)))); checks for register_globals. Originally I was under the impression that the ?GLOBALS=foo problem exists with register_globals off too. My naive understanding would be that with register_globals off the user should be _even less_ able to overwrite something in the globals variable space so the application can be sure that every global variable was set by itself. Or did I misinterpret the patch and/or the problem? Cheers, - Chris

Zeev Suraski

21 years ago
At 08:10 16/02/2005, Steph wrote:
>- I'd agree to that more if we didn't have barriers there already. But we >do. > >Did you see the sitepoint blog today?
Barriers are not binary, they accumulate. The more barriers you introduce, the less people are likely to migrate. Discontinuing register_globals is equivalent to introducing a fortified wall barrier for large code bases. Zeev

Andi Gutmans

21 years ago
Who's talking about OO code? Lots of code is functional and requires register_globals. BTW, I've talked to ppl who have ported OO apps relatively painlessly too. At 09:11 16/02/2005 +0100, Derick Rethans wrote:

Zeev Suraski

21 years ago
At 05:15 16/02/2005, Rasmus Lerdorf wrote:
>Seems like a bad excuse. Being able to destroy a superglobal array like >that is clearly a bug that should be fixed.
$GLOBALS is not a superglobal array. Zeev

Andi Gutmans

21 years ago
I agree with the first part, but I don't think we can drop register_globals completely. We will have legacy apps for years to come and should support them. At 10:01 PM 2/15/2005 -0500, Ilia Alshanetsky wrote: