The Problem With ‘extract’

In news over the weekend, Stefan Esser over on the PHP Security Blog wrote up a strong criticism of the new article 10 Tips That Every PHP Developer Should Know, Part 2 (part of a two part series).

Apart from the fact the article’s author Jeffery Vaska can’t seem to count to ten (thanks Jules for spotting that), the article contains some dubious advice as far as Stefan is concerned, and he exception to one tip in particular, tip 5 (the second tip 5; we’re genorously given two tips labelled ‘tip 5′) which explains the use of the extract language construct to extract the contents of the $_POST variable to local variables.

From Stefan:

using extract() without using prefixes or the parameter EXTR_SKIP is usually a very big security hole, because it allows an external attacker to overwrite every variable, including the superglobals (unless you use the Hardening-Patch) and this can lead in many cases to SQL injection or even Remote Code Execution Vulnerabilities

It is relatively common for PHP applications to use variables uninitialized (one reason why debugging with notices on is a good idea). If you refer to a variable $username without initializing it beforehand, you may be wrongfully assuming that its initial value will be null. extract, like register_globals can falsify that assumption, allowing the variable $username to be initialised by end users. In short, it’s just like turning register_globals on again. extract can be used securely and therefore is not a security problem in itself, but can be lethal to your security in combination with any number of other sloppy coding practices.

In my eyes, the biggest problem with extract and also the biggest reason it is a security risk is actually that extract makes code so hard to read and debug. Try maintaining some code where somebody has used extract and you’re likely to find yourself yelling ‘Where does this variable come from?!’ Variables appear to materialize out of thin air. Consider the following.


// see if the user is authorised

$details = $this->getuserdetails();

extract($details);

if ($access[$accesszone]) $this->authorise();

Where did $access and $accesszone come from? Voodoo!

In my opinion it is confusing code practices such as this which lead to real problems in the overall security of code. The harder code is to read, the harder it is to debug or review and the easier it is to inadvertantly introduce security holes.

Free book: Jump Start HTML5 Basics

Grab a free copy of one our latest ebooks! Packed with hints and tips on HTML5's most powerful new features.

  • Etnu

    I’ve been hard pressed to find any real reason why you’d want to use extract, as it really doesn’t gain you anything. Sure, it might be less to type out ($Variable vs. $Array['Variable']), but is it really worth all the confusion?

    There is only one practical use that I can think of, and that’s if you pass arguments into a function in the form of an array (because you’ve got a ton of parameters). Something like:


    function DoStuff($Args)
    {
    $FirstArg = 0;
    $SecondArg = '';
    $ThirdArg = null;
    ...
    $ArgumentNumberTwenty = 'Wow!';
    extract($Args,EXTR_IF_EXISTS)
    ...
    }

    This would save the hassle of having to use isset() on each array element. Of course, you could also achieve this same thing using array_merge with the overwrite flag set, but that’s just a matter of style preference.

  • Dr Livingston

    Much like eval() I have never used extract() either. Wouldn’t touch them with a barge pole, but I thank you for being it to the attention of other developers who may be using extract() without knowingly introduce a security hole in their scripts.

    My thoughts are just to ignore these two functions, as there are other alternatives and work arounds you can use, just as cleanly :)

  • Mike Willbanks

    Actually there is a very good use for extract that many people do not see…
    If you built a template system there is an extremely good use.

    My templates simply work by running a page and extracting the variables to it…
    Such as this:
    $tpl->SetVariable(‘something’, $somevar);
    now what happens is during the template loading (they are simply php templates.. you can run php but it is in a sandbox so to speak) and the variables that have been set are extracted to it.

    But indeed you have to set your own variables to make it work.

  • mwmitchell

    Hmm, OK one good use: If you have a PHP based template class. In the method that actually includes the template file… If you use extract (extract($this->template_data)), then you expose only the variables needed. If you use a loop, then you expose the $key/$val variables to the templates variable scope. What happens if someone has assigned a variable named ‘key’ or ‘val’? It gets squashed by your foreach loop. Not with extract().

    Matt

  • http://www.technosailor.com Sketch

    Not to mention that extract() is implicitly identical to setting register_globals to enabled.

  • http://www.deanclatworthy.com Dean C

    Etnu – that’s not a legitimate use. If you’re using a variable number of arguements then PHP has func_get_vars

  • Travis S

    Hi Thomas – interesting article. You had my heart skip a beat because I use extract in a custom template engine to bring all the assigned variables into the template’s scope.

    My tests indicate that you can’t overwrite globals in PHP 5 via extract. You can see the unit test at here. The last two methods are the latest tests to insure the integrity of GLOBALS and _SERVER.

    One thing to watch out for with extract() however, is the scope in which it’s executed. If it’s executed within a method that is expecting it to extract (such as my d51Template::display()), then I see little issue with it, however, if you are executing it in scope of a larger function or in the global scope, it could get very hard to work with. In my case, display() issues $this->_findFile(), extracts the assigned variables, and includes the template – it’s perfectly obvious what it’s doing.

    I think extract() is just like every other function/method in PHP. There are a lot of uses for it that don’t properly take into account good design, but when it’s the right tool for the job, it does that job well.

  • Matthean

    This tip has been removed from the article.

  • Etnu

    Etnu – that’s not a legitimate use. If you’re using a variable number of arguements then PHP has func_get_vars

    PHP doesn’t support named parameters (ala Perl), so it could be a semi-legitimate work around. func_get_args() doesn’t support this.

    Personally, I’d prefer just passing an array in to the messy extract().

    My personal new favorite pick for extract?

    Placating people who complain when you turn off register_globals (and, yes, there are some out there!)

  • Colin F

    Yet another commet about eval’d code
    I -rarely- use this, although I have found it quite handy when creating my library for rendering sql statments into tables, forms etc when there are little quirks I want to pass through a array that applies various formating methods, and I see no point in adding a completly new function for. It’s safe as there’s no way, globals on or off to squeeze anything through this function.

    It’s becomes akin to using something like Crystal Reports :)

    RE – Extract? Useless, damn messy. I worked on a project that a newbie built that was filled with extracts, $$var’s and with globals on on top of it, it was a messy, buggy, hell. If someone can find a really good use for this, I’ll be surprised – If there’s some reason you need to ‘extract’ data like that, stick with serialize and unserialize, a handy way to stick array’s into text/db/whatever (still kind of messy though)

  • Ramon Sosa

    I personally use extract in:
    1){
    while($row=mysql_fetch_assoc($result)){
    extract($row);
    include ‘letter_to_clients_tpl.php’;
    }

    }
    ?>

    ///letter_to_clients_tpl.php
    //Begin Template

    Dear customer:

    Some notificacions.

    Sincerely..

    //End template

  • Dorsey

    I use extract() under controlled conditions such as the previous one, where I’m putting the values from a result set into local variables for convenience. I fail to see the confusion here: the SELECT statement clearly lists the variable names; my old buddy extract() saves me the effort of decorating the variable names with $row['column_name'] everywhere. I do NOT use this on any of the globals, as I said, but only under controlled circumstances, and enjoy the convenience it offers.

  • Vaska

    I was THE particular author and I made a mistake. Writing that article got a little messy as originally it wanted to be something else. As you will notice, we pulled tip #5 from part two as it never should have been there…there was already a tip #5 in part one. Originally there were 15 tips and my plan was to use the best 10.

    As the previous poster surmises, I too believe that extract() is best used ‘under controlled cirucumstances’. The example that I gave, with globals, was simply not a good one.

    One of the things that I really valued a few years ago (well, I still do) are the discussions surrounding proper and improper use of various concepts. It’s darn informative – if you can follow along. Originally, the article in question had a working title of ‘Problems of Style’ that wanted to point out these things (the 10 tips) so that newbs could better recognize the variations because everybody does things slightly differently. However, some methods are certainly better than others. ;)

  • Pingback: SitePoint Blogs » Blog Archive » How Readable is Your PHP?

  • Pingback: Design By Tim » Blog Archive » Rethinking extract() By Convention