MOPS Submission 06: Variable Initialization in PHP

May 17th, 2010

Today we want to present you the sixth external MOPS submission. It is the second article sent in by Jakub Vrana. This one is about variable initialization in PHP.

Variable initialization in PHP

Jakub Vrana <vrana [at] php.net>, 2010-03-31

Introduction

Consider the following code:

<?php
if (authUser($_POST["login"], $_POST["password"])) {
    $auth = true;
}
if ($auth) {
    echo "Secret\n";
}
?>

You can easily spot the vulnerability in it. The $auth variable is not initialized in all cases so it can be spoofed from outside. PHP defines a way to handle uninitialized variables (unlike C language for example), they all have the null value. The problem is that the variable can be initialized from other sources:

  • Previous code
  • Included file
  • From outside if register_globals is enabled
  • By using of extract on an untrusted variable

Defense

The defense against this vulnerability is simple – always initialize all variables:

<?php
$auth = false;
if (authUser($_POST["login"], $_POST["password"])) {
    $auth = true;
}
?>

Now if $auth contained anything (or nothing) then it is always reset to false. It is a good idea to initialize variables unconditionally. The following code would work but it is not so robust:

<?php
if (authUser($_POST["login"], $_POST["password"])) {
    $auth = true;
} else {
    $auth = false;
}
?>

Now consider that someone would like to check also the IP address and do it in this way:

<?php
if ($_SERVER["REMOTE_ADDR"] == "127.0.0.1") {
    if (authUser($_POST["login"], $_POST["password"])) {
        $auth = true;
    }
} else {
    $auth = false;
}
?>

The $auth variable can be spoofed again.

Note: The attacker must know the variable name to spoof – he can guess it, get it from some error message, brute-force it, but most commonly he gets it from the source code (open-source applications or a former employee).

Disabling register_globals

It is important to mention that disabling register_globals is not a defense against this attack. It only closes the most common attack vector. It is of course a good idea to disable it but good applications that initialize all variables work independently of register_globals value. Thus, it is not necessary to emulate disabling of register_globals by some variation of the following code:

<?php
// never use this kind of code
if (ini_get("register_globals")) {
    foreach ($_REQUEST as $key =&gt; $val) {
        unset($$key);
    }
}
?>

Spotting of uninitialized variables

PHP has a mechanism to spot uninitialized variables – it issues E_NOTICE level error with most uses of uninitialized variables. There are however some problems with it:

  1. It does not warn about assigning to an uninitialized array.
  2. It warns about accessing non-existing index in properly initialized array.
  3. It is issued in runtime.

E_NOTICE does not warn about assigning to an uninitialized array

The first problem is most serious, consider the following code:

<?php
$config["password"] = "pwd";
if (isset($_POST["password"]) &amp;&amp; $_POST["password"] == $config["password"]) {
    echo "Secret information.\n";
}
?>

It issues no notices if an attacker spoofs the $config variable. If he passes a string then the code is interpreted like this:

<?php
$config[0] = "p";
// $config is a string so [] is used to access bytes in the string
// "password" is converted to number 0 because string positions are always integers
// only one byte can be written to [0] so "pwd" is interpreted as "p"
if (isset($_POST["password"]) &amp;&amp; $_POST["password"] == $config[0]) {
    echo "Secret information.\n";
}
?>

Now it is enough to guess the first character of password and send it along with spoofed $config.

E_NOTICE warns about accessing non-existing index in properly initialized array

The second problem is not security related but code-brevity related. Following code would issue a notice even if it works well and cannot be fooled:

<input name="search" value="<?php echo htmlspecialchars((string) $_GET["search"]); ?>" />

With notices enabled, we must rewrite it to a longer code with the same functionality:

<input name="search" value="<?php
if (isset($_GET["search"])) {
    echo htmlspecialchars((string) $_GET["search"]);
}
?>" />

Another example of this thoroughness – following code is perfectly valid and does what we expect from it (count group members):

<?php
$groups = array();
foreach (getData() as $data) {
    $groups[$data["id_group"]]++;
}
?>

With notices enabled, it must be rewritten like this:

<?php
$groups = array();
foreach (getData() as $data) {
    if (!isset($groups[$data["id_group"]])) {
        $groups[$data["id_group"]] = 0;
    } else {
        $groups[$data["id_group"]]++;
    }
}
?>

I would not say that this code is more clear and less error-prone (there is already one error included).

E_NOTICE is issued in runtime

The third problem of notices is security-related. If some usage of uninitialized variable is not spotted during the development then an attacker can use it. It is nice that you are informed about the usage of uninitialized variable from the error log but if it is used for an attack then it is too late. It is possible to make notices fatal by set_error_handler but it is not worth it for most applications.

Better spotting of uninitialized variables

I would not recommend disabling notices. However, it does not solve all problems and requires writing of more thorough code as you have seen. Luckily, there is a better way to spot uninitialized variables that solves all three problems of notices. It has a name php-initialized. It is a tool for analyzing PHP source code to spot uninitialized variables. It does not run the code and has some limitations but it can be used to check the code routinely for example after refactoring or before commit.

The biggest limitation is that only the current block is considered as the variable scope. Thus, the following code would complain about uninitialized $auth:

<?php
if (authUser($_POST["login"], $_POST["login"])) {
    $auth = true;
} else {
    $auth = false;
}
if ($auth) {
    echo "Secret\n";
}
// prints: Uninitialized variable $auth on line 7
?>

It is partially a political decision explained in this article – it is less error-prone to initialize variables unconditionally. Apart this limitation, the supported features covers nearly all parts of PHP.

Note: Author of this article is the main developer of php-initialized.

Do not use $_REQUEST

Variable spoofing does not involve only global variables. The $_REQUEST variable can be filled by other source than a programmer assumes. It is thoroughly explained by Stefan Esser. I would only append that as the contents of $_REQUEST can be affected by request_order since PHP 5.3.0 then an application could not rely on the contents of $_REQUEST and would not run on some configurations.

Summary

Good PHP application should always initialize all variables before usage. It is a good idea to turn off register_globals but a good application should not rely on it. PHP offers an E_NOTICE error level that can spot some uninitialized variables but it is not 100% reliable. Tool php-initialized solves the deficiencies of it.




  • I'd love to see PHP changing the scope of variables, e.g. a variable declared inside a statement (or any block really) should be out of scope outside of that block.
    This way you wouldn't need any notices, the code just wouldn't work, but would be very easy to fix.
  • cdamian
    In the first examples I would avoid the whole problem by directly assigning to the variable:

    $auth = authUser($_POST["login"], $_POST["password"]);

    Missing assignments can also spotted quiet easy if you have enough tests with good code coverage.
  • wellingtonrodrigues
    Greetings,

    Very cool your article but for matters of authentication would be much safer to carry the validity of the authentication session.

    Best regards,

    Wellington Rodrigues
    PHP Programmer Brazil
blog comments powered by Disqus