2
\$\begingroup\$

I think that the following query is preventing against SQL injection, but what other measures do I need to take to ensure my queries are 100% safe from any malicious attacks?

$statement = $db->prepare(
    "INSERT INTO blogs (blogtitle, blogdesc, coverimage, userID, frontpage, tags) 
    VALUES (:buildtitle, :builddesc, :buildcover, :userid, :frontpage, :addtags)"
);

if ($statement->execute(array(
    ':buildtitle' => $_POST['addbuildtitle'],
    ':builddesc' => $_POST['addbuilddesc'],
    ':buildcover' => $imagepath,
    ':userid' => $_POST['adduserid'],
    ':frontpage' => $frontpage,
    ':addtags' => $_POST['addtags']))
);
\$\endgroup\$
6
  • \$\begingroup\$ Which classes are you using (what is $db) ? \$\endgroup\$
    – SylvainD
    Sep 23, 2013 at 23:36
  • \$\begingroup\$ @Josay $db is obviously PDO lol \$\endgroup\$
    – Pinoniq
    Sep 24, 2013 at 8:34
  • \$\begingroup\$ Yeah PDO, $db is just the connection \$\endgroup\$
    – craig
    Sep 24, 2013 at 8:55
  • 1
    \$\begingroup\$ @Pinoniq Sorry for asking. My feeling was that PDO was not the only candidate (cf php.net/manual/en/mysqli-stmt.execute.php for instance). \$\endgroup\$
    – SylvainD
    Sep 24, 2013 at 9:20
  • \$\begingroup\$ @Josay if you read the doc. you can see that ->execute accepts 'void'. So this code is not mysqli ;) \$\endgroup\$
    – Pinoniq
    Sep 24, 2013 at 9:33

3 Answers 3

6
\$\begingroup\$

Though prepared statements do a pretty good job protecting your DB from injection attacks, it never hurts to perform some additional checks on the data you're receiving. For example: when a request is supposed to contain an email address, you might perform a SELECT query usign that email address, or insert it in your DB. If so, you should add this:

if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{
    exit();//redirect, optionally show error message
}

Same goes for any posted data, as one of the comments mentions: XSS attacks are one of the biggest threats on the web today. I've dealt with the matter in this answer.
The very least you could do is:

$execParam = array_map(
    'strip_tags'
    array(
        ':buildtitle' => $_POST['addbuildtitle'], 
        ':builddesc' => $_POST['addbuilddesc'], 
        ':buildcover' => $imagepath, 
        ':userid' => $_POST['adduserid'], 
        ':frontpage' => $frontpage,     
        ':addtags' => $_POST['addtags']
    )
);
$statement->execute($execParam);

Here, I've just called strip_tags on each and every single item you're about to insert, to avoid some jokester posting

<script>location.href = 'http://malware-infested-site.org';</script>;

in the addbuilddesc field... because, if you don't filter those things out, people visiting your site won't be all too happy about that... and be honest: would you?

You can take this as far as you want. Since you're obviously using a traditional *SQL DB, you might use the table schema, to determine if the POST params are of the correct type and length:

//assume blogtitle is a VARCHAR(255) field:
':buildtitle' => sprintf('%255s',$_POST['addbuildtitle'])
//and userID is an unsigned int, 11 long
':userid' => sprintf("%11d",abs((int)$_POST['adduserid']))

This ensures your :buildtitle bind will be no longer than 255 chars. The userid value will not be negative, and will be an integer of max 11 digits long.
But to implement this would mean a huuuge mess of sprintf calls, so you could use dataModels for this (check pretty much all of my DB related answers on this site for details), and use getters and setters that use this sprintf function to format the data accordingly.
still, this will mean a lot of overhead for each and every query. Sadly, the tradoff will always be one of performance vs security. If security is all you're after, you'll also note that, in my other answers, I basically advise everybody who's trying to create a solid (as in strong, but also as in S.O.L.I.D.) query system, to have a look at doctrine. No need to reinvent the wheel...
Anyway, I'm going off course...

Now, as far as injection goes, prepared statements are pretty robust, provided you don't take it to new, absurd, extremes. I've seen people use variables instead of the DB name, just like I've seen them use POST params to fill in the table name. You don't seem to do that, and that's good... great even. Just a friendly warning: if ever you consider using variables in those parts of your query that should be static (SELECT fieldnames FROM tblNames), step back and rethink your approach, because that's where you may still introduce vulnerabilities to your system.

Basically, prepared statements work like this:

  • Pass a string, from which a statement is to be prepared. This is processed by the DB server as is.
  • The DB server parses, and "prepares" the query (optimization etc...)
  • Pass the arguments, with which the statement is to be executed. These values (binds) are sent via a different protocol and here's where they're being escaped correctly.

As you can see, the binds are what is being escaped and sent completely separatly, that's why prepared statements are safe. This also explains why the actual SELECT and FROM clauses in a prepared statement may not be gotten from user input. This part of the query isn't processed in the same way the binds are...

Other niggles:

  • At no point do you seem to be checking isset($_POST['adduserid']) and other post keys. You just seem to expect they're there. That might trigger notices, and slows your code down. Just isset() all those params.
  • You have a semi-colon at the end of your if statement. If you copy-pasted this code, then that's going to give you grief.
  • Ensure, especially with INSERT's to set the PDO::ERR_MODE correctly, and insert in transactions. In case something goes wrong, you can still use rollBack!

Here's a little sample of how that would look

$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);//force PDO to throw on error
try
{
    $db->beginTransaction();//start here
    $stmt = $db->prepare($query);
    $stmt->execute($sanitizedInsertParamsArray);
    $db->commit();//make insert permament
}
catch(PDOExcpetion $e)
{
    $db->rollBack();//revert insert!
    exit($e->getMessage());
}

The code above will throw an instance of PDOException if the prepare, execute call or even the commit fails. As long as commit hasn't been called, the transaction is still pending (ie the data isn't really inserted yet).
So simply put, this snippet asks the DB: prepare this statement, then try to execute it with these values, if you did that, then save it. If any of these three steps failed, $db->rollBack() just tells the DB: OK, so it didn't work, forget about it... don't try and save half of the data, just forget everything.

Transactions are a good way to avoid orphaned data (as are FK's, btw). And they make for a safer query all together, too...

\$\endgroup\$
-1
\$\begingroup\$

100% safe is not possible; I'm sure someone will break prepared statements if they haven't already. You have already take an big step in security by using prepared statements over escaping.

One important step is to validate your POST variables. For instance let's say a valid buildtitle only consists of alpha numeric characters; it should be validated against that. Then you would wind up with something like ':buildtitle' => $buildtitle_clean,. You may already be doing this and just keeping it in the POST array, but it is important to change if you are not.

Other than that, I think it is as safe as you can make it.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Hello, and welcome to CR. Since I noticed your answer in the review section under "first posts" I thought I'd give you a heads up: this answer doesn't actually review the code. You don't go into details about how and what the OP should do... sure validate your POST variable is a valid critique, but be more specific... PS: you're not quite right when you say this code is as safe as you can make it (cf my answer) \$\endgroup\$ Oct 9, 2013 at 14:38
-1
\$\begingroup\$

Adding extra information about protecting from SQL injections.

  • You should sanitize / validate all data before replace it in queries. This can be achieved with filters ( as said ), regexes, substring, it depends on what to check.

  • Create different users in db if posible. User for Read, Write or Both. If not possible, create a wrapper that simulates users and permissions per page / per query, again your needs.

  • Create views if posible. Views hides complexities and can be another protection method.

  • I would recomend using unit testing software for example phpunit. It can help you too.

And why not. Test your web yourself. Use automated attack tools like havij, sqlmap, etc.

\$\endgroup\$

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