3 
<?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?

I'm just building a minimal site. I'm just wondering if the above code is vulnerable to sql injection, xss?

Improve this question

3 Answers 3

Reset to default
9

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

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Unsigned ints will be +- 4 billion max.
PHP ints are signed. There are no unsigned ints in PHP.
5

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.

Improve this answer

2 Comments

what if it is a string? do i have to mysql_real_escape string it?
No. When you use bindValue() you do not need to escape your strings. Functions like mysql_real_escape_string were important to prevent SQL injection before PHP had a modern database API. It’s no longer needed with PDO as long as you use bindValue and don’t try to manually concatenate user input into your $sql string. In other words, do it the easy way. :-)
0

Well,

You are casting the received 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 !

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.