2
\$\begingroup\$

I have the following code which is working fine, however I am concerned around the security of this code and considering I am relatively new to php I was wondering whether the below code is secure?

I have a standard form page which submits the data to the table (testing.php). This is accompanied by another page that acts as an edit form for rows submitted forming part of the table (edit.php). These are as follows:

testing.php

<!DOCTYPE html>
<html>
<head>

<title>Database Table Submission</title>
</head>
<body>
<div>
    <form method="POST" action="add.php">
        <label>Product type:</label><input type="text" name="type">
        <label>Description:</label><input type="text" name="description">
        <label>Price:</label><input type="text" name="price">
        <label>Expires:</label><input type="text" name="expiry">
        <input type="submit" name="add">
    </form>
</div>
<br>
<div>
    <table border="1">
        <thead>
            <th>Type</th>
            <th>Description</th>
            <th>Price</th>
            <th>Expires</th>
            <th>Updates</th>
        </thead>
        <tbody>
            <?php
                include('includes/db.php');
                $query=mysqli_query($mysqli,"select * from `products`");
                while($row=mysqli_fetch_array($query)){
                    ?>
                    <tr>
                        <td><?php echo $row['type']; ?></td>
                        <td><?php echo $row['description']; ?></td>
                        <td><?php echo $row['price']; ?></td>
                        <td><?php echo $row['expiry']; ?></td>
                        <td>
                            <a href="edit.php?id=<?php echo $row['userid']; ?>">Edit</a>
                            <a href="delete.php?id=<?php echo $row['userid']; ?>">Delete</a>
                        </td>
                    </tr>
                    <?php
                }
            ?>
        </tbody>
    </table>
</div>
</body>
</html>

<?php
include('includes/db.php');
$id=$_GET['id'];
$query=mysqli_query($mysqli,"select * from `products` where userid='$id'");
$row=mysqli_fetch_array($query);
?>

edit.php

<!DOCTYPE html>
<html>
<head>
<title>Edit</title>
</head>
<body>
<h2>Edit</h2>
<form method="POST" action="update.php?id=<?php echo $id; ?>">
    <label>Type:</label><input type="text" value="<?php echo $row['type']; ?>" name="type">
    <label>Description:</label><input type="text" value="<?php echo $row['description']; ?>" name="description">
    <label>Price:</label><input type="text" value="<?php echo $row['price']; ?>" name="price">
    <label>Expiry:</label><input type="text" value="<?php echo $row['expiry']; ?>" name="expiry">

    <input type="submit" name="submit">
    <a href="testing.php">Back</a>
</form>
</body>
</html>

3 php scripts do the work adding/updating and deleting information from the table (add.php, update.php and delete.php).

add.php

<?php
include('includes/db.php');

$type=$_POST['type'];
$description=$_POST['description'];
$price=$_POST['price'];
$expiry=$_POST['expiry'];

mysqli_query($mysqli,"insert into `products` (type, description, price, expiry) values ('$type','$description','$price','$expiry')");
header('location:testing.php');

?>

update.php

<?php
include('includes/db.php');
$id=$_GET['id'];

$type=$_POST['type'];
$description=$_POST['description'];
$price=$_POST['price'];
$expiry=$_POST['expiry'];

mysqli_query($mysqli,"update `products` set type='$type', description='$description', price='$price', expiry='$expiry' where userid='$id'");
header('location:testing.php');
?>

delete.php

<?php
$id=$_GET['id'];
include('includes/db.php');
mysqli_query($mysqli,"delete from `products` where userid='$id'");
header('location:testing.php');
?>

Any suggestions and guidance would be greatly appreciated.

Bonus: For some reason there is a significant delay in the table being updated? Not sure why this is?

\$\endgroup\$
8
  • 1
    \$\begingroup\$ On Stack Overflow it would have been closed as a duplicate for stackoverflow.com/q/60174/285587 \$\endgroup\$ Commented May 15, 2018 at 10:48
  • \$\begingroup\$ php.net/manual/en/security.database.sql-injection.php \$\endgroup\$
    – BCdotWEB
    Commented May 15, 2018 at 11:33
  • \$\begingroup\$ @YourCommonSense thanks for the link. In terms of sanitizing the above input, how would this be done? My assumption would be that this would have to be done in all files listed above for any instance where adjustment to the table is being performed? \$\endgroup\$
    – NickvR
    Commented May 15, 2018 at 13:19
  • \$\begingroup\$ @NickvR no need to write your code in the comments. write it in your editor and then run in your browser. As long as there are no variables in your queries, your code is safe. \$\endgroup\$ Commented May 15, 2018 at 13:21
  • 1
    \$\begingroup\$ Please don't edit your code after receiving answers. If you want further advice on updated code, post a new question. \$\endgroup\$
    – Josiah
    Commented May 18, 2018 at 7:21

1 Answer 1

1
\$\begingroup\$

Your code is vulnerable to both SQL injection and HTML injection. So on Stack Overflow, this question would be a duplicate of both How can I prevent SQL injection in PHP? and How to prevent XSS with HTML/PHP?

Code like

                        <td><?php echo $row['type']; ?></td>

would become something like

                        <td><?php echo htmlspecialchars($row['type'], ENT_QUOTES, 'UTF-8'); ?></td>

and code like

$id=$_GET['id'];
$query=mysqli_query($mysqli,"select * from `products` where userid='$id'");

could become something like

$id = mysqli_real_escape_string($mysqli, $_GET['id']);
$query = mysqli_query($mysqli, "select * from `products` where userid='$id'");

And yes, as you speculated in a comment, you would have to do these in each file unless you rework things so as to make them happen automatically. E.g. something like

                        <td><?php safe_echo($row['type']); ?></td>

where

function safe_echo($unknown_output) {
    $safe_output = htmlspecialchars($unknown_output, ENT_QUOTES, 'UTF-8');
    echo $safe_output;
}

For the SQL injection, a more common way to approach this is parameterized queries.

$statement = mysqli_prepare($mysqli, "SELECT * FROM `products` WHERE userid=?");
mysqli_stmt_bind_param($statement, 's', $_GET['id'];
mysqli_stmt_execute($statement);
mysqli_stmt_bind_result($statement, $type, $description, $price, $expiry);

And you would use the results like

while (mysqli_stmt_fetch($statement)) {
    echo <<<EOHTML
                <td><?php echo $type; ?></td>
                <td><?php echo $description; ?></td>
                <td><?php echo $price; ?></td>
                <td><?php echo $expiry; ?></td>
EOHTML;
}

mysqli_stmt_close($statement);

Parameterized queries don't require separate escaping. The binding process will handle that automatically.

In both cases, you still have to change every file, but you don't necessarily have to write out the solution everywhere.

\$\endgroup\$
3
  • \$\begingroup\$ thanks for the notes @mdfst13, I'll do a bit of a rework on this and see if I can improve these vulnerabilities. \$\endgroup\$
    – NickvR
    Commented May 16, 2018 at 6:43
  • \$\begingroup\$ "could become something like" is no way a solid explanation, that makes it most likely to be misunderstood. There are enough PHP users who believe in the "escape string is intended to protect from SQL injection" nonsense already, no need to add another to their number. \$\endgroup\$ Commented May 16, 2018 at 7:06
  • \$\begingroup\$ The update includes prepared statements? @YourCommonSense \$\endgroup\$
    – NickvR
    Commented May 18, 2018 at 6:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.