Jump to content
ababba2

Help sanitizing this script against mysql injection

Recommended Posts

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;
	
}

?>

Share this post


Link to post
Share on other sites

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 by ababba2

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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 by mac_gyver

Share this post


Link to post
Share on other sites

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 by ababba2

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

 

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.

Share this post


Link to post
Share on other sites

^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.

Share this post


Link to post
Share on other sites

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;

 

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.