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

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 abl开发者_JAVA技巧e 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?

---

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.

---

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.

---

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.

---

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.)

---

Right so! As the programmer in question there are actually three separate issues here.

1. That $_REQUEST is no more insecure than $_GET or $_POST given that by default PHP $_COOKIES takes precedence over $_GET and $_POST.
2. Setting a superglobal's value is more secure than a local variable in a function or an object method local variable.
3. End user created variable in a function are globally accessible.

Let's start at the top.

1. $_POST and $_GET are not more secure than $_REQUEST. All information received from a form is dirty. Period. Regardless. If I can send in faked cookies I have the know how to fake POST variables. The argument is moot.
2. So we take a local variable we create with $var. The scope of this variable is only within the function/method. Is setting the contents of a superglobal to the value of user supplied data MORE secure than a local variable that gets scrubbed when the function/method stack gets cleaned? I wonder...
3. Lets take a snippet

 

function some_function() {
   foreach($_POST as $var=>$val) {
        $$var = $val; 
  }
}

Now, if $_POST contains a variable called _POST this code will set a local variable $_POST containing the value. However, superglobals will override this and any reference to $_POST will reference the superglobal, not the local variable and as such the local $_POST is unrefereceable. Given this functionality of PHP, without using the global keyword it is normally impossible to completely override the superglobal