Can't understand syntax error, unexpected '=' in eval()'d code

Question

I've search different forms but didn't understand how to solve the problem first found here (first link, second link) but its giving me the error in eval. I can't figure it out how to solve in that code in foreach loop.

foreach($_POST as $key => $value) {
        if(!strstr($key, 'removeFile')){
                //initialize variables using eval
                eval("$" . $key . " = '" . sanitize($value) . "';");
        }
}

Answer 1

As the first answer to the post you linked to, the problem is that when using double quotes PHP thinks your eval() code starts with a variable. As that is not the case you have two options. Use single quotes and remember to escape the single quotes declaring a string in the code or escape the dollar sign.

Bonus note

There exist more elegant solutions to the problem you are trying to solve. The best solution I can think of is using the extract function. This gives to two main benefits.

1. It works with all associative arrays
2. You can specify different flags that can help you distinguish the extracted variables apart and avoid variable injection.

One flag you can use is EXTR_PREFIX_ALL. This will prefix all the extracted variables with your own prefix. You would then access the variables with a prefix of 'PREFIX_' like the following:

$array = [
    'variable1' => 'foo', 
    'variable2' => 'bar' 
];

extract($array, EXTR_PREFIX_ALL, 'PREFIX_');

$value1 = $PREFIX_variable1; // Equals: foo
$value2 = $PREFIX_variable2; // Equals: bar

A little on code injection

Suppose you have some code:

$login = false;

The $login variable determines if a user is logged in or not. Then somewhere you use the ´extract´ function with an array of the following without using any flags.

$array = [
    'login' => true,
    'foo'   => 'bar'
];

extract($array);

Now your $login variable would be set to true and the user posting the data would have overwritten the initial setting and gained access to your website without a valid login. Bear in mind this is a over simplified example, but nonetheless valid.

To overcome this you can use the flag EXTR_SKIP or prefix them like I previously showed. The EXTR_SKIP flag will skip the array element if a variable with the same name already is defined. So now your code would not overwrite your $login variable.

extract($array, EXTR_SKIP); // Skip existing variables
// Or
extract($array, EXTR_PREFIX_ALL, 'prefix'); // Prefix them all.

Hope this can guide to the right choice for your needs.

Regards.

Answer 2

First, the issues I have with your code:

1. eval is very, very rarely needed, and extremely dangerous, use with caution. I've been developing in PHP for over 10 years, and never really encountered a situation that needed eval. This is no exception. Eval is not required
2. You're sanitizing the entire $_POST array. That's great, but there are special functions for doing that, like: filter_input_array, array_filter and many, many more... not to mention ready-made, open source projects and frameworks that already contain a solid request validation component.
3. Always check the return values of functions, which you seem to be doing with strstr, but be weary of functions that return different types (like strstr: it returns false if the needle isn't found, but returns 0 if the needle is found at the start of the haystack string). Your if statement might not work as intended.
4. You're assuming sanitize($value) values will not contain any single quotes. Why? Because if they do, you'll end up with a syntax error in your evaled string

Be that as it may, you could easily write your code using variable variables and add a simple check not to step on existing variables in scope:

$sanitized = array_filter($_POST, 'sanitize');//call sanitize on all values
foreach ($sanitized as $key => $value)
{
    if (!isset($$key) && strstr($key, 'removeFile') === false)
        $$key = $value;
}

But really, $_POST values belong together, they are part of the request, and should remain grouped... either in an array, or in an object of some sort. Don't assign each value to its own variable, because pretty soon you'll loose track of what variables are set and which are not. Using an unset variable creates that variable, assigning value null, so what you have now makes for very error-prone code:

//request 1: POST => id=123&foo=bar
foreach ($sanitized as $k => $v)
    $$k = $v;
$query = 'SELECT x, y, z FROM tbl WHERE id = ?';//using posted ID as value
$stmt = $db->prepare($query);
$stmt->execute(array($id));

All is well, because $id was set, but never trust the network, don't assume that, just because $_POST is set, all the keys will be set, and their values will be correct:

//request 2: POST => foo=bar&page=2
foreach ($sanitized as $k => $v)
    $$k = $v;
$query = 'SELECT x, y, z FROM tbl WHERE id = ?';//using posted ID as value
$stmt = $db->prepare($query);
$stmt->execute(array($id));//id is null

Now we have a problem. This is just one example of how your code might cause issues. Imagine the script grows a bit, and look at this:

//request 3: POST => id=123&foo=bar&page=2
foreach ($sanitized as $k => $v)
    $$k = $v;
//$id is 123, $foo is bar and $page = 2
$query = 'SELECT x, y, z FROM tbl WHERE id = ? LIMIT 10';//using posted ID as value
//a lot more code containing this statement:
$page = someFunc();
$log->write('someFunc returned log: '.$page);
//more code
$offset = 10*($page-1);//<-- page is not what we expected it to be
$query .= sprintf(' OFFSET %d', $offset);
$stmt = $db->prepare($query);
$stmt->execute(array($id));

Now this may seem far-fetched, and idiotic, but believe me: all of these things happen, more than I care to know. Adding some code that accidentally overwrites an existing variable that is used further down happens all the time. Especially in procedural code. Don't just blindly unpack an array. Keep that single variable, and use the keys to avoid:

• grey hair
• sudden, dramatic baldness
• loss of sanity
• bleeding ulcers
• In a work environment: catastrophic loss of data
• Sudden loss of job
• ... because code like this makes unicorns cry, and bronies will hunt you down