CODEWITHSUNDEEP
phpsqlsecuritysql-injectionowasp
joey
joey

Reputation: 87

SQL injection vulnerable code even when we are sanitizing the input mysql_real_escape_string

We have been attacked; the hackers entered the system from a page <login> that's in the code shown below, but we couldn't figure out the actual problem in this code.

Could you point out the problem in this code and also a possible fix?

    <?php
        //login.php page code
        //...
        $user = $_POST['user'];
        $pass = $_POST['password'];
        //...
        mysql_connect("127.0.0.1", "root", "");
        mysql_select_db("xxxx");

        $user = mysql_real_escape_string($user);
        $pass = mysql_real_escape_string($pass);
        $pass = hash("sha1", $pass, true);
        //...
        $query = "select user, pass from users where user='$user' and pass='$pass'";
        //...

    ?>

Upvotes: 7

Views: 2708

Answers (5)

Robbie
Robbie

Reputation: 17710

(Supplementary to the other answers / comments about using PDO, correct use of passwords etc; Logging this here in case someone else stumbles on this question.)

No one has pointed out:

mysql_connect("127.0.0.1","root","");
mysql_select_db("xxxx");

as being a point of weakness.

This means that: - the DB server is on the same host as the web server, and therefore has a network interface to the world. - this have the most basic user (root) available, - and without a password.

Hopefully this is an example/test, but if not, ensure that at least the server port (3306) is blocked by firewall / not accessible externally.

Otherwise a simple mysql -h [webserver address] -u root will connect and it's game over.

Upvotes: 0

Tom Robinson
Tom Robinson

Reputation: 1920

You can fix your existing code, without breaking any of the existing passwords, by adding one line:

 $pass = $_POST['password'];                 // the actual password
 $pass = mysql_real_escape_string($pass);    // escaped version of the actual password
 $pass = hash("sha1",$pass, true);           // binary hash of the escaped password
 // At this point, $pass is the exact string that is stored in the database.
 $pass = mysql_real_escape_string($pass);    // ***ADD THIS LINE***
 $query = "select user, pass from users where user='$user' and pass='$pass'";

Note that the password stored in the database is the binary hash of the escaped version of the actual password. Since it is a binary string, you need to escape it. Be sure to add the extra escaping to the code that stores the password in the first place, otherwise password setting will also have a SQL injection vulnerability.

Upvotes: -2

MIvanIsten
MIvanIsten

Reputation: 490

You can rewrite your validation logic as a quick fix to the issue explained by @zerocool.

// don't send password hash to mysql, user should be uniqe anyway
$query = "select user, pass from users where user='$user'";

// validate hash in php
if (hash_equals(hash('sha1', $pass, true), $user_hash_from_db)){...}

And as others wrote, stop using mysql_* functions ASAP, and use stronger hashing algo.

Upvotes: -1

Your Common Sense
Your Common Sense

Reputation: 157839

To supplement the excellent answer from zerocool.

The problem here is the false notion that mysql(i)_real_escape_string prevents SQL injection. Unfortunately, too many people have been led to believe that this function's purpose is to protect them from injections. While of course it is not nearly true.

Had the author of this code the correct understanding of this function's purpose (which is escaping special characters in a string literal), they would have written this code as 

$user = mysql_real_escape_string($user);
$pass = hash("sha1", $pass, true);
$pass = mysql_real_escape_string($pass);

and there wouldn't have been any injections at all.

And here we come to an important conclusion: given escaping's purpose is not to prevent SQL injections, for such a purpose we should use another mechanism, namely prepared statements. Especially given the fact that mysql extension doesn't exist in PHP anymore while all other extensions support prepared statements all right (yet if you want to reduce the pain of transition you should definitely use PDO, however paradoxical it may sound).

Upvotes: 5

zerocool
zerocool

Reputation: 3502

The problem here is in $pass= hash("sha1",$pass, true);

You need to put it like this $pass= hash("sha1",$pass, false);

A good option is to move to PDO.

Let's see why this happen:

What your code is doing is returning a raw binary hash that means at a point in time the hash may contain an equal character =, for your example the hash that going to result in SQL injection in this case is "ocpe" because hash ("ocpe",sha1) have a '=' character, but how can I figure that out?

You only need to run a simple brute force and test if it contains a '=' inside the hash raw bit.

This is a simple code which can help you with that

<?php
$v = 'a';
while(1)
{
        $hash = hash("sha1",$v, true);
        if( substr_count( $hash, "'='" ) == 1 ) {
            echo $v;
            break;
        }
        $v++;
}

?>

Now you you have a string that gives a hash that has an equal inside of it '='

The query becomes:

$query = "select user, pass from users where user='$user' and pass='hash("ocpe",sha1)'";

then

$query = "select user, pass from users where user='$user' and pass='first_Part_of_hash'='Second_part_of_hash'";

In this case I assume that ocpe string has a hash of this format first_Part_of_hash'='Second_part_of_hash

Because pass='first_Part_of_hash' going to result in 0 and 0='Second_part_of_hash' is typecasted by the SQL engine, but in case of string if we type cast it to a int it's going to give as 0 ((int)'Second_part_of_hash' is result in 0)
so in the end 0=0 

$query = "select user, pass from users where user='$user' and 0=0";

Which going to result in "true" every time and as you can see it can be applied to all hash functions like MD5 and sha256 etc.

Good resources to check:

How can I prevent SQL injection in PHP?

Could hashing prevent SQL injection?

Upvotes: 16

Related Questions