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 PDO
and mysqli
.
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.
Also: avoid 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',
(int) $pid
);
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);
The 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.
$thisrow=mysql_fetch_row($result);
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 true
. If $result
is true
, your 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 INSERT
or UPDATE
, the or die
is going to cause you grief.
$thisrow=mysql_fetch_row($result);
if ($thisrow)
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.
TL;TR
The best thing for you to do now is to rewrite the code entirely. Preferably using PDO
or mysqli
and prepared statements. It should look something along the lines of:
if (isset($_REQUEST['product_id']))
{
$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;
else
echo 'Product not found';
}
else
{
echo 'Please select a product or something...';
}