CODEWITHSUNDEEP
phparraysextract
barfoon
barfoon

Reputation: 28157

Why is calling extract() on submission data (ex. $_GET, $_POST) risky / bad practice?

I was recently reading this thread, on some of the worst PHP practices. In the second answer there is a mini discussion on the use of extract(), and im just wondering what all the huff is about.

I personally use it to chop up a given array such as $_GET or $_POST where I then sanitize the variables later, as they have been conveniently named for me.

Is this bad practice? What is the risk here? What are your thoughts on the use of extract()?

Upvotes: 47

Views: 31285

Answers (17)

Tell
Tell

Reputation: 316

To just expound a little on previous answers... There's nothing wrong with extract() so long as you filter the input correctly (as other's have stated); otherwise you can end up with huge security issues like this:

<?php

// http://foobar.doo?isLoggedIn=1

$isLoggedIn = (new AdminLogin())->isLoggedIn(); // Let's assume this returns FALSE

extract($_GET);

if ($isLoggedIn) {
    echo "Okay, Houston, we've had a problem here.";
} else {
    echo "This is Houston. Say again, please.";
}

Upvotes: 0

TheCrazyProfessor
TheCrazyProfessor

Reputation: 949

Be aware that extract() is not safe if you are working with user data (like results of requests), so it is better to use this function with the flags EXTR_IF_EXISTS and EXTR_PREFIX_ALL.

If you use it right, it safe to use

Upvotes: 0

dr Hannibal Lecter
dr Hannibal Lecter

Reputation: 6721

Come on now. People blame the tool instead of the user.

That's like talking against unlink() because you can delete files with it. extract() is a function like any other, use it wisely and responsibly. But don't claim it's bad per se, that's just ignorant.

Upvotes: 43

Chinoto Vokro
Chinoto Vokro

Reputation: 978

Extract is safe as long as you use it in a safe manner. What you want to do is filter the array's keys to only the ones you intend to use and maybe check that all those keys exist if your scenario requires their existence.

#Extract only the specified keys.
$extract=array_intersect_key(
    get_data()
    ,$keys=array_flip(['key1','key2','key3','key4','key5'])
);

#Make sure all the keys exist.
if ($missing=array_keys(array_diff_key($keys,$extract))) {
    throw new Exception('Missing variables: '.implode(', ',$missing));
}

#Everything is good to go, you may proceed.
extract($extract);

or

#If you don't care to check that all keys exist, you could just do this.
extract(array_intersect_key(
    get_data()
    ,array_flip(['key1','key2','key3','key4','key5'])
));

Upvotes: 2

Dixita
Dixita

Reputation: 101

Every method usage can lead to some conditions where it can be a point of failure for application. I personally feel that extract() should not be used for user input(that is not predictable) and for data that is not sanitized.

Even CodeIgniter core code uses extract, so there must be no harm in using the method if data is sanitized and handled well.

I have used extract in CodeIgniter models with the EXTR_IF_EXISTS switch and limiting the number of variables, it works pretty well.

Upvotes: 1

cpres
cpres

Reputation: 1003

One additional good reason to no longer use extract() is that there is a momentum in PHP to use HHVM which is claiming to make PHP about 10x faster. Facebook (who made it) is using it, Wikipedia is on it, and WordPress is rumored to to be looking at it.

HHVM doesn't allow extract()

It's still kind of alpha, so it's not the largest concern

Upvotes: -2

user10306
user10306

Reputation: 1113

As someone noted in a different thread, here is a safer way to use extract, by only allowing it to extract variables you specify, instead of everything the array contains.

This serves a dual purpose of documenting what variables are coming out of it so tracking back a variable wont be so difficult.

Upvotes: 1

stephenminded
stephenminded

Reputation: 316

People get all up-in-arms about extract because it has the potential to be misused. Doing something like extract($_POST) is not a good idea in any case, even if you know what you are doing. However, it does have it's uses when you are doing things like exposing variables to a view template or something similar. Basically, only use it when you are very certain that you have a good reason for doing so, and understand how to use the extract type parameter if you get the idea of passing something crazy like $_POST to it.

Upvotes: 6

nickf
nickf

Reputation: 546035

I find that it is only bad practice in that it can lead to a number of variables which future maintainers (or yourself in a few weeks) have no idea where they're coming from. Consider this scenario:

extract($someArray); // could be $_POST or anything

/* snip a dozen or more lines */

echo $someVariable;

Where did $someVariable come from? How can anyone tell?

I don't see the problem in accessing the variables from within the array they started in, so you'd really need to present a good case for using extract() for me to think it's worth it. If you're really concerned about typing out some extra characters then just do this:

$a = $someLongNameOfTheVariableArrayIDidntWantToType;

$a['myVariable'];

I think the comments here on the security aspects of it are overblown somewhat. The function can take a second parameter that actually gives you fairly good control over the newly created variables, including not overwriting any existing variables (EXTR_SKIP), ONLY overwriting existing variables (so you can create a whitelist) (EXTR_IF_EXISTS), or adding prefixes to the variables (EXTR_PREFIX_ALL).

Upvotes: 72

SeanDowney
SeanDowney

Reputation: 17744

If not used carefully it can confuse the heck out of others you work with consider:

<?php

    $array = array('huh' => 'var_dump', 'whatThe' => 'It\'s tricky!', 'iDontGetIt' => 'This Extract Function');
    extract($array);
    $huh($whatThe, $iDontGetIt);


?>

Yields: 

string(12) "It's tricky!"
string(21) "This Extract Function"

Would be useful to use in an obfuscation. But I can't get over the "Where did that var come from?" problem that I run into.

Upvotes: 6

James Socol
James Socol

Reputation: 1795

Never extract($_GET) in a global scope. Other than that, it has its uses, like calling a function that could (potentially) have lots of optional arguments.

This should look vaguely familiar to WordPress developers:

function widget (Array $args = NULL)
{
    extract($args);

    if($before_widget) echo $before_widget;

    // do the widget stuff

    if($after_widget) echo $after_widget;
}

widget(array(
    'before_widget' => '<div class="widget">',
    'after_widget' => '</div>'
));

Upvotes: 2

OIS
OIS

Reputation: 10033

If you extract in a function, the variables will only be available in that scope. This is often used in views. Simple example:

//View.php
class View {
    function render($filename = null) {
        if ($filename !== null) {
            $this->filename = $filename;
        }
        unset($filename);
        extract($this->variables);
        ob_start();
        $this->returned = include($this->dir . $this->filename);
        return ob_get_clean();
    }
}

//test.php
$view = new View;
$view->filename = 'test.phtml';
$view->dir = './';
$view->variables = array('test' => 'tset');
echo $view->render('test.phtml');
var_dump($view->returned);

//test.phtml
<p><?php echo $test; ?></p>

With some alternative directories, checks to see if the file exists and defined variables and methods - you've pretty much replicated Zend_View.

You can also add $this->outVariables = get_defined_vars(); after the include to run code with specific variabels and get the result of these for use with old php code.

Upvotes: 4

Powerlord
Powerlord

Reputation: 88796

I'll let the PHP manual do the talking for me.

Background: extract($_REQUEST) is the same as setting register_globals = On in php.ini

Upvotes: 3

Jamol
Jamol

Reputation: 2291

There is nothing wrong with it. Otherwise it wouldnt be implemented. Many (MVC) frameworks use it when you pass (assign) variables to Views. You just need to use it carefully. Sanitize those arrays before passing it to extract() and make sure it does not override your variables. Dont forget that this function also accepts a few more arguments! Using the second and third arguments you can control the behavior if collision occurs. You can override, skip or add prefix. http://www.php.net/extract

Upvotes: 9

stefs
stefs

Reputation: 18549

the risk is: don't trust data from users, and extracting into the current symbol table means, your variables could be overwritten by something the user provides.

<?php
    $systemCall = 'ls -lh';
    $i = 0;

    extract($_GET);

    system($systemCall);

    do {
        print_r($data[$i];
        $i++;
    } while ($i != 3);

?>

(a nonsensical example)

but now a malicious user who guesses or knows the code calls: 

yourscript.php?i=10&systemCall=rm%20-rf

instead of

yourscript.php?data[]=a&data[]=b&data[]=c

now, $systemCall and $i are overwritten, resulting in your script deleting your data first and hanging then.

Upvotes: 18

karim79
karim79

Reputation: 342635

I guess the reason a lot of people don't recommend using it is that extracting $_GET and $_POST (even $_REQUEST) superglobals registers variables in the global namespace with the same name as each key within those arrays, which is basically emulating REGISTER_GLOBALS = 1.

Upvotes: 5

Tomalak
Tomalak

Reputation: 338208

The risk is the same as with register_globals. You enable the attacker to set variables in your script, simply by tampering with the request.

Upvotes: 1

Related Questions