Protect against SQL injection

6
votes

I'm developing a website and I'm trying to secure the connection part.

I used the addslashes function on $login to stop SQL injection but some friends told me that's not enough security. However, they didn't show me how to exploit this vulnerability.

How can I / could you break this code? How can I secure it?

<?php

    if ( isset($_POST) && (!empty($_POST['login'])) && (!empty($_POST['password'])) )
    {
        extract($_POST);
        $sql = "SELECT pseudo, sex, city, pwd FROM auth WHERE pseudo = '".addslashes($login)."'";
        $req = mysql_query($sql) or die('Erreur SQL');
        if (mysql_num_rows($req) > 0)
        {
            $data = mysql_fetch_assoc($req);
            if ($password == $data['pwd'])
            {
                $loginOK = true;
            }
        }
    }
    ?>
6
phpsqlsecuritysql-injection
google addslashes VS mysql_real_escape_string : sitepoint.com/forums/showthread.php?t=337881 – Vincent Mimoun-Prat
There're many outdated tutorials out there that suggest addslashes() as a mechanism to escape stuff in SQL queries. If you are learning from one of those, I suggest you try to find something more up-to-date and accurate. Also, extract($_POST) is a nice example of vulnerability; don't do it! BTW, welcome to StackOverflow. – Álvaro González
almost exact duplicate of stackoverflow.com/questions/60174/… – jamesbtate
that's pretty enough as long as you're using single-byte or utb-8 encoding. – Your Common Sense
However you're suffering from much worst injection, out of extract() function. What if there will be loginOk field in the form?.. – Your Common Sense

6 Answers

14
votes

You should use mysql_real_escape_string for escaping string input parameters in a query. Use type casting to sanitize numeric parameters and whitelisting to sanitize identifiers.

In the referenced PHP page, there is an example of a sql injection in a login form.

A better solution would be to use prepared statements, you can do this by using PDO or mysqli.

5
votes

You are storing your passwords in plaintext! That's a major security issue if ever I saw one. What to do about that: at least use a (per-user) salted hash of the password, as seen e.g. here.

2
votes

Use:

mysql_real_escape_string($inputToClean);
2
votes

There's another gaping security hole - extract. It may save you from typing a few characters, but opens up holes too numerous to mention, for it will overwrite any global variables.

What happens if I post this?

$_POST {
    'login' => 'Admin',
    'loginOK' => 1
}

Guess what, $loginOK is now == 1 , and I'll be logged in as Admin.

Save yourself a lot of grief later, and just use the variables you want to use, instead of relying on the horrible hack that is extract.

2
votes

Apart from the usage of addslashes(), these are some random issues found in this code:

  • isset($_POST) is always TRUE, unless you run it from the command line. You can probably remove it.
  • empty() is very tricky. For instance, if $password = '0' then empty($password) is TRUE.
  • You can do this: if( isset($_POST['login']) && $_POST['login']!='' ){}
  • extract($_POST) is a huge vulnerability: anyone can set variables in your code from outside.
  • $password == $data['pwd'] suggests that you are storing plain text passwords in your database. That's a terrible practice. Google for "salted password".
  • You can also do $loginOK = $password == $data['pwd'];. Do you realise why? ;-)
1
votes

Rather than addslashes you should use mysql_real_escape_string.