Is this php code safe ?

Question

Hi ,

I am aware I should be using prepared statement's but my next project will be using prepared statement's I just need to finish this simple little application.

So my question is:

Is this following snippet of code secure ?

I have used htmlentities aswell as mysql_real_escape_string because I thought it was a safe option.

//Image 
$imageInput = $_POST['Image'];
$imageClean = htmlentities($imageInput, ENT_QUOTES, 'UTF-8');



//Inserts values into relevant field and creates a new row.
mysql_query("UPDATE ***** SET image='" . mysql_real_escape_string($imageClean) . "'     WHERE id=" . mysql_real_escape_string($idClean) . "");

to add the code for the $idClean is:

//Id to change
if(ctype_digit($_POST['testimonial']))
{
    $idInput = $_POST['testmonial'];
    $idClean = htmlentities($idInput, ENT_QUTOES, 'UTF-8');
}

Thanks for your help.

p.s if you could suggest something to add that would be great.

Answer 1

You should only apply entity escaping at the point of output - there's no value in escaping the data prior to database insertion. That said, you're doing the right thing in terms of the mysql_real_escape_string.

Other than that, as @Piskvor says there's a potential issue with the idClean variable. (Is this being cast to an int for example?)

You could use the following for example:

mysql_real_escape_string(intval($idClean))

Answer 2

$idClean where come from?

another $_POST? it is supposed to be an integer, doesnt it?

dont do html sanitization on it, just $idClean = (int)$_POST['id'];... will 'force' it to be an integer, 'killing' all possible xss/sql injection (just for $idCelan i mean)

And in general speaking, there is no one best way to sanitize inputs; all depends about what the input should contain, where it will be stored and how will be used in the future.

EDIT: after your comment on middaparka answer, i suppose $idClean come from a form (an hidden input probably).

If you like to prevent even maliciuos uses of that form, i suggest you to add another hidden field with the $idclean hashed, and then in the process page check for the hash to see if someone changed the id manually (if you doesnt do it already)

This is usually an mis-design in users management, dont know if is your behavior.

Answer 3

Assuming that $_POST['Image'] is a URI, no. Someone could submit a URI using the javascript: scheme and cause other people to run arbitrary JavaScript.

Answer 4

Depends on how clean your $idClean is.

WHERE id=" . mysql_real_escape_string($idClean) . "

mysql_real_escape_string only prepends backslashes to \x00, \n, \r, \, ', " and \x1a, but it won't stop the attacker using

$idClean = "1 OR 1=1 AND POSSIBLY OTHER SQL STATEMENTS"

Instead of mysql_real_escape_string you should just convert it to an int.

Answer 5

What is the expected range of characters for "Image" and the ID? While escaping input is a good idea, you should always check that the input only contains legitimate characters for the given data type. e.g.

if (preg_match("/\w(\w|\\.){0,31}/", $imageInput) && preg_match("/\d{1,12}/", $idClean)) {
// do database update
} else {
// invalid input, let the user know!
}

The above regex on $imageInput should only allow a image name with alphanumeric, underscore and full stop in it. Length of 1 to 32 characters (You would want to match this to your database definition). No other characters allowed. I did the regex off memory, forgive me if the escaping isn't correct, but the principle of using regex to sanitize against known good input is the way to go.

Database safety isn't the only aspect of security you need to consider. Think about cross site scripting (XSS). What if the user enters the image name as: javascript:alert('abc'); then when you display it back to the user it will execute in their browser!

Answer 6

As has been said before you have an issue with id=xyz. But rather than converting it to an integer in php I'd pass it as a string literal, too, for two reasons:

• You can change the type of the id field without touching that particular part of the code. An id doesn't always have to be an integer.
• The value range (esp. the maximum value) of php's integer might be smaller than the type of the field definition.

The connection charset influences the behavior of mysql_real\escape_string(). Therefore you should pass the connection resource as second parameter to mysql_real\escape_string().

Whether to use htmlentities() or not before inserting the data depends on what you want to achieve. If you ever need the data as something else than html/utf-8 you'd have to convert it back. But since this isn't exactly security related ....have it your way ;-)

$mysql = mysql_connect('..', '..', '..');
//...
$query = sprintf("
  UPDATE
    *****
  SET
    image='%s'
  WHERE
    id='%s'
  ", 
  mysql_real_escape_string($imageClean, $mysql),
  mysql_real_escape_string($id, $mysql)
);

Oh, and by the way: Obligatory hint to prepared statements: http://docs.php.net/pdo.prepared-statements