3
\$\begingroup\$

This is a security question.

I'm quite new to PHP and just a beginner in SQLite.

For practical, and educational reasons, I'm writing a small PHP site that will serve as one-threaded discussion (meaning, only one topic / no topics) between me and my friends.

The site will be open to the Internet and the final version will include authentication, but I haven't reached the point of authentication yet. I have, however, reached the point where I can write and read posts in a database, and I need your opinion on how secure my ways are.

This is code taken, and simplified from my prototype. It demonstrates how I read and write things with SQLite. I've removed things such as paging (dividing into pages) and posting a message, along with the sane checks for those things (i.e.: posts have a limit of 10kB).

You can see the code bellow, but in summary these are my security measures:

  • I escape strings
  • I replace HTML special characters
  • I use PDO
  • I use prepared statements
  • I make sure my queries/statements end with a ';'

My questions are:

  1. I'm using a prepared statement to write data inside the db. Can, and should I use a prepared statement to read data from the db? If yes, then can you make an example?

  2. Are these measures enough to avoid SQL injections and XSS injections?

  3. What else do you see wrong in this code? What security holes does it have that I haven't found yet?

Also, if you have a performance recommendation, that doesn't compromise security, then go right ahead :)

<?php

// script settings
error_reporting(E_ALL);
ini_set('default_charset', 'UTF-8');
date_default_timezone_set('Europe/Helsinki');

// time
$unixtime = time();
$localtime_offset = date('Z');

// our input
$message = 'hello world';

if (!isset($message)) die("Give me something to store.");

$message = SQLite3::escapeString($message);
$message = htmlspecialchars($message, ENT_XHTML);

echo "<html>\n<head>\n</head>\n<body>\n\n";

try
{
  $db = new PDO('sqlite:stackoveflow.db') or die("Can't create database.");

  $db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);

  $db->exec("CREATE TABLE IF NOT EXISTS Wall (Id INTEGER PRIMARY KEY, Message TEXT NOT NULL, Unixtime INTEGER NOT NULL, Localtime_offset INTEGER NOT NULL);");

  // write the data
  $statement = "INSERT INTO Wall (Message, Unixtime, Localtime_offset) VALUES (:Message,:Unixtime,:Localtime_offset);";  
  $query = $db->prepare($statement);
  $query->bindValue(':Message', $message, SQLITE3_TEXT);
  $query->bindValue(':Unixtime', $unixtime, SQLITE3_INTEGER);
  $query->bindValue(':Localtime_offset', $localtime_offset, SQLITE3_INTEGER);
  $query->execute() or die($db->errorInfo());

  // read the data
  $result = $db->query('SELECT * FROM Wall');
  if (!$result) die("Can't execute database query.");
  foreach($result as $row)
  {
    $localtime = date("Y-m-d H:i:s P", $row['Unixtime']);
    print "<h2>Post #" . $row['Id'] . ", on " . $localtime . "</h2>\n";
    print $row['Message'] . "\n\n";
  }

  $db = NULL;
}
catch(PDOException $exception)
{
  echo $exception->getMessage();
}

echo "\n</body>\n</html>";

?>

Update 1 : 2013-04-15

I've made some adjustments based on user88171's suggestions.

// script settings
error_reporting(E_ALL);
ini_set('default_charset', 'UTF-8');
date_default_timezone_set('Europe/Helsinki');

// time
$unixtime = time();
$localtime_offset = date('Z');
$date = date('Y-m-d H:i:s O');

// our input
$message = "hello 气 " . chr(27) . "world á, é, í, ó, ú, ü, ñ, ¿, ¡ \xEF\xBB\xBF";

// function to end the script and write a log report for the admin
function error($msg)
{
    // write the message to the error log and send an email to the admin
    echo "<p>An internal error occured. The administrator has been notified.</p>";
    exit(1);
}

echo "<html>\n<head>\n</head>\n<body>\n\n";

// show us the input before insirting into the db
$hex = bin2hex($message);
$hex = chunk_split($hex,2," ");
echo "<p><b>Our input:</b> $message</p>\n\n";
echo "<p><b>Out input in hex:</b> $hex</p>\n\n";
echo "<hr>\n\n";

if (!isset($message)) die("<p>You need to give me an input.</p>");

$message = str_replace("\r", '', $message);
$message = SQLite3::escapeString($message);
$message = htmlspecialchars($message, ENT_XHTML);

try
{
    $db = new PDO('sqlite:stackoveflow.db') or error("Can't open or create database.");

    $db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);

    $db->exec("CREATE TABLE IF NOT EXISTS Wall (Id INTEGER PRIMARY KEY, Message TEXT NOT     NULL, Unixtime INTEGER NOT NULL, Localtime_offset INTEGER NOT NULL);");

    // write the data
    $statement = "INSERT INTO Wall (Message, Unixtime, Localtime_offset) VALUES     (:Message,:Unixtime,:Localtime_offset);";    
    $query = $db->prepare($statement);
    $query->bindValue(':Message', $message, SQLITE3_TEXT);
    $query->bindValue(':Unixtime', $unixtime, SQLITE3_INTEGER);
    $query->bindValue(':Localtime_offset', $localtime_offset, SQLITE3_INTEGER);
    $query->execute() or error($db->errorInfo());

    // read the data
    $result = $db->query('SELECT * FROM Wall');
    if (!$result) error("Can't execute database query.");
    foreach($result as $row)
    {
        $localtime = date("Y-m-d H:i:s P", $row['Unixtime']);
        print "<h2>Post #" . $row['Id'] . ", on " . $localtime . "</h2>\n\n";
        print "\t<p>" . $row['Message'] . "</p>\n\n";
    }

    $db = NULL;
}
catch(PDOException $exception)
{
    error($exception->getMessage());
}

echo "</body>\n</html>";

?>                        
\$\endgroup\$
  • \$\begingroup\$ 1. Doesn't $query->bindValue() escape $message? 2. You should call htmlspecialchars() in your view code, not before you store it in the DB. \$\endgroup\$ – inanimatt Apr 28 '13 at 9:06
  • \$\begingroup\$ Oh, and you're using SQLITE3_ constants in your PDO bindValue() calls. It's only luck that this works. Use PDO::PARAM_INT and PDO::PARAM_STR in preference. \$\endgroup\$ – inanimatt Apr 28 '13 at 9:11
1
\$\begingroup\$

Your sample $message is, frankly, kind of boring. Make it use accented vowels and the Chinese glyph 气 ("life"). Put an ASCII NUL in the middle. Ensure that SQLite3::escapeString() will insert at least two escapes, and that htmlspecialchars() will change one of them. You might want to write a predicate, no_angle_brackets() or some such, that verifies the result of htmlspecialchars() is "safe", and then call the same predicate on each $message retrieved from the database. This guards against bit rot, since you might improve your checking code a month from now but neglect to update stored rows.

Unicode is significantly more complex than Latin-1. You might choose to reject any $message with characters outside a fairly conservative range. No need to accept the BOM, Combining Ring, or the Devanagari Combining Vowel Sign U, for example.

In $query = $db->prepare($statement);, you chose a gratuitously confusing name; better to call it something like $insert. Is autocommit on, so insert doesn't need a commit?

The new PDO() or die() is a very nice idiom, which we see again after the insert; I recommend reusing the or die idiom after the select.

The select should probably mention order by id.

The <h2> should probably be <div> + CSS.

Consider ripping out the catch / try. That is, think about what you're trying to accomplish with it. Would it be so bad if the exception continued on up the stack? Are (non-PDO) traces reported the way you want them? Think about ugly non-escaped characters that might sneak into the getMessage() text or the $db->errorInfo()text.

Consider writing external unit tests, which simulate "friendly" writing / reading, and "hostile" transactions. Verify that SQLite3 locking copes with "simultaneous" web transactions.

Overall, it looks fairly good, the boring $message test seems the greatest deficiency. The simple fact that you have your paranoid hat on and are carefully reviewing the code makes it unlikely that Bobby Tables will trash your site.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I'm confused by you supporting exceptions, but encouraging him to use or die. If an exception is thrown, the die won't execute. And or die is most certainly not a nice idiom. phpfreaks.com/blog/or-die-must-die \$\endgroup\$ – Corbin Apr 13 '13 at 21:18
  • \$\begingroup\$ @Corbin, granted, die() is unfortunate, trigger_error() or throw new Exception() would be better. Better still would be for new PDO() to throw an exception if it fails, so the caller needn't check for empty result. The key is to ensure that after an unexpected thing happens, we don't blithely keep on executing code in the known bad state. BTW, there seems to be little need to check for empty select * result -- the loop body will simply execute zero times. \$\endgroup\$ – user88171 Apr 14 '13 at 5:07
  • \$\begingroup\$ On a website dedicated to pedantic pursuit of the better option, presenting an inferior option as good is bad. Also, those does literally cannot run. Exceptions will pull the execution off that path. \$\endgroup\$ – Corbin Apr 14 '13 at 8:24
  • \$\begingroup\$ @user88171: The script works even with a Chinese character, an ASCII NULL, and BOM. I've posted an updated version which removes carriages and don't just or die(). You make a number of points. I want to sit on one for now: what should I allow inside the post body (the $message). Why reject the BOM, the Combining Ring, or the Devanagari Combining Vowel Sign U? The last two are valid punctuation in some languages. I don't want to limit my users in what language they'll write. As for the BOM, it doesn't seem to cause a problem. I'm not going against what you say. I just want to understand. \$\endgroup\$ – jsmith6 Apr 15 '13 at 15:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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