Help sanitizing this script against mysql injection

[ababba2]

Could someone please help me?

<?php

 
if (  $_POST['option'] == "com_content"
      && $_POST['view'] == "video"
      && is_numeric($_POST['id']))
{
  	// connect to the database
  	include_once("../configuration.php");
  	$cg = new JConfig;
  	$con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db);
  	if (mysqli_connect_errno()){
    	die('n/a');
	}
	
	// grab the new hit count
	$query = "SELECT  times_viewed
				  FROM  ".$cg->dbprefix."hdflv_upload
				  WHERE   `id` = " . $_POST['id'] . "
				  LIMIT 1;
	 ";	
	 
	 $new_hits = mysqli_fetch_assoc(mysqli_query($con,$query));
	 
	 
	// add new hit
	$addone = $new_hits['times_viewed']+1;
	$queryadd = "UPDATE ".$cg->dbprefix."hdflv_upload
				SET times_viewed=".$addone."
				  WHERE   `id` = " . $_POST['id'] . ";
	 ";	
	mysqli_query($con,$queryadd);

	  // close the connection to the database
	  mysqli_close($con);
	
	  echo $addone;
	
}

?>

[NotionCommotion]

Your danger is WHERE `id` = " . $_POST['id'] . "

 

Your best bet is using PDO's prepared statements.

 

If not, using something like http://php.net/manual/en/mysqli.real-escape-string.php.

[ababba2]

Could you please help me understand if now the danger is fixed?

<?php

if (  $_POST['option'] == "com_content"
      && $_POST['view'] == "video"
      && is_numeric($_POST['id']))
{
  	// connect to the database
  	include_once("../configuration.php");
  	$cg = new JConfig;
  	$con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db);
  	if (mysqli_connect_errno()){
    	die('n/a');
	}
	
	$idpos = mysqli_real_escape_string($_POST['id']);
	
	// grab the new hit count
	$query = "SELECT  times_viewed
				  FROM  ".$cg->dbprefix."hdflv_upload
				  WHERE   `id` = " . $idpos . "
				  LIMIT 1;
	 ";	
	 
	 $new_hits = mysqli_fetch_assoc(mysqli_query($con,$query));
	 
	 
	// add new hit
	$addone = $new_hits['times_viewed']+1;
	$queryadd = "UPDATE ".$cg->dbprefix."hdflv_upload
				SET times_viewed=".$addone."
				  WHERE   `id` = " . $idpos . ";
	 ";	
	mysqli_query($con,$queryadd);

	  // close the connection to the database
	  mysqli_close($con);
	
	  echo $addone;
	
}

?>

Edited December 27, 2015 by ababba2

[NotionCommotion]

You should be fine.  After looking at your original script, I see you have is_numeric($_POST['id']) in your IF statement, so technically you might not have needed to escaped it.  That being said, you are almost always better off doing so just so you don't change the code and forget.  I still think PDO is an easier to maintain solution as well.

[mac_gyver]

because this value is not a string and is not being treated as a string in the sql statement, using any escape string function on it does NOT protect against sql injection. you can inject sql using a hexadecimal value (that encodes some sql syntax) that contains no sql special characters, that the escape string function has no affect on, and mysql will happily convert the hexadecimal value back to the original encoded string.

 

this is made worse by the posted code because is_numeric() allows a hexadecimal value.

 

you need to either validate/cast each value as the CORRECT data type that it is or use prepared sql queries. if the $idpos is expected to be an integer, you must validate/cast it as ONLY an integer value.  unfortunately, using any php code that treats the value as an integer will limit the value to php's maximum integer value, which varies depending on the bit length supported on your hardware/operating system. making sure the value only contains numeric characters, see ctype_digit(), will at least limit it to an integer value, including zero.  as has already been posted, using PDO for prepared sql queries is more consistent and simpler than using msyqli_ for prepared queries.

 

also, you don't need to SELECT data in order to UPDATE it and in fact there's a race condition present where you will loose counts when there are multiple concurrent instances of your code running. you should be using one INSERT ... ON DUPLICATE KEY UPDATE ... query to  do this.

Edited December 27, 2015 by mac_gyver

[ababba2]

So adding this should be fine?

$idpos = intval(mysql_real_escape_string($_POST['id']));

and could you please code for me the last part, I didn't understood this:

also, you don't need to SELECT data in order to UPDATE it and in fact there's a race condition present where you will loose counts when there are multiple concurrent instances of your code running. you should be using one INSERT ... ON DUPLICATE KEY UPDATE ... query to  do this.

Edited December 27, 2015 by ababba2

[ababba2]

^Because I'm using Ajax.
If I remove select query then the counter begone. Even using INSERT seems to be useless

[Jacques1]

 

So adding this should be fine?

$idpos = intval(mysql_real_escape_string($_POST['id']));

 

No, it's not fine, because

• mysql_real_escape_string() is for quoted SQL string literals only (hence the name). It's completely useless for numbers.
• intval() is likely to truncate the number if it's too big to be represented by a PHP integer.

As multiple people have said multiple times already: Use a prepared statement. That's the only sensible solution.

[NotionCommotion]

^Because I'm using Ajax.

If I remove select query then the counter begone. Even using INSERT seems to be useless

 

Sorry, maybe not the answer you were looking for, but ajax has nothing to do with the database.  You are sending a request to the PHP server, the server validates and saves the data, and replies with typically HTML if a standard request, or JSON or similar if an ajax request.

[mac_gyver]

after you figure out if you are going to use prepared queries or not, forget about the INSERT ... ON DUPLICATE part of a single query that i mentioned. you already have the data inserted and an id assigned that corresponds to what is being viewed, you would just use a single UPDATE query.

 

the following code and query will both update the view count by one and retrieve and echo the updated count value - 

$query = "UPDATE ".$cg->dbprefix."hdflv_upload 
    SET times_viewed = LAST_INSERT_ID(times_viewed+1)
    WHERE id = ?"; // use a bound parameter in a prepared query for the $idpos value

// mysqli prepared query
$stmt = $con->prepare($query);
$stmt->bind_param("i", $idpos);
$stmt->execute();

$addone = mysqli_insert_id($con); // retrieve the updated times_viewed value

echo $addone;

 

[ababba2]

Thank you