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?

301_Moved_Permanently
29.4k3 gold badges49 silver badges98 bronze badges
asked May 15, 2018 at 9:54
\$\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\$ 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\$ 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\$ 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.

answered May 15, 2018 at 22:10
\$\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\$ 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\$ Commented May 18, 2018 at 6:35

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.