Protection against SQL injection

Question

Does the following PHP MySQL statement protect against SQL Injection?

$strSQL = "SELECT * FROM Benutzer WHERE Benutzername = '".$Benutzer."' AND Password = '".md5($PW)."'";

The Variables $Benutzer and $PW are inputs from the User.

We're checking the username and password against common SQL Injection techniques:

' or 0=0 --, " or 0=0 --, or 0=0 --, ' or 0=0 #, " or 0=0 #, or 0=0 #, ' or 'x'='x, " or "x"="x, ') or ('x'='x, ' or 1=1--, " or 1=1--, or 1=1--, ' or a=a--, " or "a"="a, ') or ('a'='a, ") or ("a"="a, hi" or "a"="a, hi" or 1=1 --, hi' or 1=1 --, hi' or 'a'='a, hi') or ('a'='a and hi") or ("a"="a.

Am I missing something? Should I use a different method to protect against SQL injection?

where are those variables coming from? you mention skipping login so does this mean those variables are coming from a cookie? — robjmills, Sep 22 '09 at 13:27
the variables are coming from a $_POST and will be sent to a function. After a successful login we write a Session-Var. With skipping i mean to login without a valid password. - by the way - thanks for so many quick replies. — wildhaber, Sep 22 '09 at 13:37
If you ever see SQL being produced by appending strings together, loud alarm bells should go off. Always, always use bound parameters, if available. See PDO and MySQLi libraries in PHP. — Cheekysoft, Sep 24 '09 at 11:51

score 11 · Accepted Answer · answered Sep 22 '09 at 13:27

You may want to look into parameterized queries for querying the database. This eliminates SQL injection attacks.

I work primarily with postgreSQL, and the format for doing such a query would look something like this:

$query = 'select * from Benutzer where Benutzername = $1 and Passwort = $2';
$params = array($Benutzer, md5($PW));
$results = pg_query_params($query, $params);

Most databases have a function that will be similar to this funationality.

I hope this helps and good luck!

Kyle

PDO works like this. see http://fr2.php.net/manual/en/pdostatement.execute.php — Aif, Sep 22 '09 at 13:30

Ryan McCue · Answer 2 · 2009-09-22T13:33:18.433

5

I'd personally advise using an algorithm such as SHA1 over MD5, as there is a lower possibility of collisions. You should be salting your passwords too, otherwise one can use rainbow tables to crack your passwords.

Also, use mysql_real_escape_string() to escape things, don't simply do a search and replace for common injections.

edited Sep 22 '09 at 13:33

answered Sep 22 '09 at 13:25

Ryan McCue

1,628
14
23

9

No, do *not* use string escaping. String escaping is very easy to do wrong, and hard to do right. Use bound parameters. Bound parameters are sent to the database through alternate channels, and so cannot possibly be parsed as SQL. – nobody Sep 22 '09 at 13:33
5

`mysql_real_escape_string()` is executed by the MySQL server, and I believe the MySQL guys know a thing or two about getting that stuff right. – Ryan McCue Sep 22 '09 at 13:36
Or, rather, the MySQL library in PHP, which is made by the MySQL team, IIRC. – Ryan McCue Sep 22 '09 at 13:41
thank you all - we will add a little bit of salt to the password and use the mysql_real_escape_string() to protect the user inputs we used in our sql-statements. – wildhaber Sep 22 '09 at 13:46
@Ryan McCue: you'd hope so, but not really. mysql_real_escape_string() exists because its predecessor mysql_escape_string was horribly broken. And anyway it's still a flawed methodology. – nobody Sep 22 '09 at 14:08
MySQL string literal escaping isn't hard to do right, it's just easy to forget to do, which is why parameterised queries are a good thing. But Ryan is correct that escaping is the right concept (whether explicitly or via parameters), whilst trying to sniff for “bad” strings that “might” be signs of an injection attempt is totally the wrong thing: laughably incomplete, whilst also blocking some valid input. – bobince Sep 22 '09 at 17:06

score 5 · Answer 3 · answered Sep 22 '09 at 13:39

5

Prepared statements are a fantastic invention.

http://www.php.net/manual/en/pdo.prepare.php and http://www.php.net/manual/en/mysqli.prepare.php for examples.

answered Sep 22 '09 at 13:39

Björn

29,019
9
65
81

score 2 · Answer 4 · answered Sep 22 '09 at 13:26

2

If security is supposed to be as high as possible, you ought to use some other hashing method than md5 - like e.g. sha256. Also, use a password salt.

answered Sep 22 '09 at 13:26

gnud

77,584
5
64
78

1

He is asking about SQL injection. – Pawka Sep 22 '09 at 13:34
1

Yes, and that part was already answered. He also said that he wants high security - so I gave him some extra tips, free of charge. – gnud Sep 22 '09 at 14:03

TigerTiger · Answer 5 · 2009-09-22T13:30:47.140

0

Try using mysql_real_escape_string()

$strSQL = "SELECT * 
        FROM Benutzer 
        WHERE Benutzername = '".mysql_real_escape_string($Benutzer)."' 
        AND Passwort = '".mysql_real_escape_string(md5($PW))."'";

Make sure that when users register, you register using the same technique .. for example,

AND Passwort = '".mysql_real_escape_string(md5($PW))."'"

and

AND Passwort = '".md5(mysql_real_escape_string($PW))."'"

may produce different results depending upon the input text.

And yes, also add some "salt" to this password as well. For some ideas you do like this

sha1(md5($pass)):
sha1($salt.$pass):
sha1($pass.$salt):
sha1($username.$pass):
sha1($username.$pass.$salt):

edited Sep 22 '09 at 13:30

answered Sep 22 '09 at 13:24

TigerTiger

10,590
15
57
72

2

There is no need to escape the password with `mysql_real_escape_string()`, since MD5s are hexadecimal numbers, and will not contain anything but the characters 0-9 or A-F. – Ryan McCue Sep 22 '09 at 13:30
2

No, do not use string escaping. Use bound parameters. – nobody Sep 22 '09 at 13:34
2

There may be no need to escape in that particular case, but it's a good habit to get into to use the escaping on *every* string if you're going to be using string-building. Separation of concerns: it's not the job of your query builder to know what are valid characters in an MD5 digest, and maybe in the future someone will swap the md5 call for something else that can include characters you do need to worry about. – bobince Sep 22 '09 at 17:09

score 0 · Answer 6 · answered Sep 22 '09 at 14:57

The security standards should be as high as possible.

If md5 got included in your design with nobody raising an eyebrow or spitting coffee on themselves, then you need to revisit the security issue with the client. If security is that critical they need to spring for a security specialist to join your team. Everybody says they want secure code. Few are willing to pay for it, so few actually get it.

score 0 · Answer 7 · answered Sep 22 '09 at 15:26

0

Be sure to strip_tags any public content (or use your own html filter) to prevent XSS as well.

answered Sep 22 '09 at 15:26

Citizen

12,430
26
76
117

htmlspecialchars() during the output phase is the correct thing to do to prevent XSS. – bobince Sep 22 '09 at 17:06
Unless you want to display some user generated html... – Citizen Sep 23 '09 at 17:30

Protection against SQL injection

7 Answers7