Protecting a database from bad data

Question

I'm just getting into SQL injection and data sanitization and seeking some advice on my script to get started. I have made this simple program which allows the user to enter their name into a form and the information gets saved into the database. There is also a button that lists all the current names in the database.

<?php
require 'db.php';

//If the "name" form was submitted, enter it into the database.
if (isset($_POST['submit'])) {
    $name = $_POST['name'];
    echo $name . ', you submitted it!';

    $con->query("INSERT INTO name (name) VALUES ('$name')");

}

//Delete name from database.
if (isset($_POST['del_submit'])) {
    $con->query("DELETE FROM name WHERE name='$del'");
    echo $del . ' deleted successfully!';   
}

?>

<form id="devvo" method="post">
    <h2>Enter your name:</h2>
    <p><input type="text" name="name" /></p>
    <p><input type="submit" value="Submit" /></p>
    <p><input type="hidden" name="submit" /></p>
</form>
<br />
<br />
<form id="list" method="post">
    <input type="submit" value="List Everyone...">
    <input type="hidden" name="list" /></p>
</form>

<?php

//If the "list" form was submitted, get all names from database and display them to the user.
if (isset($_POST['list'])) {
    $result = $con->query("SELECT * FROM name");

    while ($row = $result->fetch_assoc()) {
        $del = $row['name'];

        echo '<strong>Name:</strong> <br />';
        echo $row['name'];
        echo '<form id="del" method="post"><input type="submit" name="del_submit" value="X" /><input type="hidden" name="del" value="'.$del.'" /></form>';
        echo '<br />';
    }
}
?>

My knowledge of PHP is very limited (as you can tell from the program) but I'm definitely seeking some best practices and simple ways to improve the security of a simple program such as this. Feel free to tell me all the different ways this program could be exploited!

Answer 1

A few quick things:

• $del is never defined when you delete based on it
• Use prepared statements
  • Not technically necessary here in your script (depending on $del)
    • a good habit to have even when data is safe
    • If you remove the filter later, then the escaping will already be there
• Don't silently change user's input
  • If the user provides invalid input, tell him; don't silently change it.
  • Not a hard rule by any means, but if a user provides a name and you change it for him, that might be a bit confusing.
• Strive for valid HTML
  • Probably just meant to be a toy-esque script, but it's CodeReview, so might as well be pedantic :-).
  • You do have a stray </p> though after the list input
• It's a good habit to separate business logic and presentation
  • MVC is basically what I'm hinting at, but there's no need to go all the way
  • New data should never be fetched or generated inside of HTML
  • The best description of what I'm getting at that I've ever heard is probably (summarized):
    • If you can't reskin your site without repeating PHP code other than simple output, something is wrong.
  • A bit hard to explain if you've never seen it, so if you want, I can do a little example.
• Escape HTML
  • Any time you put a variable into html, it should be escaped with htmlspecialchars or htmlentities.
  • echo '<tr><td>' . htmlspecialchars($row['name']) . '</td>';
• Just because one input is set does not mean that another is
  • $_POST['submit'] and $_POST['name'] are not magically linked
  • A user could exploit this to cause a notice
  • Never directly access anything a user can control the existence or content of
    • Use filter_input like showerhead suggested
    • Or, if you want to do it more directly:
      • $name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
      • Note: everything comes in as either a string or an array even if it's an integer.
      • $id = (isset($_POST['id']) && is_string($_POST['id'])) ? (int) $_POST['id'] : null;'
    • (Yes, I am 100% crazy-level paranoid)

---

Edit: This is a response to the comment below

if you could elaborate on a couple of your points that would be great. Particularly: "Just because one input is set does not mean that another is $_POST['submit'] and $_POST['name'] are not magically linked A user could exploit this to cause a notice"

Consider this code:

if (isset($_POST['submit'])) {
    $name = $_POST['name'];
    ...
}

As discussed above, you should never access a key that your not sure exists. I'm assuming the logic here is, "well if the form was submitted, then the name field must exist."

That's not the case though. Consider this HTML:

<form action="http://yoursite.tld/yourpage.php" method="POST">
    <input type="submit" name="submit" value="Submit">
</form>

Now imagine that someone submits this form.

Reading $_POST['name'] will trigger an "undefined index" notice.

So, in short, what I was getting at is that there is no link between the submit and name indexes. It's completely possible for one to be set and not the other.

Would this actually ever matter? Probably not. But still, it can have bad effects like a malicious user seeing file path or trying to fill up a log file.

The code should look like this:

if (isset($_POST['submit'])) {
    $name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
    ...
}

(Or you could use filter_input.)

And: The $del issue. The script works great even though you say it isn't defined?

The first time $del appears is on line 15 ($db->query("DELETE FROM name WHERE name='$del'");). This means that it is either defined in db.php, or it's not defined.

If it's defined in db.php, it shouldn't be.

If it's not defined, that means that it's issuing a notice and being treated as null.

(There's a third, very unlikely possibility that register globals is enabled. If that abomination is enabled, immediately disable it, or if your hosting company has enabled it without you requesting it, promptly switch companies.)

When null is casted to a string, it becomes an empty string, so the query is probably running like:

$db->query("DELETE FROM name WHERE name=''");

Also, this query falls under my "Use prepared statements" point above.

Imagine if someone submits the following form:

<form action="http://yoursite.tld/page.php" method="POST">
    <input type="submit" name="del_submit" value="Delete">
    <input type="hidden" name="del" value="' OR 1">
</form>

That means that this query would be executed:

DELETE FROM name WHERE name='' OR 1

1 is always true, thus the entire table would be cleared.

Answer 2

Can't help you much here, not much of a SQL person. However, right off the bat you are going to want to validate and sanitize your data. You never want to use raw user data. That's always bad. You are doing an ok job in validating using the isset() function. But there's more to it. You should make sure you are getting the proper data type (alphanumeric, alpha only, numeric only, etc...). You should also make sure there is no sort of injection going on by sanitizing any unwanted data. If you have PHP version >= 5.2 I would recommend using the filter_input() function here as it can do both. For instance:

$name = filter_input( INPUT_POST, 'name', FILTER_SANITIZE_STRING );

I'll let everyone else help you with the SQL specific stuff :)