Protect against SQL injection

Question

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;
            }
        }
    }
    ?>

google addslashes VS mysql_real_escape_string : sitepoint.com/forums/showthread.php?t=337881 — Vincent Mimoun-Prat
– Vincent Mimoun-Prat, Commented Jan 20, 2011 at 16:20
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
– Álvaro González, Commented Jan 20, 2011 at 16:20
almost exact duplicate of stackoverflow.com/questions/60174/… — jamesbtate
– jamesbtate, Commented Jan 20, 2011 at 16:20
that's pretty enough as long as you're using single-byte or utb-8 encoding. — Your Common Sense
– Your Common Sense, Commented Jan 20, 2011 at 16:21
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
– Your Common Sense, Commented Jan 20, 2011 at 16:22

alexn · Accepted Answer · 2011-01-20 16:39:00Z

14

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.

edited Jan 20, 2011 at 16:39

answered Jan 20, 2011 at 16:16

alexn

59.2k14 gold badges113 silver badges146 bronze badges

Sign up to request clarification or add additional context in comments.

20 Comments

Tom-pouce Over a year ago

ok, but could you give me an example of SQL injection for this code? I don't understand how can I break this code whereas I'm escaping quotes in it.

ircmaxell Over a year ago

@Tom: check out This article on the subject

Your Common Sense Over a year ago

mysql_real_escape_string alone IS NOT ENOUGH to substitute addlashes. Most likely in your code there is no difference between these functions

Tom-pouce Over a year ago

I've already seen this article. But I didn't success to break my code. Could you give me examples?

Your Common Sense Over a year ago

You are wrong abut "not vulnerable to injections at all". What if you're going to make a query "ORDER BY $sort_field". and still, without mysql_set_charset your mysql_real_escape_string will behave exactly the same as addslashes.

|

2 revs · Accepted Answer · 2017-05-23 10:32:29Z

5

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.

edited May 23, 2017 at 10:32

community wiki

2 revs
Piskvor

1 Comment

Piskvor left the building Over a year ago

(feel free to edit to a better explanation of hashing and salting, this is one I've found by quickly searching SO)

diagonalbatman · Accepted Answer · 2011-01-20 16:18:11Z

2

Use:

mysql_real_escape_string($inputToClean);

answered Jan 20, 2011 at 16:18

diagonalbatman

18.1k3 gold badges33 silver badges31 bronze badges

2 Comments

Your Common Sense Over a year ago

this function being called alone can do nothing better than addslashes

Piskvor left the building Over a year ago

@Col. Shrapnel: It is an epsilon better than addslashes (Unicode representations and whatnot) - but you are right that a programmer using it as a abracadabra-it-is-now-secure magic dust is no safer than with addslashes.

Piskvor left the building · Accepted Answer · 2011-01-20 16:23:22Z

2

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.

answered Jan 20, 2011 at 16:23

Piskvor left the building

93.1k46 gold badges182 silver badges226 bronze badges

1 Comment

Piskvor left the building Over a year ago

@Tom-pouce: You're welcome. btw, that's eeexactly the case against extract - it creates variables "out of thin air" (I've been bitten by this, and it is really hard to debug).

Jonathan Leffler · Accepted Answer · 2011-01-20 23:13:57Z

2

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? ;-)

edited Jan 20, 2011 at 23:13

Jonathan Leffler

759k145 gold badges961 silver badges1.3k bronze badges

answered Jan 20, 2011 at 16:32

Álvaro González

147k45 gold badges282 silver badges378 bronze badges

Comments

Matt Lowden · Accepted Answer · 2011-01-20 16:18:08Z

1

Rather than addslashes you should use mysql_real_escape_string.

edited Jan 20, 2011 at 16:18

answered Jan 20, 2011 at 16:17

Matt Lowden

2,61618 silver badges19 bronze badges

Collectives™ on Stack Overflow

Protect against SQL injection

6 Answers 6

20 Comments

1 Comment

2 Comments

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

20 Comments

1 Comment

2 Comments

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related