CODEWITHSUNDEEP
phpsecuritypost
Abraham Brookes
Abraham Brookes

Reputation: 1997

php $POST dynamic variable names security concerns

I am thinking of making a loop to gather all my $_POST variables and assign them to dynamically named variables.Something like this (not tested)

 for($i; $i <= $_POST[].length; $i++){
  ${$_POST[i]} = $_POST[i]
  }

But I am wondering about the security of something like this. This would then create a variable in the system for every bit of post data sent to the page. Can that variable be damaging even if the script I write doesn't reference it? Is this the type of thing I should avoid entirely? I have some pages that send quite a few variables and a script like this would prevent a whole lot of writing, but is it safe enough?

Upvotes: 2

Views: 1818

Answers (7)

Lawrence Cherone
Lawrence Cherone

Reputation: 46620

Yes there can be security concerns/problems, for example one could overwrite any local variables which are already set, like database, config values ect.

So something like this should be avoided:

$yourImportantVar = 'Something relies on this';

//User POSTS yourImportantVar=overwritten
foreach ($_POST as $key => $value) {
    $$key = $value; 
}
echo $yourImportantVar; //overwritten

But if you want to implement a loop to save a chunk of code, you could create an allowed array which you loop over and extract out the value from the $_POST.

foreach (array(
    'name',
    'address',
    'somethingelse',
    'ect'
) as $key) {
    $$key = isset($_POST[$key]) ? $_POST[$key] : null;
}

Upvotes: 2

Gumbo
Gumbo

Reputation: 655439

You are basically implementing extract($_POST, EXTR_OVERWRITE) which will overwrite any already existing variables. The manual already warns to use extract in a way you do:

Do not use extract() on untrusted data, like user input (i.e. $_GET, $_FILES, etc.). If you do, for example if you want to run old code that relies on register_globals temporarily, make sure you use one of the non-overwriting extract_type values such as EXTR_SKIP and be aware that you should extract in the same order that's defined in variables_order within the php.ini.

This can result in overwriting essential and sensitive variables, including those that cannot really be modified directly like $_SESSION, $_SERVER, and $GLOBALS:

POST /foo.php HTTP/1.1
Content-Type: application/x-www-urlencoded

_SESSION[user]=admin

This would have the same effect as $_SESSION = array('user'=>'admin').

Upvotes: 0

Bailey Parker
Bailey Parker

Reputation: 15905

PHP had a feature (using the term loosely) called register_globals. It has since been deprecated (PHP 5.3) and removed (PHP 5.4), but it mirrored the functionality for which you are looking. It performed the same thing as the PHP function extract() does, which sets variables in the current scope with names of the keys and values of the matching array values. This is most definitely a security risk. Consider the example of a poor check for authentication:

if($is_logged_in) {
    // Allow execution of destructive actions
}

If this feature was enabled (or you mimicked it), a malicious user would be able to set the variable $is_logged_in and bypass the login screen. Don't worry about saving typing. If you need to copy and paste a code block like this at the beginning of your files:

$something = $_POST['something'];
$another   = $_POST['another'];
$stuff     = $_POST['stuff'];
//etc.

Not only is it much more secure, but it doesn't leave developers (who aren't expecting register_globals) puzzled when undeclared variables start being used. Also, the fact that PHP has removed it and there are plenty of arguments against its use should be evidence enough.

Upvotes: 1

UltraInstinct
UltraInstinct

Reputation: 44444

Only issue I can think of at the moment is when that overwrites the existing variable in the scope. This can be very unsafe depending on what you do with it. Think about the variable being the URL you are doing a HTTP request to. Or worse, some flag variable which accesses some critical part of your code.

I will post an example that speaks about HTTP request:

<?php
    $url = "http://safe/url/to/POSTto";
    $var = array("url" => "http://www.mysite.com/url"); //assume this is $_POST
    foreach($var as $key => $value){
        ${$key} = $value;
    }

    //now upon the HttpRequest, your site can receive the (critical) data which was actually meant for the safe site.
?>

EDIT: @Galen has posted about the flag variable I was talking about, so may be I need not post any example to highlight the problem.

Upvotes: 1

ifunk
ifunk

Reputation: 637

This is a very bad idea for security and maintainability. Simplified example why...

<?php

if (someRandomSessionCheck()) {
    $isAdminUser = true;
}


if ($isAdminUser) {
    // give access to everything
}

?>

Someone could post to the page with a variable "isAdminUser=1" and would have access to everything.

Another reason it is a bad idea is you can't clearly see from the script where your variables are created. This reduces maintainability of the script. What if you now want to run the script but instead need to get the data from somewhere else and not a POST?

Upvotes: 2

Galen
Galen

Reputation: 30170

Yes there are possible security risks.

Say you have a variable $is_admin defined earlier in the code that gives someone admin abilities. If someone POSTS to that page with

$_POST['is_admin'] = true;

Then $is_admin is now true. Not good.

What's wrong with using $_POST?

Upvotes: 4

user1487634
user1487634

Reputation: 

 <?php

/* Suppose that $var_array is an array returned from
  wddx_deserialize */

 $size = "large";
 $var_array = array("color" => "blue",
               "size"  => "medium",
               "shape" => "sphere");
 extract($var_array, EXTR_PREFIX_SAME, "wddx");

 echo "$color, $size, $shape, $wddx_size\n";

  ?>

Please check this. Same thing what you are going do by using loop. may help you

Upvotes: 0

Related Questions