PHP newbie security question

Question

<?php

$id = intval($_GET['id']);

 $sql = mysql_query("SELECT username FROM users WHERE id = $id");
 $row = mysql_fetch_assoc($sql);

$user = htmlspecialchars($row['username']);

?>

<h1>User:<?php echo $user ?></h1>

Can you see any threats in the above code? Do I have to use htmlspecialchars on everything I output? And should i use is_numeric or intval to check so that the get is numeric?

Im just building a minimal site. Im just wondering if the above code is vurneable to sql injection, xss?

Answer 1

Generally speaking mysql_real_escape_string() is preferred but since it's a number, intval() is OK. So yes, it looks OK from a security perspective.

One thing though, on many platforms, ints are limited to 32 bits so if you want to deal in numbers larger than ~2.1 billion then it won't work. Well, it won't work how you expect anyway.

These sorts of security precautions apply to any form of user input including cookies (something many people forget).

Answer 2

Well,

You are casting the receied id to an int ; so no possible SQL injection here.
And the rest of the DB query is "hard-coded", so no problem there either.

If id was a string in DB, you'd have to use mysql_real_escape_string, but for an integer, intval is the right tool :-)

About the output, you are escaping data too (and, as you are outputting HTML, htmlspecialchars is OK) ; so no HTML/JS injection.

So, this short portion of code looks OK to me :-)

As a sidenote, if you are starting developping a new website, it is the moment or never to take a look at either mysqli (instead of mysql), and/or PDO ;-)

It would allow you to use functionnalities provided by recent versions of MySQL, like prepared statements, for instance -- which are a good way to protect yourself from SQL injection !

Answer 3

I would strongly recommend using PDO and prepared statements. While your statement above looks safe, you're going to have problems as soon as you do more complex queries.

Instead of puzzling over whether a particular query is safe, learn about prepared statements and you won't have to worry. Here is your example, re-written with PDO:

# Make a database connection
$db = new PDO('mysql:dbname=your_db;host=your_db_server', 'username',
    'password');

# The placeholder (:id) will be replaced with the actual value
$sql = 'SELECT username FROM users WHERE id=:id';

# Prepare the statement
$stmt = $db->prepare($sql);

# Now replace the placeholder (:id) with the actual value. This
# is called "binding" the value. Note that you don't have to
# convert it or escape it when you do it this way.
$stmt->bindValue(':id', $id);

# Run the query
$stmt->execute();

# Get the results
$row = $stmt->fetch();

# Clean up
$stmt->closeCursor();

# Do your stuff
$user = htmlspecialchars($row['username']);

I've added a lot of comments; it's not as much code as it looks like. When you use bindValue, you never have to worry about SQL injection.