Variable initialization in PHP

Školení, která pořádám

This article was published as the Month of PHP Security Submission.

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:

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 => $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"]) && $_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"]) && $_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 Steffan 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.

Jakub Vrána, Výuka, 17.5.2010, comments: 7 (new: 0)

Comments

Tomas:

I didn't use PHP for quite a long time, but assigning true/false to a variable as the only thing done in the if statement is a bit surprising. Are there any possible issues if you write just:

  <?php
  $auth = authUser($_POST["login"], $_POST["login"]);
  // rest of the code
  ?>

This seems much more natural to me.. Just wondering :-).
18.5.2010 05:12:44

ikona Jakub Vrána OpenID:

You are absolutely right, $auth = authUser() is much better in this simple case. The example shows only a bad design pattern. If there would be more conditions, database queries etc. then a simple assignment would be impossible.
18.5.2010 12:29:23

Ondrej Galbavý:

or this way: $auth = (bool)authUser(); if it returns object with user and only boolean is required
19.5.2010 18:55:39

aaa:

>> 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

What I don't like about unconditional variant is that is seems to be like default (or more expected) value of the variable which can be misleading. Because of this I often prefer:

if($boolean)
    $auth = true;
else
    $auth = false;
18.5.2010 10:11:47

ikona Jakub Vrána OpenID:

$auth = false really is the default value.
18.5.2010 12:30:28

Koubas:

Why it's not worth it using set_error_handler? Non-fatal errors are one thing that bothers me about PHP. The language it self cannot decide, whether some error is critical or not, because even a warning means that something went wrong and application could be in an unpredictible state (assuming that no today's php programmer releases code throwing warnings that he knows about). Or is something wrong about set_error_handler mechanism itself?
21.5.2010 13:51:25

Miško:

Ďakujem, pekný článok.
22.5.2010 14:02:37

Diskuse je zrušena z důvodu spamu.

avatar © 2005-2024 Jakub Vrána. Publikované texty můžete přetiskovat pouze se svolením autora. Ukázky kódu smíte používat s uvedením autora a URL tohoto webu bez dalších omezení Creative Commons. Můžeme si tykat. Skripty předpokládají nastavení: magic_quotes_gpc=Off, magic_quotes_runtime=Off, error_reporting=E_ALL & ~E_NOTICE a očekávají předchozí zavolání mysql_set_charset. Skripty by měly být funkční v PHP >= 4.3 a PHP >= 5.0.