Is this PHP code vulnerable to SQL injection?

Question

I have a website where I use PHP in server side and mysql as database. I use the following script to retrieve data from database. Could anybody let me know whether this code is vulnerable to injection attack? If so could you please give a solution?

<?php
// PHP script 

$usrname=$_POST['usrname'];
$_SESSION['usremail']=$usrname;
$usrpassword=$_POST['passwd']; 
$db=mysql_select_db('mydb',$connection);
$result=mysql_query("select usrfname,usrlname from userinformation where usremail='$usrname' and usrpassword='$usrpassword'") or die('failed to login');

Any help is greatly appreciated.

Thanks

Is this a question on a quiz? Where does the value of `$usrname` come from? How is it "protected", if at all? — , Apr 28 '12 at 17:32
Yes, this is vulnerable to injection. Also, it looks like you're storing passwords in plaintext; not a good idea. — Jimmy Sawczuk, Apr 28 '12 at 17:32
**When you downvote a question give a reason**. This is a perfectly good question and definitely not "too localized". — nico, Apr 28 '12 at 17:36
@pst: being in `$_POST` it must come from a form, and I don't see any way of (seriously) protecting it. — nico, Apr 28 '12 at 17:44
@nico Hence the quotes. Also, there are multiple types of "protection" and the *only* one that applies to SQL-Injection is the kind that ensures the statement structure cannot be altered; there may be other non-SQL-Injection usage problems, however. (Placeholders are the universal solution to this problem.) — , Apr 28 '12 at 19:00
@pst: maybe I am missing something, but in what way would you protect something coming from POST? (I mean, aside from cleaning the string later in PHP) — nico, Apr 29 '12 at 08:51
@nico That's the point: **nothing** is being done here so there is no way that an SQL injection attack is thwarted under any circumstances. It (correct SQL quoting) *should* be done at the SQL boundary to protect SQL injection attacks (again, *placeholders* accomplish this). Other sanitizing is for business rules and is generally done sooner (but it might be part of the RDBM rules/triggers). It *may* be the business rule "sanitizing" covers SQL injection attacks (see user602088's answer), however, this should *not* be use in-leui of proper SQL usage (again, *placeholders*). — , May 01 '12 at 01:39
@pst: ah OK, I tought you had some way to protect the POST before it gets to the PHP page, which is not possible. Placeholders etc. are all manipulations done **after** the form is submitted. — nico, May 01 '12 at 06:41
***Please [stop using `mysql_*` functions](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php).*** [These extensions](http://php.net/manual/en/migration70.removed-exts-sapis.php) have been removed in PHP 7. Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [PDO](http://php.net/manual/en/pdo.prepared-statements.php) and [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and consider using PDO, [it's really pretty easy](http://jayblanchard.net/demystifying_php_pdo.html). — Jay Blanchard, May 12 '17 at 13:14
The question of comments + downvotes has been discussed ad nauseum on Meta. Many folks just choose to DV and move on. Many offer advice. Many try to light the path for newbies. But no one is ever obligated to give reasons @nico, because the arrow has reasons associated with it. — Jay Blanchard, May 12 '17 at 13:15
[Little Bobby](http://bobby-tables.com/) says ***[your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php)***. Even [escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! — Jay Blanchard, May 12 '17 at 13:15
@JayBlanchard I know that has been discussed ad nauseam, however those comments were made in 2012 :) — nico, May 12 '17 at 16:14

score 6 · Accepted Answer · edited May 23 '17 at 11:47

Yes, it's vulnerable. You're talking values directly from user input and placing it into your query.

You should look at mysql_real_escape_string, or (preferably) use MySQLi which provides parameterised queries. SQL injections are caused by user data being injected as SQL code instead of data. The only true way to secure a query is to use parameterised queries, which separate the data and query text at the protocol level.

Furthermore, your passwords are stored in plaintext. You should use a salted hash function as an absolute minimum.

You should also take a look at these awesome questions:

+1 for "use ... parametrized queries" – Apr 28 '12 at 18:59 — , Apr 28 '12 at 18:59

score 4 · Answer 2 · answered Apr 28 '12 at 17:33

4

Of course it's vulnerable. You are never sanitizing your inputs. Although mysql_* functions are deprecated, you will still find use of the mysql_real_escape_string function. Just apply it to your variables.

$usrname = mysql_real_escape_string($_POST['usrname']);
$usrpassword = mysql_real_escape_string($_POST['passwd']);

answered Apr 28 '12 at 17:33

Marcus Recck

4,879
2
14
26

1

Or use parameterised queries like you're supposed to these days. – Polynomial Apr 28 '12 at 17:35
Some of the mysql_ functions have been deprecated, but not all of them. – Sam Dufel Apr 28 '12 at 17:35
2

Of course, using PDO or MySQLi would be a much better solution. – Marcus Recck Apr 28 '12 at 17:40
Keep in mind that `mysql_real_escape_string` is **NOT** infallable. It simply performs a blacklist filter on characters that must be escaped. It does not separate data from query language. As such, I can pass `1=1` in and have it completely screw up the logic of your query. If you want to have real security against SQL injection, and I really hope you do, you have to use a mechanism that entirely separates query language from input data - parameterised queries in MySQLi and PDO can do that. – Polynomial Apr 29 '12 at 10:25
@Polynomial: can you give an example of how you would pass 1=1 in? The `mysql_real_escape_string` removes all single quotes, so your 1=1 would always be '1=1' in the query, just a string, no? – Konerak May 04 '12 at 09:24
Not if you're doing a query that doesn't use quotes, for example a numeric comparison, e.g. `SELECT * FROM products WHERE price > $price` – Polynomial May 06 '12 at 14:31
Just to be clear, the above can be exploited with `0 OR 1=1`, or worse: `0 OR 1=1 LIMIT N,1 --`, which will ignore other query constraints and return the Nth row in the table. – Polynomial May 06 '12 at 22:32

score 2 · Answer 3 · answered Apr 28 '12 at 17:33

2

Yup, it is..

Do this instead:

$usrname=$_POST['usrname'];
$_SESSION['usremail']=$usrname;
$usrname=mysql_real_escape_string($usrname);
$usrpassword=mysql_real_escape_string($_POST['passwd']); 
$db=mysql_select_db('mydb',$connection);
$result=mysql_query("select usrfname,usrlname from userinformation where usremail='$usrname' and usrpassword='$usrpassword'") or die('failed to login');`

You should also look into Prepared Statements.

answered Apr 28 '12 at 17:33

2

As I've stated on other answers, don't use the old deprecated functions. Prepared statements are the only way to go if you want any semblance of security. – Polynomial Apr 28 '12 at 17:41

score 1 · Answer 4 · answered Apr 28 '12 at 17:38

1

use mysql_real_escape_string() on user generated strings you want to use in queries.

$usrname     = mysql_real_escape_string($_POST['usrname']);
$usrpassword = mysql_real_escape_string($_POST['passwd']); 

$db=mysql_select_db('mydb',$connection);
$result = mysql_query("SELECT usrfname, usrlname FROM userinformation WHERE usremail='$usrname' AND usrpassword='$usrpassword'") or die('failed to login');

// Set session data only if login is successful 
$_SESSION['usremail']=$usrname;

answered Apr 28 '12 at 17:38

fhugas

382
1
10

1

Please don't advocate using deprecated functions. Prepared statements are the only way to provide solid security against SQL injection attacks. – Polynomial Apr 28 '12 at 17:40
`mysql_real_escape_string()` is not a deprecated function, and although prepared statements makes life easier it is not the _only_ way to provide security against SQL injections, it only allows you to write a little more sloppy code. ;) – fhugas Apr 28 '12 at 17:51
2

http://php.net/manual/en/pdo.prepared-statements.php is one place to start looking into "prepared statements" After 30 seconds of reading it will become obvious why this will give you a quantum leap in security. – Claude Apr 28 '12 at 17:51
@fhugas But `mysql_query` is deprecated, and the PHP devs advise not using the old style procedural query functions. The `mysql_real_escape_string` function is not 100% effective against SQL injections, either. For example, if you use `"SELECT * FROM users WHERE id = " . mysql_real_escape_string($_GET['id'])`, I can inject `1=1` into the ID field and have the query return all values. Potential security problem right there :) – Polynomial Apr 28 '12 at 17:55
Hehe, but it's still not deprecated ;) Looks like you forgot to quote that input in your query there `"SELECT * FROM users WHERE id = '" . mysql_real_escape_string($_GET['id']) ."'"; Now you can't inject anything.. :) – fhugas Apr 28 '12 at 18:00
Thanks a lot guys for your valuable inputs. – day_dreamer Apr 29 '12 at 06:48
[Escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! – Jay Blanchard May 12 '17 at 13:16

score 0 · Answer 5 · answered Apr 28 '12 at 17:41

0

Yes, you should never accept user input (from a form (POST data), as part of a url (GET data) or even a cookie without first checking it is what you would expect. For example, your username might consist of a to z perhaps with a dot allowed. So you would check that that is all it contains before you let the input text near the database. Google preg_match.

And you need to do it at the server side, just using javascript at the browser is not enough.

I would also store the password as a md5 hash of what the user entered when they signed up, so you get the $password at signup, check it has a limited range of characters, do $hass_pw=md5($password) and store the $hash_pw in the d/b. Then when the user logs in, you hash it again and use that in the query. (I left out $_POST for clarity here).

eg:

if preg_match(/"[^a-z]/i", $_POST['usrname'])
    print "bad username format)
else    
    {
    // good format username, safe to use in query
    }

answered Apr 28 '12 at 17:41

KevInSol

2,380
3
30
40

This talks about how to sanitize input (it is just a side-effect that the *particular* match is a subset of characters that will effectively prevent an SQL Injection attack). It does not talk about SQL Injection, however. SQL Injection is a specific vulnerability where the *structure* of an SQL query can be altered. There may be other vulnerabilities and/or ways to violate business rules that need to be addressed, which is what this answer covers. – Apr 28 '12 at 19:04
MD5 to store a password? No, that is unsafe and for a long time, even given the date this was posted. – Funk Forty Niner May 12 '17 at 12:34
***You really shouldn't use [MD5 password hashes](http://security.stackexchange.com/questions/19906/is-md5-considered-insecure)*** and you really should use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. Make sure you [don't escape passwords](http://stackoverflow.com/q/36628418/1011527) or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard May 12 '17 at 13:13

Is this PHP code vulnerable to SQL injection?

5 Answers5

Linked