is my php code vulnerable to sql injections

Question

a friend of mine told me that using this script to authenticate users is real risk to sql vulns

<?php
if(strlen(strstr($_SERVER['HTTP_USER_AGENT'],"-- IPB Vaidation  --")) <= 0 ){ 
die('Login Failed!, Please try again.');
}
$name = strtolower($_GET["name"]);
$password = $_GET["password"];
$digits = $_GET["digits"];
$random_number = 70; 
$sum_total2 = $digits * $random_number;
$con = mysql_connect("127.0.0.1","usernamehere","passhere");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }
mysql_select_db("database here", $con);
$sql = "SELECT * FROM ipbmembers WHERE members_seo_name='{$name}'";
$result = mysql_query($sql) or die(mysql_error());
while($row = mysql_fetch_array($result))
  {
  $trueHash = $row['members_pass_hash'];
  $salt = $row['members_pass_salt'];
  }
$hash = md5(md5($salt) . md5($password));
if($hash == $trueHash)
{
echo "Thank you for logging in";
echo ($sum_total2);
}
else
{
echo "Login Failed!, Please try again.";
echo ($sum_total2);
}  
?>

can anyone show me how its done? or what iam doing wrong?

Thanks!

yep it is you're directly injecting variables into a sql statement without even attempting to escape them. You're also using mysql_ functions which are deprecated move to mysqli or pdo and swap to using prepared statements and you'll be much safer — Dave, Dec 11 '13 at 13:00
this is a question for [CodeReview](http://codereview.stackexchange.com/) much more than for this site — avalancha, Dec 11 '13 at 13:03
Lately I've been pointing people to [bobby-tables.com](http://bobby-tables.com/), it's a wellmade site that explains the issues with SQL injection, how it's done and how it can be remedied in a large number of programming languages. — fvu, Dec 11 '13 at 13:07
Have you tried seeing what happens if you enter a name like "Thomas O'Malley"? — Mark Baker, Dec 11 '13 at 13:11
Use PDO or mysqli for security. http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php?rq=1 — user2486495, Dec 11 '13 at 13:16
If you build your SQL statements with variables from the outside, you are at risk for SQL injection. Learn to use prepared statements and placeholders. As @fvu says, http://bobby-tables.com/php gives you plenty of examples. — Andy Lester, Dec 11 '13 at 14:00
As a sidenote, single round of MD5 is not enough for password hashing. You should use bcrypt, scrypt, or pbkdf2. — 1615903, Dec 11 '13 at 14:57

score 2 · Answer 1 · answered Dec 11 '13 at 14:03

2

Yes, your code is vulnerable. Look at this statement:

$sql = "SELECT * FROM ipbmembers WHERE members_seo_name='{$name}'";

What if $name has the value of '; DROP TABLE ipbmembers; --? Then the SQL statement you build will be:

SELECT * FROM ipbmembers WHERE members_seo_name=''; DROP TABLE ipbmembers; --'

You'll do a SELECT followed by DROPping the table.

Don't build SQL from outside variables. Use placeholders and prepared statements.

answered Dec 11 '13 at 14:03

Andy Lester

91,102
13
100
152

(Quick question Andy). If that stands to happen, couldn't one just add a filter to sniff out if someone entered "DROP" and/or "TABLE" and just make it `die()` before ever reaching that stage? – Funk Forty Niner Dec 11 '13 at 14:49
1

@Fred-ii- you definately could, but you would most likely make a mistake, leaving the code vulnerable. Why do it wrong, when you can just as easily do it right - by using prepared statements. – 1615903 Dec 11 '13 at 15:00
1

I agree 100%, it was just a question that's been on my mind for a while. I always use prepared statements myself; was just curious. @user1615903 I never trust user-input. – Funk Forty Niner Dec 11 '13 at 15:01
@Fred-ii-: Sure you can sniff out DROP or TABLE, but what about INSERT or COPY or any of a million other things? What if the attacker's inserted code was a SELECT statement that required a full table scan, effectively causing a denial-of-service attack? Far better to protect against everything rather than the attacks that you think of at the moment you're writing the code. – Andy Lester Dec 11 '13 at 15:49
1

@AndyLester You have a point there, I hadn't thought of that. Thanks for added knowledge Andy. – Funk Forty Niner Dec 11 '13 at 15:54

score 0 · Answer 2 · answered Dec 11 '13 at 19:43

Your code is vulnerable. I recently made the switch to PDO from using what you are using now. Google PDO and research it a bit but here is what you would need in order to secure your SQL code using PDO:

$sql= $conn->prepare('SELECT * FROM ipbmembers WHERE members_seo_name=:name');
$sql->bindParam(':name', $name);
$sql->execute();
while($row = $sql->fetch())
{
$trueHash = $row['members_pass_hash'];
$salt = $row['members_pass_salt'];
}

That code will do the same thing you are doing but the sql statement is prepared beforehand and then the value are put in place of the placehoders whenever the statement executes using $sql->execute(); Here is a great article that explains more in detail what is happening.

Also might I suggest using $name=htmlspecialchars($name,ENT_QUOTES) $password=htmlspecialchars($password,ENT_QUOTES) and $digits=htmlspecialchars($digits,ENT_QUOTES) on your incoming variables? This will protect from users adding javascript code or other code to mess with those variables. I picked that up as well when learning about SQL Injection.

PS: add this to the php to connect to the database using PDO `$conn = new PDO("mysql:host=HOST;dbname=DBNAME", USERNAME, PASSWORD); $conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION ); ` — user2362601, Dec 11 '13 at 20:16

is my php code vulnerable to sql injections

2 Answers2