Links generated in while loop vulnerable to SQL injections

Question

I have a database where I store data of users and devices. Every user has a list of devices. When the user logs into his account the php code below generates a list of the users devices:

$query_user="SELECT * FROM devices WHERE users_id = '".$user_id."'";

$result = mysqli_query($db, $query_user);

// generating device list and ON and OFF device links
while($row = $result->fetch_assoc()) 
{
    echo "<br><p>". $row["device"] ."</p><a href=on.php?data=" . $row["device"] . ">ON</a><br><a href=off.php?data=" . $row["device"] . ">OFF</a><br>";
}

The code also generates a ON and OFF link so the user can manipulate his devices. When the user click for example ON then the following URL is passed to the browser:

https://.../on.php?data=device23

The part of the on.php code that handles the URL data:

if(isset($_GET["data"]))
{
    $device_name = $_GET["data"];
}

$query_user="UPDATE devices SET status='ON' WHERE device = '".$device_name."'";

mysqli_query($db, $query_user);

So my problem here is that this approach is vulnerable to SQL injection, for example someone can type:

https://.../on.php?data=device17

in the browser and turn the device17 on.

My question is how the generate ON and OFF links for a list of devices and pass the data safe to the on.php and off.php files.

EDIT:

My main problem here is how to generate ON and OFF links for every users device (every user has a different number of devices). When the user clicks the ON link of for example device23 (or any other device, initially I don't know the device name, I am getting the device names from the first db query) I have to pass the device name to the on.php file so it can preform the SQL query that updates the device status.

Answer 1

I would double-check that the device belongs to the user as you make the UPDATE statement in on.php:

$query_user = "UPDATE devices SET status='ON' WHERE users_id = ? AND device = ?";

That's one problem solved. Now for a couple of other problems.

Notice I have avoided putting the PHP variables directly into the SQL query by concatenation. This is the SQL injection risk, which you identified. The way to solve that is to use query parameters.

$stmt = mysqli_prepare($db, $query_user);
if ($stmt === false) {
    error_log(mysqli_error($db));
    die("Sorry, there has been a software error");
}
$ok = mysqli_stmt_bind_param($stmt, "ss", $user_id, $device_name);
if ($ok === false) {
    error_log(mysqli_stmt_error($db));
    die("Sorry, there has been a software error");
}
$ok = mysqli_stmt_execute($stmt);
if ($ok === false) {
    error_log(mysqli_stmt_error($db));
    die("Sorry, there has been a software error");
}

It's good practice to check for errors after every call to one of the mysqli functions. They return false if there's an error, and then it's up to you to check for that, log it so you can troubleshoot later, and terminate the PHP request with a friendly error message.

A final problem is the fact that your code is updating data in a GET request. This is usually recommended to be avoided, because search engines that index your site will follow GET links on a page. If a search engine indexer can view your page of links, it will follow each of them, and thus all devices will be turned ON.

In this case, since turning on a device is limited to the devices for the logged-in user, and the search engine indexer is unlikely to have a valid user recorded in the session, it probably won't be a risk. But you should get into the habit of using POST only when you want to update data.

But you can't make a simple href link for a POST request. Therefore you would have to make an anchor tag and use Javascript to make an AJAX POST request, as mentioned in the comment from RamRaider above.

Answer 2

There is nothing stopping a user from typing such an url in their browser and doing what the link does, you are creating. That is perfectly valid (and in fact, what the browser does anyway by clicking on that link)

Beside that, yes your script is vurnurable to sql injection. You may want to consider using prepared statements to access your database (https://www.w3schools.com/PHP/php_mysql_prepared_statements.asp) and additionally implement some kind of authentication mechanism (like user/password authentication, system one-time-passwords,...)

UPDATE: The linked (duplicated) question‘s answer seems quite reasonable to prevent sql injection in php.

Answer 3

The handler of on/off-request has to make two things:

• checks whether the current request is authorized (with using $_SESSION) - is the user has login yet? Yes - is success. No - is failure.
• updates records (table devices) are linked with the current authorized user_id only.

And I suggest you start using the prepared statements for sql-statements. It will protect you from sql-inject attacks.