2

I am checking input against a database like this:

$email = $_POST['newsletter_email'];
$email = filter_var($email, FILTER_SANITIZE_EMAIL);

and then:

$is_user = $db->count_rows('users', "WHERE email='" . $email . "'");

The question is, is it good enough? Or should I also use mysql_real_escape_string?

Scott Arciszewski
  • 30,409
  • 16
  • 85
  • 198
user1227914
  • 3,166
  • 9
  • 36
  • 71
  • 3
    You should really be using [PDO](http://php.net/manual/en/ref.pdo-mysql.php) or [MySQLi](http://php.net/manual/en/book.mysqli.php). – Stuart Wagner May 25 '15 at 18:04
  • 2
    The `FILTER_SANITIZE_EMAIL`, http://php.net/manual/en/filter.filters.sanitize.php, leaves single quotes so it is not sufficient. `Remove all characters except letters, digits and !#$%&'*+-/=?^_\`{|}~@.[]` You should use parameterized queries with PDO or mysqli_. – chris85 May 25 '15 at 18:10
  • Why aim for “good enough” when it comes to security? – Martin Bean May 25 '15 at 19:53
  • 1
    `real_escape_string() != security()`, it's used to escape characters (quotes) that are used to delineate a value. Hence, it's for making a valid query, *not* for security purposes. The confusion comes in that, not having handled the data correctly, when you add it to your query's string you are introducing a problem that could be exploited to run arbitrary queries/code. Using prepared statements (PDO/MYSQLI) doesn't have this problem, because the parameters are handed to MySQL separately, which knows how to handle inserting them to make a valid query. – Jared Farrish May 25 '15 at 19:58

2 Answers2

3

There are two parts to this question.

  1. How to validate an email address.
  2. How to prevent SQL Injection.

Input Validation

$email = filter_var($_POST['email'], FILTER_VALIDATE_EMAIL);
if ($email !== false) {
    // Well, you've got yourself a valid email address!
}

Preventing SQL Injection

Don't use mysql_real_escape_string(). The proper way to prevent SQL Injection is to use Parameterized Queries. Also known as Prepared Statements.

Recommended reading:

Your code should look like this:

$stmt = $db->prepare('SELECT count(userid) FROM users WHERE email = ?');
if ($stmt->exec([ $email ])) {
    $is_user = $stmt->fetchColumn(0);
}

If that seems too cumbersome for you, you can load up the EasyDB library and make your query look like this:

$is_user = $db->cell('SELECT count(userid) FROM users WHERE email = ?', $email);

Please retire the use of input escaping and adopt prepared statements. They send the query and the parameters in separate packets, which means altering the query structure is not possible, even with Unicode hacks.

Important

You must use prepared statements correctly (i.e. NEVER concatenate user input with the query string passed to prepare()) to achieve this guaranteed level of security.

Furthermore, in PHP, some drivers will by default emulate the prepared statements. From the PHP manual:

PDO::ATTR_EMULATE_PREPARES Enables or disables emulation of prepared statements. Some drivers do not support native prepared statements or have limited support for them. Use this setting to force PDO to either always emulate prepared statements (if TRUE), or to try to use native prepared statements (if FALSE). It will always fall back to emulating the prepared statement if the driver cannot successfully prepare the current query. Requires bool.

Emulated prepared statements does not offer the same security guarantee (i.e. data/instruction separation) as true prepared statements. To be safe, disable emulated prepares.

$db = new PDO(/* etc */);
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
Community
  • 1
  • 1
Scott Arciszewski
  • 30,409
  • 16
  • 85
  • 198
  • I'm not convinced by the forcefulness of recommending parameterized queries over escaping. Both can be used incorrectly, and both are vulnerable to the attack you link to under particular configurations (obscure charsets, emulated prepares). Parametrised statements are probably a better habit, but they are no more a panacea than escaping. – IMSoP Aug 22 '15 at 18:13
  • They are provably secure: they separate the data from the instructions. Yes, PHP does the dumb thing with emulation. Turn that off. – Scott Arciszewski Aug 22 '15 at 18:25
  • 1
    They're provably secure *if used correctly* (and configured correctly, which you didn't mention in your answer). It's perfectly possible to build an SQL statement insecurely and then pass it to a prepare function; for instance, when taking a column name to sort by from the query string (for which neither parametrisation nor standard escape functions will help). It is unhelpful to give the impression that once you start using prepared queries you never need to think about SQL injection again. – IMSoP Aug 22 '15 at 19:27
0

You are "sanitizing" and not "validating" the input. Just because you used FILTER_SANITIZE_EMAIL, does not mean the input was an email address. So yes, in your example mysql_real_escape_string() would be a wise choice.

<?php

$email = filter_var($_POST['newsletter_email'], FILTER_SANITIZE_EMAIL);

// Validate the post data as an email address.
IF (filter_var($email, FILTER_VALIDATE_EMAIL)) {
  $cln_email = mysql_real_escape_string($email);
  $is_user = $db->count_rows('users', "WHERE email='" . $cln_email. "'");
}
?>
halfer
  • 18,701
  • 13
  • 79
  • 158
Brian
  • 1,025
  • 7
  • 13