2

I would like to know if my query is safe from an SQL injection.

Below is the PHP Code

$orderid = mysql_real_escape_string($orderid);
mysql_query("SELECT * FROM orders WHERE id =\"".$orderid."\"");  

Also

If the end user can change the order ID to what they wanted, what could they do here as worst case scenario?

  • If your $orderid canot be changed by end-user your request is safe. Else, additionaly, make sure that $orderid is integer (if this suggestion is valid) by simple (int)$orderid; Edit: jsut find out that you're using the DEPRECATED mysql_. Try out PDO instead! – Ignat B. Aug 03 '13 at 16:12
  • SQL strings should be enclosed in single quotes, btw. Double quotes are for identifiers if in ANSI mode. – mario Aug 03 '13 at 16:12
  • How would one make an SQL injection here if they could make the orderid what they wanted? –  Aug 03 '13 at 16:14
  • This question appears to be off-topic because it is requesting [a code review](http://codereview.stackexchange.com/) – Quentin Aug 03 '13 at 16:35
  • Rather than asking if it is safe or not, learn to use prepared statements and parametrized queries using PDO, and then you won't have to ask. See http://bobby-tables.com/php for examples. – Andy Lester Aug 03 '13 at 22:55

1 Answers1

2

Yes, the example you show is safe from SQL injection.

There is no content in $orderid that could introduce SQL injection, assuming you aren't using an ancient version of the MySQL client that had bug #8378. If you're using MySQL 4.1.20, 5.0.22, or 5.1.11 or later, you're okay.

The comments you received are about style and best practices.

  • Query parameters can be easier to use because you don't worry about backslashing your quotes and string concatenation and so on. But for this example, query parameters are not inherently more secure. Both escaping and parameterization require that you take care to use them consistently.

  • PHP's mysql extension is now officially deprecated, and will be removed in a future version of PHP. This has a connection with writing more secure code, because the mysql extension does not support parameters, whereas the other extensions mysqli and PDO do support parameters.

    Developers are encouraged to start using either the mysqli or PDO extension now, so that when that happens, you don't find yourself unable to upgrade PHP version without rewriting all your code.

    This may help you: http://phpmaster.com/migrate-from-the-mysql-extension-to-pdo/

  • Standard SQL uses single-quotes to delimit string and date literals. MySQL also supports double-quotes for this purpose, but that's dependent on the setting of SQL_MODE. If you want your code to be more stable and standard, use single-quotes for string/date literals.

    Also, it's easier for you because you don't have to backslash single quotes inside a PHP double-quoted string.

    mysql_query("SELECT * FROM orders WHERE id = '".$orderid."'");  
    
  • If your id column is an integer, it's also easy and simple to cast $orderid to an integer, which would also be just as safe and makes your code even cleaner and simpler:

    $orderid = (int) $orderid;
    mysql_query("SELECT * FROM orders WHERE id = $orderid");  
    
Bill Karwin
  • 538,548
  • 86
  • 673
  • 828