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.
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.
Thomas Rutter
