3

How dangerous is this php code? What can be done about it?

$name = $_POST["user"]; $pwd = $_POST["pwd"]; $query = "SELECT name,pwd FROM users WHERE name = '$name' AND pwd = '$pwd'";
Improve this question
3
  • This sure sounds like a homework question... Commented Mar 6, 2009 at 18:58
  • No, I'm just looking at an application at work :( Commented Mar 6, 2009 at 19:04
  • 2
    How could you quantify danger? Megajowles of danger? seems like you might want to rephrase the question, to something like "why is this dangerous?" Commented Mar 6, 2009 at 19:24

13 Answers 13

Reset to default
24

Possible Problems:

  1. SQL Injection
  2. XSS Injection (if this code was an insert query, it would be a definite problem)
  3. Plain Text Password

Your SQL Statement can be problematic. It is bad practice to leave yourself open for SQL injection.

SQL Injection is bad. Trust me.

If you want to display the $user on an HTML page, then you may not want to include the ability for people to "hack" your layout by typing in commands like 

<H1>HI MOM</H1>

or a bunch of javascript.

Also, never store your password in plain text (good catch cagcowboy!). It gives too much power to people administering (or hacking) your database. You should never NEED to know someone's password.

Try tactics like these:

// mostly pulled from http://snippets.dzone.com/posts/show/2738 function MakeSafe($unsafestring) { $unsafestring= htmlentities($unsafestring, ENT_QUOTES); if (get_magic_quotes_gpc()) { $unsafestring= stripslashes($unsafestring); } $unsafestring= mysql_real_escape_string(trim($unsafestring)); $unsafestring= strip_tags($unsafestring); $unsafestring= str_replace("\r\n", "", $unsafestring); return $unsafestring; } // Call a function to make sure the variables you are // pulling in are not able to inject sql into your // sql statement causing massive doom and destruction. $name = MakeSafe( $_POST["user"] ); $pwd = MakeSafe( $_POST["pwd"] ); // As suggested by cagcowboy: // You should NEVER store passwords decrypted. // Ever. // sha1 creates a hash of your password // pack helps to shrink your hash // base64_encode turns it into base64 $pwd = base64_encode(pack("H*",sha1($pwd)))
Improve this answer
Sign up to request clarification or add additional context in comments.

14 Comments

Sorry, but htmlentities() is just wrong. To 'undo' the magic_quotes_gpc-effect you have to use stripslashes() and still use mysql_real_escape_string() after that.
Also only escape for html entities RIGHT before output. Dont escape and store it in the database.
Replacing mysql_escape_string with one appropriate to your database (or mysql_real_escape_string if you're paranoid like me and want the DB to escape it rather than PHP).
What sikx said. This is very bad code. You need to call stripslashes if get_magic_quote_gpc return TRUE. And then you always call mysql_real_escape_string() after that. htmlentities is useless. You call htmlspecialchars when sending the text back to the browser.
Also, base64_encode of the output from sha1 is 33% larger than the out to sha1. You need base64_encode(pack("H*",sha1($pwd))) if you want to reduce the size. (If you only work with PHP5+ you can say base64_encode(sha1($pwd,true)), too.)
|
14

It's this dangerous: xkcd bobby tables

Improve this answer

Comments

12

SQL Injection aside, it looks like your passwords might be stored in plain text, which isn't great.

Improve this answer

1 Comment

I updated my answer to include your catch. Since it was the accepted answer I did not want other people (who may stumble on it) to not see that catch.
12

That code is very safe if you never pass $query to a SQL database.

Improve this answer

1 Comment

funny - but not an real answer for the question. try to help the asking people and dont make fun about them.
3

If one were to post 0';drop table users;-- for a name

your command would end up being 

select name, pwd form users where name='0'; drop table users; --'and pwd = '[VALUE OF PWD]'

So first it would get your data, then kill your users table, and do nothing with the rest since it is a comment.

Certain mysql commands in php will perform multiple queries when passed sql, the best way to avoid this is parametrized queries.

I use PDO for all my DB access, and highly recommend it. I do not have any links off the top of my head but I remember the tutorials I used topped Google.

Improve this answer

2 Comments

mysql does not process multiple sql statements. It happens to be immune that particular SQL injection. In mysql, the one to look out for is $pwd = "x' OR '1'='1" which causes the password lookup to always succeed.
Oh good to know. I had thought that the early mysql commands allowed multiple statements. I stand corrected.
3

It is not only prone to SQL injections, it will also fail in cases where an injection is not even intended:

For example a user wants the name "Guillaume François Antoine, Marquis de L’Hospital". Since the username contains a quote and you are not escaping it, your query will fail, although the user never wanted to break the system!

Either use PDO or do it in this way:

$query = sprintf( "SELECT 1 FROM users WHERE name = '%s' AND password = '%s'", mysql_real_escape_string($_POST['name']), mysql_real_escape_string(md5($_POST['password'])) );

Improve this answer

2 Comments

You don’t need to apply mysql_real_escape_string on a md5 value.
I know, but it is good to use escape on all runtime-defined values, since even the behaviour of md5() can change at any point and it might return a string that will break the query.
2

Believe it or not, this is safe... if magic_quotes_gpc is turned on. Which it will never be in PHP6, so fixing it prior to then is a good idea.

Improve this answer

Comments

1

  1. $_POST['user'] = "' or 1=1; --"; Anyone gets instant access to your app

  2. $_POST['user'] = "'; DROP TABLE user; --"; Kiss your (paid?) user list goodbye

  3. If you later echo $name in your output, that can result in a XSS injection attack

Improve this answer

Comments

0

:O don't do it never ever, This can cause SQLInjection attack. If for example user input somehow: ' drop table users -- as input in $username; this code will concatinate to your orginal code and will drop your table. The hackers can do more and can hack your website.

Improve this answer

Comments

0

This is typically very dangerous. It could be mitigated by database permissions in some cases.

You don't validate the input ($name and $pwd). A user could send in SQL in one or both of these fields. The SQL could delete or modify other data in your database.

Improve this answer

Comments

0

Very very dangerous. A good idea for passwords is to convert the password into a MD5 hash and store that as the user's 'password'.
1) protects the users from having their passwords stolen 2) if a user writes a malicious string they could wipe out your entry/table/database

Also you should do some basic match regex expression on the name to make sure it only uses A-Za-z0-9 and maybe a few accented characters (no special characters, *'s, <'s, >'s in particular).

Improve this answer

2 Comments

Why limit the user in choice of characters?
MD5 is weak; use SHA1 instead.
0

When user data is involed in a SQL query, always sanatize the data with mysql_real_escape_string.

Furthermore, you should store just a salted hash of the password instead of the password itself. You can use the following function to generate and check a salted hash with a random salt value:

function saltedHash($data, $hash=null) { if (is_null($hash)) { $salt = substr(md5(uniqid(rand())), 0, 8); } else { $salt = substr($hash, 0, 8); } $h = $salt.md5($salt.$data); if (!is_null($hash)) { return $h === $hash; } return $h; }

All together:

$query = 'SELECT pwd FROM users WHERE name = "'.mysql_real_escape_string($_POST['user']).'"'; $res = mysql_query($query); if (mysql_num_rows($res)) { $row = mysql_fetch_assoc($res); if (saltedHash($_POST["pwd"], $row['pwd'])) { // authentic } else { // incorrect password } } else { // incorrect username }
Improve this answer

Comments

0

Its not safe, you might want to look into something like PDO. PHP PDO

Improve this answer

Comments