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 : http://www.sitepoint.com/forums/showthread.php?t=337881 — Vincent Mimoun-Prat, Jan 20 '11 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, Jan 20 '11 at 16:20
almost exact duplicate of http://stackoverflow.com/questions/60174/best-way-to-stop-sql-injection-in-php — jamesbtate, Jan 20 '11 at 16:20
**that's pretty enough** as long as you're using single-byte or utb-8 encoding. — Your Common Sense, Jan 20 '11 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, Jan 20 '11 at 16:22

alexn · Accepted Answer · 2011-01-20T16:39:00.810

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 '11 at 16:39

answered Jan 20 '11 at 16:16

alexn

57,867
14
111
145

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. – Tom-pouce Jan 20 '11 at 16:18
@Tom: check out [This article on the subject](http://shiflett.org/blog/2006/jan/addslashes-versus-mysql-real-escape-string) – ircmaxell Jan 20 '11 at 16:20
mysql_real_escape_string alone IS NOT ENOUGH to substitute addlashes. Most likely in your code there is no difference between these functions – Your Common Sense Jan 20 '11 at 16:24
I've already seen this article. But I didn't success to break my code. Could you give me examples? – Tom-pouce Jan 20 '11 at 16:24
leemings tend to upvote an answer on the topic they don't understand – Your Common Sense Jan 20 '11 at 16:26
Col. Shrapnel thanks, that seems to be correct, did not know that one. I thougt mysql_real_escape_string also escaped `%` chars etc. However, the 'safest' solution is still to use prepared statements thats not vulnerable to injections at all. – alexn Jan 20 '11 at 16:29
Edited your extremely popular answer a bit – Your Common Sense Jan 20 '11 at 16:31
1

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. – Your Common Sense Jan 20 '11 at 16:32
@Col: please explain how mres when used properly can be broken. You say it's not enough. Give us some proof. And can you give me any documented reason why you need to call `mysql_set_charset` (which wasn't introduced until 5.2.3) for mres to work? I've **never** heard or seen any evidence to that effect... Don't just go off making elitist remarks unless you have the proof to back it up. And also this is a civilized site, could you please stop calling people *lemmings* or other slurs. Show some respect for both everyone and for yourself... – ircmaxell Jan 20 '11 at 16:35
@Col as I said, when using prepared statements you're not vulnerable to sql injections. If you're doing `"ORDER BY $sort_field"` you're not using prepared statements. If not, i'm happy to be proven false. – alexn Jan 20 '11 at 16:37
@ircmaxell do a little research yourself. Yes, it's harder than typing usual mindless answers by repeating the same nonsense again and again, but.. just try it. – Your Common Sense Jan 20 '11 at 16:41
@alexn hell HAVE YOU EVER USED prepared statements yourself? If not - why do you mention it? How can it help you in this ORDER BY case? – Your Common Sense Jan 20 '11 at 16:42
@Col Yes I have. Please give me an example of a sql injection when using prepared statements and i'm more than happy to edit my answer. From the mysql documentation. Quote from the mysql documentation: `This is unnecessary when dealing with prepared statements. The separation of the data allows MySQL to automatically take into account these characters and they do not need to be escaped using any special function.`. http://dev.mysql.com/tech-resources/articles/4.1/prepared-statements.html – alexn Jan 20 '11 at 16:45
@Col: Where I come from, when you make a claim it's proper for you to provide the evidence to back it up. I have done research on the matter. I have not seen a single article or resource or documentation on your claim at all. So please, can you post proof? I will be glad to be proven wrong... And where do you see that you need to call `mysql_set_charset` in [the documentation](http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-string.html)? If you keep the default charset (which most do), there's no need to call it... Worth noting, but not "wrong"... – ircmaxell Jan 20 '11 at 16:45
@alexn I gave you that. Did you ever tried to understand that example? Or only you can do is mindless copy/paste? – Your Common Sense Jan 20 '11 at 16:49
@ircmaxell yes, that's the point. if ou keep the default charset which is latin1, your precious mres will act exactly the same way as blamed addslashes with not a single difference from it (save for mysql_escape_string's insignificant \n-like symbols) – Your Common Sense Jan 20 '11 at 16:52
The default charset may not be `latin1`. But it's worth noting. Just say so next time rather than saying it won't work... – ircmaxell Jan 20 '11 at 16:54
@ircmaxell: Exactly. The default charset is latin1 by default (pun not intended), but mysql config can override that to set a different default charset - so if you don't set a charset explicitly, you are in essence gambling (and it *will* bite you when you least expect it). – Piskvor left the building Jan 20 '11 at 20:19
@Piskvor so, that was my point too. use mysql_set_charset to alter defaults. If you, like that Shiflett, on some extraordinary rare encoding. Tell it to your friend :) – Your Common Sense Jan 20 '11 at 20:38
Also, I hate this way of suggestiong something - "use this **or** that". How it is supposed to choose from these options? Do **you** have your own opinion, which to use? Why not to tell it? – Your Common Sense Jan 21 '11 at 08:09

score 5 · Answer 2 · edited May 23 '17 at 10:32

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 '17 at 10:32

Community

1
1

answered Jan 20 '11 at 16:32

Piskvor left the building

91,498
46
177
222

(feel free to edit to a better explanation of hashing and salting, this is one I've found by quickly searching SO) – Piskvor left the building Jan 20 '11 at 16:37

score 2 · Answer 3 · answered Jan 20 '11 at 16:18

2

Use:

mysql_real_escape_string($inputToClean);

answered Jan 20 '11 at 16:18

diagonalbatman

17,340
3
31
31

this function being called alone can do nothing better than addslashes – Your Common Sense Jan 20 '11 at 16:24
1

@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 Jan 20 '11 at 16:36

score 2 · Answer 4 · answered Jan 20 '11 at 16:23

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 '11 at 16:23

Piskvor left the building

91,498
46
177
222

@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). – Piskvor left the building Jan 20 '11 at 16:41

score 2 · Answer 5 · edited Jan 20 '11 at 23:13

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

Matt Lowden · Answer 6 · 2011-01-20T16:18:08.677

1

Rather than addslashes you should use mysql_real_escape_string.

edited Jan 20 '11 at 16:18

answered Jan 20 '11 at 16:17

Matt Lowden

2,586
17
19

Protect against SQL injection

6 Answers6

Linked