This question already has an answer here:
I have an application that I have created in MYSQL and PHP, it is said that MYSQL is subject to SQL injection - (I respect that) but why? I have code that works fine in this condition or can someone can prove me wrong?
There is a form with 2 fields, username and password, and I post it on a page. Here is my code:
$user = stripslashes($user);
$pwd = trim($pwd);
$pwd = stripslashes($pwd);
$user = mysql_real_escape_string($user);
$pwd = mysql_real_escape_string($pwd);
$pwd = md5($pwd);
$rs = mysql_query("select * from `login` where upper(USER_ID) = upper('$user') AND PASS = '$pwd'");
My username is administrator.
So, that's my code, I am doing escaping, how can this be subjected to SQL injection? That's an open challenge :) for those who says PDO is the future :).
Thank you.
</div>
$user = stripslashes($user);
This is fine as long as you are using addslashes first.
$pwd = trim($pwd);
I don't like this, because you are silently removing characters from the password I chose. What if my password is: " liIo1sor&DINg "?
mysql_real_escape_string(x)
The problem with this is that you are turning down an alternative which can essentially assure that you are vulnerable to SQLi vs escaping, which has some potential for some random exploit to come along and break your site.
From: http://php.net/manual/en/pdo.prepared-statements.php
If an application exclusively uses prepared statements, the developer can be sure that no SQL injection will occur (however, if other portions of the query are being built up with unescaped input, SQL injection is still possible).
This is from the official PHP docs. Your method is possibly safe, but you have a pretty much perfect method available... so why go with probably safe?
So let's go under the assumption that mysql_real_escape_string() is perfect and will prevent injection universally. You still have the problem that if you build all your SQL queries the way you did in this post, then there is a chance that you (or another developer) may come along and add a param and forget to escape it. It is not a secure starting point, and encourages risky development.
md5($pwd);
As others have said md5 should not be used for hashing passwords. You should probably use bcrypt, scrypt, or pbkdf2 if you can. You are also omitting a per-user unique and random salt. This is especially important if you are set on not using prepared statements, since your database will inevitably be stolen (just kidding :P). More about storing passwords here: https://security.stackexchange.com/questions/211/how-to-securely-hash-passwords
$user = stripslashes($user);
$pwd = stripslashes($pwd);
$user = mysql_real_escape_string($user);
$pwd = mysql_real_escape_string($pwd);
$password = password_hash($password, PASSWORD_BCRYPT);c
$rs = mysql_query("select * from `login` where upper(USER_ID) = upper('$user') AND PASS = '$pwd'");
Is the secure way to do it, a quick note on login, you have to use mysql_real_escape_string, and mysql_query on the string again, and then run password_verify() on the output of those two functions. Also, you can use trim, but then you have to do it on login too. This may shorten the users passwords and make them easier to brute force so I would not recommend it.