How dangerous is this PHP code?

StackOverflow https://stackoverflow.com/questions/619991

  •  03-07-2019
  •  | 
  •  

Question

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'";
Was it helpful?

Solution

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)))

OTHER TIPS

It's this dangerous: xkcd bobby tables

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

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

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.

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']))
                 );

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.

  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

: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.

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.

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).

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
}

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

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top