5
\$\begingroup\$

I want to be sure that this isn't vulnerable to SQL injection. If yes, then how can it be improved?

$pid = $_REQUEST['product_id'];
$query1 = "SELECT * FROM products WHERE id = '" 
  . $pid . "'";

$result = mysql_db_query($dbname, $query1) or die("Failed Query of " . $query1);
$thisrow=mysql_fetch_row($result);
if ($thisrow) {
  echo "The Product was found.";
}
else
{
  echo "The Product was not found.";
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Short version: never insert a user-assigned variable directly into any SQL statement. Always sanitize it first somehow. See @janos answer below. \$\endgroup\$ – mikebabcock Nov 18 '14 at 21:22
  • \$\begingroup\$ In my case near similar to this, I put a replace function to replace single quote (') to (`) then only pass into the parameter. Is this applicable to you? \$\endgroup\$ – 4 Leave Cover Nov 19 '14 at 0:52
  • 2
    \$\begingroup\$ @mikebabcock: never try to sanitize anything, just use prepared statements (properly separating code from data) and live happily ever after. Naive sanitization code just gives a false sense of security. \$\endgroup\$ – Matteo Italia Nov 19 '14 at 8:15
  • 4
    \$\begingroup\$ This code is pretty much the exact definition of "vulnerable to SQL insertion". \$\endgroup\$ – Shadur Nov 19 '14 at 8:43
  • 1
    \$\begingroup\$ @MatteoItalia: Note that prepared statements are not magic. They protect you from first class injection attacks provided the charset is set correctly. That's it. Omitting the charset, or using resultset data in secondary queries are still risk factors. \$\endgroup\$ – Elias Van Ootegem Nov 19 '14 at 9:01
17
\$\begingroup\$

On demand -> issues mentioned in comments, reviewed & posted as an answer:

The answer to your question has already been given: Yes, you have a serious vulnerability, and the first step to solving this is using the extensions that aren't deprecated, like PDO and mysqli.
Both of these extensions support the easiest, and pretty solid (not perfect, but solid) prodection from injection attacks: prepared statements. Learn to use them, learn to love them.

The rest of your code is, I'm afraid to say pretty bad. A lot of the issues are the result of your using the mysql_* extension, but there are other issues, too.
A line-by-line review pointing out issues as we go:

$pid = $_REQUEST['product_id'];

This statement can, and probably will at some point, produce notices. If the product_id parameter isn't set, you'll get an "undefined index" notice. When writing, and testing, new code, always set your error_reporting to E_STRICT | E_ALL (which, if you're running php 5.5+ will show E_DEPRECATED notices for your using the mysql_* extension). And set display_errors to true, so you can See the problems. Anyway, always check if an index exists, before using it:

$pid = isset($_REQUEST['product_id']) ? $_REQUEST['product_id'] : null;

Next - Concatenating raw values into a query is never a good idea. Your query appears to have id = '<someValue>' as a WHERE clause, which suggests that id is a string, but that wouldn't make sense. You're also assuming that the client will behave nicely, and not request a product_id value that contains single quotes (which would mess up your query!). Nor do you seem to mind negative values, or any other non-valid input for that matter. Worst of all, its laughably easy for me to send a request that'll result in a valid query that performs a full table scan (a products table can be quite large, so this would be something that can be exploited in a DOS attack):

//the product_id value:
$pid = "1' OR '1";
//the resulting query would be
$query1 = "SELECT * FROM products WHERE id = '1' OR '1'";
//which, basically amounts to:
$query1 = 'SELECT * FROM products';

It's quite obvious that this is not what you want, you don't want your code to echo The product was found when you're actually querying to see if any product exists. Especially if that query basically selects all records in your table.
So this statement needs to be reworked:

$query1 = "SELECT * FROM products WHERE id = '" 
. $pid . "'";

Given that you're using an extension that doesn't support prepared statements, the best thing you can do is actually casting the values, and using printf to insert the value of $pid in your query string.
Also: avoid SELECT * whenever possible. Select the fields you need, and name them. Be explicit.

I'd also add some basic validation, though like is_numeric, to make sure the request parameter is a valid id value. If not, the client might attempting an injection attack. Anyway, the simple fix here would be:

$queryString = sprintf(
    'SELECT * FROM products WHERE id = %d',
    (int) $pid
);

I left out the validation, that's code you'll have to write yourself first...

Onwards - Making the resource parameter optional in mysql was, IMHO, a sure fire way to breed bad practices, and error-prone code. You've fallen into this trap... don't worry, we all have at some point, we've all written the infamous or die, and used mysql_query assuming that the last active connection would be the connection we needed. But most of us have found out the hard way that this isn't always the case... So please, fix this bit, too:

$result = mysql_db_query($dbname, $query1)
    or die("Failed Query of " . $query1);

The or die is just smelly, IMHO. I don't like code that just gives up like that, wrap this snippet in a function, and replace the or die with throwing an exception, so that you can handle the error a bit cleaner than just dumping a query onto the screen (which is showing that the product_id parameter is used in this query, so it's basically sending an injection-attack-invitation).

You are also not passing a connection resource to mysql_db_query. I know it's an optional parameter, but you'd think it'd be nice to know what connection is actually being used to query this product stuff, if only to show that you actually know what DB this table is on, and to prevent bugs popping up, when you start adding multiple connections to the project.

$thisrow=mysql_fetch_row($result);

Again, in light of my aversion for or die constructs, I'd much rather see code actually bothering to check the return value of functions, like here: checking if $result is a resource, or if it's false or even true. If $result is true, your or die statement won't stop the code from executing, BTW. I know it's not very likely to happen in this case, but you never know, if, down the road, the SELECT gets replaced with an INSERT or UPDATE, the or die is going to cause you grief.

$thisrow=mysql_fetch_row($result);
if ($thisrow)

Why are you fetching a row, if you're only checking to make sure the id is found in the database. Why not use mysql_num_rows, apart from the mysql extension being deprecated, of course.

TL;TR

The best thing for you to do now is to rewrite the code entirely. Preferably using PDO or mysqli and prepared statements. It should look something along the lines of:

if (isset($_REQUEST['product_id']))
{
    $db = new PDO($dsn, $user, $pass, [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
    $stmt = $db->prepare('SELECT id FROM products WHERE id = :id');
    $stmt->execute([':id' => $_REQUEST['product_id']]);
    if ($row = $stmt->fetch(PDO::FETCH_OBJ))
        echo 'Found product with id: ', $row->id;
    else
        echo 'Product not found';
}
else
{
    echo 'Please select a product or something...';
}
\$\endgroup\$
  • \$\begingroup\$ Why are you doing $queryString = sprintf('SELECT * FROM products WHERE id = %d', (int)$pid); when taking off that implicit integer casting (the (int)part) will produce the exact same result? \$\endgroup\$ – Ismael Miguel Nov 19 '14 at 9:51
  • \$\begingroup\$ @IsmaelMiguel: For clarity, in case people don't know the format specifiers. Also, when using printf & co with various types, I tend to use casts to easily keep track of the value indexes when using %2$s and the like \$\endgroup\$ – Elias Van Ootegem Nov 19 '14 at 9:54
  • \$\begingroup\$ @EliasVanOotegem That is a valid point, but you should provide a link to the documentation of the function. (I went ahead and got it for you: php.net/manual/en/function.sprintf.php ) \$\endgroup\$ – Ismael Miguel Nov 19 '14 at 9:59
7
\$\begingroup\$

Yes, it is vulnerable. Somebody can make a request to this page using as the value of the product_id field this string:

'; DROP TABLE products;

@mikebabcock said it best in a comment:

never insert a user-assigned variable directly into any SQL statement. Always sanitize it first somehow.

There are countless articles on the web for preventing SQL injection attacks with PHP, for example this answer looks pretty good.

Using PDO, you could rewrite like this:

$pid = $_REQUEST['product_id'];
$stmt = $pdo->prepare('SELECT * FROM products WHERE id = :id');
$stmt->execute(array('id' => $pid));
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.