Is this PHP code safe?

Question

Just a quick question: is the following PHP code safe? Also is there anything you think I could or should add?

$post = $_GET['post']; if(is_numeric($post)) { $post = mysql_real_escape_string($post); } else { die("NAUGHTY NAUGHTY"); } mysql_select_db("****", $*****); $content = mysql_query("SELECT * FROM tbl_***** WHERE Id='" . $post . "'"); 

Answer 1

In this particular case, I think is_numeric saves you from SQL injection (although you can still split the SQL query, cf Alex 'answer). However, I really think that you should consider using parameterized queries (the so-called prepared statements), because:

• They will protect you even when using non-numeric type parameters.
• You do not risk forgetting the entry sanitation when adding additional parameters
• Your code will be much easier to write and read.

Here is an example (where $db is a PDO ):

 $stmt = $db->prepare('SELECT * FROM tbl_Persons WHERE Id = :id'); $stmt->execute(array(':id' => $_GET['post'])); $rows = $stmt->fetchAll(); 

For more information on parameterized SQL operations in PHP, see:

• Best way to stop SQL Injection in PHP

Answer 2

It's a little rude, but I don’t see anything at once that will cause you serious problems. It should be noted that hexadecimal notation is accepted within the is_numeric() range in accordance with the documentation. You can use is_int() or apply it. And for clarity, I would suggest using parameterized queries:

 $sql = sprintf("SELECT col1 FROM tbl WHERE col2 = '%s'", mysql_real_escape_string($post)); 

In this case, $post will be passed as the value of %s .

Answer 3

is_numeric will return true for hexadecimal numbers such as '0xFF'.

EDIT: To get around this, you can do something like:

 sprintf('%d', mysql_real_escape_string($post, $conn)); //If $post is not an int, it will become 0 by sprintf 

Take a look at the snippet here at php.net for more information.

Answer 4

You have the right idea, but is_numeric () may not behave the way you planned.

Try this test:

 <?php $tests = Array( "42", 1337, "1e4", "not numeric", Array(), 9.1 ); foreach($tests as $element) { if(is_numeric($element)) { echo "'{$element}' is numeric", PHP_EOL; } else { echo "'{$element}' is NOT numeric", PHP_EOL; } } ?> 

Result:

 '42' is numeric '1337' is numeric '1e4' is numeric 'not numeric' is NOT numeric 'Array' is NOT numeric '9.1' is numeric 

1e4 may not be something that your sql server understands if you are looking for what is usually called a numerical value. In terms of SQL injection, you're fine.

Answer 5

You are not passing the connection resource to mysql_real_escape_string () (but you seem to do it with mysql_select_db ()). The connection resource, among other things, saves the connection encoding setting, which may affect the behavior of real_escape_string () .
Either do not transfer the resource anywhere, nor (preferably) always transmit it, but do not make it worse than not transferring the resource, mixing both.

The "security" in my book also includes whether the code is readable, "understandable", and "straightforward." In this example, you would at least have to explain to me why you have a branch !numeric -> die in general, when you treat the identifier as a string in a SELECT query. My counterargument (as an example, may be wrong in your context): “Why bother? SELECT just won't return any record for a non-numeric identifier,” which reduces the code to

 if ( isset($_GET['post']) ) { $query = sprintf( "SELECT x,y,z FROM foo WHERE id='%s'", mysql_real_escape_string($_GET['post'], $mysqlconn) ); ... } 

This automatically fixes the problems you might encounter, because is_numeric () does not behave as you expect (as explained in other answers).

edit: And there is something to be said about using die() for frequent / early production code. This is good for test / sample code, but on a live system, you almost always want to return control to the surrounding code, and not just exit (so your application can handle the error gracefully). At the development stage, you may want to help out earlier or put more tests. In this case, see http://docs.php.net/assert .
Your example may qualify for approval. This will not be violated if the statement is deactivated, but can provide the developer with more information about why he is not working as intended (by another developer) when a non-numeric argument is passed. But you have to be careful to separate the necessary tests from the statements; they are not silver bullets.
If you feel that is_numeric () is an important test, your function (?) May return false, throw an exception or something to signal a status. But for me, an early mind () is an easy way out, a bit like a wordless possum: “I don’t know what to do now. If I play dead, probably no one will notice”; -)

Mandatory hint of prepared statements: http://docs.php.net/pdo.prepared-statements

Answer 6

I think it looks normal.

When accessing databases, I always use query binding, this avoids problems if I forget to avoid strings.