On demand -> issues mentioned in comments, reviewed & posted as an answer:
The answer to your question has already been given: Yes, you have a serious vulnerability, and the first step to solving this is using the extensions that aren't deprecated, like
Both of these extensions support the easiest, and pretty solid (not perfect, but solid) prodection from injection attacks: prepared statements. Learn to use them, learn to love them.
The rest of your code is, I'm afraid to say pretty bad. A lot of the issues are the result of your using the
mysql_* extension, but there are other issues, too.
A line-by-line review pointing out issues as we go:
$pid = $_REQUEST['product_id'];
This statement can, and probably will at some point, produce notices. If the
product_id parameter isn't set, you'll get an "undefined index" notice. When writing, and testing, new code, always set your error_reporting to
E_STRICT | E_ALL (which, if you're running php 5.5+ will show
E_DEPRECATED notices for your using the
mysql_* extension). And set
display_errors to true, so you can See the problems. Anyway, always check if an index exists, before using it:
$pid = isset($_REQUEST['product_id']) ? $_REQUEST['product_id'] : null;
Next - Concatenating raw values into a query is never a good idea. Your query appears to have
id = '<someValue>' as a WHERE clause, which suggests that
id is a string, but that wouldn't make sense. You're also assuming that the client will behave nicely, and not request a
product_id value that contains single quotes (which would mess up your query!). Nor do you seem to mind negative values, or any other non-valid input for that matter. Worst of all, its laughably easy for me to send a request that'll result in a valid query that performs a full table scan (a
products table can be quite large, so this would be something that can be exploited in a DOS attack):
//the product_id value:
$pid = "1' OR '1";
//the resulting query would be
$query1 = "SELECT * FROM products WHERE id = '1' OR '1'";
//which, basically amounts to:
$query1 = 'SELECT * FROM products';
It's quite obvious that this is not what you want, you don't want your code to echo
The product was found when you're actually querying to see if any product exists. Especially if that query basically selects all records in your table.
So this statement needs to be reworked:
$query1 = "SELECT * FROM products WHERE id = '"
. $pid . "'";
Given that you're using an extension that doesn't support prepared statements, the best thing you can do is actually casting the values, and using
printf to insert the value of
$pid in your query string.
SELECT * whenever possible. Select the fields you need, and name them. Be explicit.
I'd also add some basic validation, though like
is_numeric, to make sure the request parameter is a valid
id value. If not, the client might attempting an injection attack. Anyway, the simple fix here would be:
$queryString = sprintf(
'SELECT * FROM products WHERE id = %d',
I left out the validation, that's code you'll have to write yourself first...
Onwards - Making the resource parameter optional in
mysql was, IMHO, a sure fire way to breed bad practices, and error-prone code. You've fallen into this trap... don't worry, we all have at some point, we've all written the infamous
or die, and used
mysql_query assuming that the last active connection would be the connection we needed. But most of us have found out the hard way that this isn't always the case... So please, fix this bit, too:
$result = mysql_db_query($dbname, $query1)
or die("Failed Query of " . $query1);
or die is just smelly, IMHO. I don't like code that just gives up like that, wrap this snippet in a function, and replace the
or die with throwing an exception, so that you can handle the error a bit cleaner than just dumping a query onto the screen (which is showing that the product_id parameter is used in this query, so it's basically sending an injection-attack-invitation).
You are also not passing a connection resource to
mysql_db_query. I know it's an optional parameter, but you'd think it'd be nice to know what connection is actually being used to query this product stuff, if only to show that you actually know what DB this table is on, and to prevent bugs popping up, when you start adding multiple connections to the project.
Again, in light of my aversion for
or die constructs, I'd much rather see code actually bothering to check the return value of functions, like here: checking if
$result is a resource, or if it's
false or even
or die statement won't stop the code from executing, BTW. I know it's not very likely to happen in this case, but you never know, if, down the road, the
SELECT gets replaced with an
or die is going to cause you grief.
Why are you fetching a row, if you're only checking to make sure the id is found in the database. Why not use
mysql_num_rows, apart from the
mysql extension being deprecated, of course.
The best thing for you to do now is to rewrite the code entirely. Preferably using
mysqli and prepared statements. It should look something along the lines of:
$db = new PDO($dsn, $user, $pass, [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
$stmt = $db->prepare('SELECT id FROM products WHERE id = :id');
$stmt->execute([':id' => $_REQUEST['product_id']]);
if ($row = $stmt->fetch(PDO::FETCH_OBJ))
echo 'Found product with id: ', $row->id;
echo 'Product not found';
echo 'Please select a product or something...';