0

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.

Gray
  • 7,050
  • 2
  • 29
  • 52
user3646405
  • 55
  • 7
  • 3
    it's time to step into the 21st century. That MD5 has seen better days also. – Funk Forty Niner Sep 11 '15 at 16:55
  • 3
    There are ways around `mysql_real_escape_string`. They usually involve weird unicode characters, though. `md5` isn't secure, pretty sure the people who made it have said so. – gen_Eric Sep 11 '15 at 16:56
  • As mentioned, MD5 is deprecated as insecure. You should switch to [password_hash](http://php.net/manual/en/function.password-hash.php) – Machavity Sep 11 '15 at 16:57
  • While this code *may* be "secure", ***why*** do you refuse to upgrade? `mysql_query` is actually going to be *removed* in future versions of PHP, so you may no longer be able to use it. – gen_Eric Sep 11 '15 at 16:57
  • @Fred-ii- I wont say that you are wrong, but ofcourse as script kid I want to know why, I dont mind stepping in 21st century – user3646405 Sep 11 '15 at 16:57
  • 1
    You really shouldn't use MD5 password hashes and you really should use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). – Jay Blanchard Sep 11 '15 at 16:58
  • @RocketHazmat ofcourse upgrade needs time investment. – user3646405 Sep 11 '15 at 16:58
  • 1
    you probably could still get away with the `mysql_` functions, but that MD5 will eventually bite you back, harder than a junkyard dog. – Funk Forty Niner Sep 11 '15 at 16:59
  • 3
    The why? The `mysql_` API has gaping security holes and some of the functions, like `mysql_real_escape_string()` are easily defeated. Once defeated you may be introduced to [Little Bobby Tables](https://xkcd.com/327/). In addition, the hacks for MD5 are widely publicized, making the hash incredibly easy to break. – Jay Blanchard Sep 11 '15 at 16:59
  • *"ofcourse upgrade needs time investment"* - It will be time well-spent and a good "investment" at best. – Funk Forty Niner Sep 11 '15 at 17:00
  • @Fred-ii- ok boss :) – user3646405 Sep 11 '15 at 17:00
  • It's not really that your approach is completely insecure. It should stop the majority of injection issues, but to be much safer, you can do a few more things. First and foremost, instead of escaping your input and using string concatenation to build your queries, use prepared statements. Next, use the built in password hashing MySQL function. And you should also move from the mysql_* functions (that are deprecated) to pdo_mysql or mysqli. – Kevin Nagurski Sep 11 '15 at 17:01
  • [PDO is really not hard](http://jayblanchard.net/demystifying_php_pdo.html). – Jay Blanchard Sep 11 '15 at 17:01
  • Then you have XSS injection to think about... which is a different animal altogether. That even applies to using prepared statements, believe it or not. – Funk Forty Niner Sep 11 '15 at 17:02
  • @Fred-ii- an example is worth thousand logics and reasons :) my code is there ,its in mysql, no prepared statement, but its Secure, belive it or not :) – user3646405 Sep 11 '15 at 17:05
  • Have you got an example of this running on a live site? If so, please share the url and let others ') have alook to see if it really is secure – Professor Abronsius Sep 11 '15 at 17:13
  • @RamRaider not on a live site, it was on my localhost, but as I said, it just has 2 forms and then this php code. – user3646405 Sep 11 '15 at 17:24
  • Sadly, the 2 referenced reasons for closing this (**1.** The question from 2011, and **2.** the Answer from the same guy a few days ago who as of yet refuses to take followup Questions to his Answer) clearly do not represent much clarity or authority on the topic. Oddly, also the deciding vote on closing this, and he who swept thru here with downvotes in his wake last nite. So much for open transparent discussions. Rather, enter fiefdoms – Drew Sep 13 '15 at 11:51

2 Answers2

0
$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

Community
  • 1
  • 1
Gray
  • 7,050
  • 2
  • 29
  • 52
  • I think I know whatever you mentioned in the answer, since I studied that in my university too, but as I said example is worth thousand logics, even my code is in mysql, it is not being vulnerable :) – user3646405 Sep 12 '15 at 16:51
  • Why ask a question if you don't want advice? The vulnerability is when you are maintaining and forget to escape a param. – Gray Sep 12 '15 at 16:53
  • respectfully the advices are powered by some logics and reason specially proofs – user3646405 Sep 12 '15 at 18:20
  • I'm not sure I understand what you are trying to say with that comment, but it seems like the only way to answer this question is either to say "you are totally right" or demonstrate a 0-day against the function? – Gray Sep 12 '15 at 20:06
0
$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.

Jake Sylvestre
  • 936
  • 3
  • 14
  • 39