Buying $5USD PHP programming question

Can anyone answer this PHP programming question?

What is the security flaw in the following line of code and how would you fix it?
$sql = "UPDATE products SET `name` = '" . $_GET["name"] . "', `price` = '" . $_GET["price"] . "' WHERE id = " . $_GET["id"] . ";";

The first person that gets the right answer will get $5usd via paypal!

Best,
Ormus

 

http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php

You can keep the $5.

 

Well, first of all that script should be run behind a protected are. Otherwise everyone could run it. To answer your question, I would use escape string.

 

Ok, post the answer here. The first one that post the correct answer will get the money via paypal + job offer for 100x more pay.

 

$sql = "UPDATE products SET `name` = '" . mysql_real_escape_string($_GET["name"]) . "', `price` = '" . mysql_real_escape_string($_GET["price"]) . "' WHERE id = " . $_GET["id"] . ";";

 

You should use parameters instead of escaping the string. It will have better performance in most databases and is a better practice.

 

you need the parameter inside the query to make it more secure. here is to fix it :
<?php
$host = "localhost";
$db_name = "yourdb";
$username = "root";
$password = "";

try {
$con = new PDO("mysql:host={$host};dbname={$db_name}", $username, $password);
}catch(PDOException $exception){ //to handle connection error
echo "Connection error: " . $exception->getMessage();
}

$query = "UPDATE products SET name = :name , price = :sprice
WHERE id = :id";

$stmt = $con->prepare( $query );
//bind the parameters
$stmt->bindParam(':name', $_GET["name"]);
$stmt->bindParam(':sprice', $_GET["price"]);
$stmt->bindParam(':id', $_GET["id"]);


$stmt->execute();
?>

I have tested this and work

 

The question is a little convoluted without enough actual information to answer it properly.
First off what type of database are you using? MySQL, MSSQL, PostgreSQL, SQLight..
Secondly what type of connection are you using? Legacy mysql, mysqli, pdo...

Regardless of the above questions the correct answer would be to use prepared statements of some sort, the function calls used would be determined by the answer to the questions above.

 

the fact that he's getting parameters directly from $_GET is the most obvious security flaw, anyone can type the parameters in the address bar and the records will be vulnerable, regardless of escaping and prepared sql statements.

 

Yes, but we have this: SET `name` = '" . $_GET["name"] . "', `price` = '" . $_GET["price"] . "'

We set an existing value, so the $_GET["name"] should be safe and then, considering that we are talking about a price, we can accept only number as a value for the "price" variable. It should be safe enough.

 

i dont know what you mean by setting an existing value, but $_GET always gets the value from a querystring, im not even worried about the correct values for your fields right now, its this line that is most dangerous

WHERE id = $_GET["id"]

any user, can manipulate the id variable using just the address bar and update/mess any of your products, and that is the tip of the iceberg, if all your queries are constructed like that, i cant even begin to explain, how easy for it for anyone to completely mess up your website

 

creativeGenius,

I agree that these variables should not be being passed using query strings. An insert or update query should never use them, but given that I have seen it in production code many times I can't say that this would come as a surprise to me.

In a case like this you would have to assume that before this code there are at lease security checks to make sure the user has the correct privileges to update the product in this way to begin with. Other than that you still need to secure the variables to prevent injection attacks, that they are even of the correct type for those database fields or that the variables are simply not empty.

Sometimes in real world situations you just have to run with what your given and make the best of it. So even if they are passing the variables through $_GET there are still many security precautions that can be taken to prevent this from being a real security issue.

 

I truly don't understand why you open this stupid trades and waving $100 job you have.
Answering a simple question surly don't represent the quality of a coder and no good programmers will waste his time on this questions game so do yourself a favor and if you looking for good coder post the info for the $100 work and test the programmers right now you are wasting your time and the other members time (that is if you really have $100 job).

anyway i wish you good luck with your projects!