Is this php code safe?

Question

I know that I must use a prepared statement, but my next project will use a prepared statement. I just need to finish this simple little application.

So my question is:

Is this next piece of code safe?

I used htmlentities as well 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 code for $ idClean:

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

Thanks for your help.

ps, if you could suggest adding something, that would be great.

Answer 1

Depends on how clean your $idClean .

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

mysql_real_escape_string only adds a backslash to \x00 , \n , \r , \ , ' , " and \x1a , but this will not 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 int.

Answer 2

You should only use object shielding at the output point — there is no value for shielding data before inserting the database. However, you are doing the right thing in terms of mysql_real_escape_string.

Other than that, since @Piskvor talks about a potential problem with the idClean variable. (Is this casting to int, for example?)

You can use the following, for example:

 mysql_real_escape_string(intval($idClean)) 

Answer 3

$ idClean, where did it come from?

another $ _POST? it must be an integer, right?

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

And generally speaking, there is no better way to disinfect resources; it all depends on what the input should contain , where it will be saved and how it will be used in the future.

EDIT : after your comment on middaparka's answer, I assume that $ idClean comes from the form (possibly hidden input).

If you like to prevent even maliciuos from using this form, I suggest you add another hidden field with a hash of $ idclean, and then check the hash on the process page to see if anyone has changed it manually (if you don't already)

This is usually the wrong design in user management, I do not know if your behavior.

Answer 4

Assuming $ _POST ['Image'] is a URI, no. Someone can send a URI using a javascript: scheme and make other people run arbitrary JavaScript.

Answer 5

What is the expected character range for "Image" and ID? While shielding the input is a good idea, you should always check that the input contains only valid characters for the data type. eg.

 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 an image name with alphanumeric, underscore, and full stop. Length is from 1 to 32 characters (you would like to match this with the database definition). No other characters are allowed. I made a regular expression with memory, forgive me if the escaping is wrong, but the principle of using a regular expression to disinfect from a known good input is the way to go.

Database security is not the only security aspect you need to consider. Think of 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 your browser!

Answer 6

As already mentioned, you have a problem with id=xyz . But instead of converting it to an integer in php, I would pass it as a string literal for two reasons:

• You can change the type of the id field without touching this part of the code. id does not always have to be an integer.
• The range of values ​​(especially the maximum value) of php integers may be less than the type of field definition.

connection charset affects the behavior of mysql_real \ escape_string (). Therefore, you must pass the connection resource as the second parameter to mysql_real \ escape_string ().

Whether or not to use htmlentities () before inserting data depends on what you want to achieve. If you ever need data like something else than html / utf-8, you will need to return it. But since it is not entirely related to security ... keep this in mind -)

 $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: A mandatory hint of prepared statements: http://docs.php.net/pdo.prepared-statements