Security Risk? $_REQUEST variables ... $$ on the local stack

Question

I was talking with one of my programmers earlier and he showed me a piece of code he was considering:

foreach($_REQUEST as $var=>$val) {
    $$var = addslashes($val);
}

He wanted to be able to use $varName instead of having to write $_REQUEST['varName']

I advised him to use the mysql_real_escape_string instead of addSlashes and to not put the $_REQUEST variables onto the local stack because that gives hackers an attach vector. To me that seems like the same problem that the old REGISTER_GLOBALS directive had.

He said there was not the same security risks because those variables were all being created on the local stack. So I was uncertain and I checked out the PHP variable variables page at: http://www.php.net/manual/en/language.variables.variable.php but saw no reference to Super Globals and security other then the warning box.

Can hackers easily take advantage of that construct?

Answer 1

I haven't coded php in years, but isn't there a function that does this? Perhaps called extract?

There are a few warnings about using that function with user inputted data.. those warnings would apply to your code snippet as well.

Answer 2

Yes. If the request URL contains ...&GLOBALS[var]=1 then it will overwrite the according global variable.

He should at least consider following construct for safety:

 extract($_REQUEST, EXTR_PREFIX_ALL, "var_");  // or EXTR_SKIP

This will give the localized variables at least a prefix and prevent overwriting of globals. For good measure also use array_map("addslashes", $_REQUEST) or a more appropriate escaping function. But really, this is just magic_quotes by another name. (Offtopic: try to let go off mysql_query and use PDO and prepared statements, which is simpler and more secure.)

Answer 3

The "local stack" has nothing to do with it. It still lets anyone create a new variable in some part of the code that the programmer may not have anctipated.

If he really wants to take the request parameters out of $_REQUEST and make variables of them all, then he should transfer only the ones the code is going to look for and no more.

And ditch addslashes(). That is not only the wrong place to escape incoming data, but the wrong function, too.

Answer 4

This is like turning back 6 years of PHP security enhancements... Basically, register_globals and magic_quotes put together! Those two are marked deprecated in recent versions of PHP, and will be removed from future versions, for very good reasons.

Imagine the following code:

if ($is_admin) {
    do_administrative_task();
}

Now somebody makes the following request:

http://www.example.com/script.php?is_admin=1

And just like that, you're admin!

Likewise, addslashes() doesn't really provide any protections against SQL injection attacks, because it doesn't understand modern character sets. It's ridiculously easy to craft something that will bypass addslashes() and pwn your database.