Possible security issues in email validation

Question

I need feedback on this code regarding security issues.

<?php
if(!isset($_POST['submit']))
{
    //This page should not be accessed directly. Need to submit the form.
    echo "error; you need to submit the form!";
}
$name = $_POST['name'];
$visitor_email = $_POST['email'];
$message = $_POST['message'];

//Validate first
if(empty($name)||empty($visitor_email)) 
{
    echo "Name and email are mandatory!";
    exit;
}

if(IsInjected($visitor_email))
{
    echo "Bad email value!";
    exit;
}

$email_from = '[email protected]';//<== update the email address
$email_subject = "New Form submission";
$email_body = "You have received a new message from the user $name.\n".
    "Here is the message:\n $message".

$to = "[email protected]";//<== update the email address
$headers = "From: $email_from \r\n";
$headers .= "Reply-To: $visitor_email \r\n";
//Send the email!
mail($to,$email_subject,$email_body,$headers);
//done. redirect to thank-you page.
header('Location: thank-you.html');

I'm using this IsInjected($str) function to validate email addresses:

// Function to validate against any email injection attempts
function IsInjected($str)
{
  $injections = array('(\n+)',
              '(\r+)',
              '(\t+)',
              '(%0A+)',
              '(%0D+)',
              '(%08+)',
              '(%09+)'
              );
  $inject = join('|', $injections);
  $inject = "/$inject/i";
  if(preg_match($inject,$str))
    {
    return true;
  }
  else
    {
    return false;
  }
}

?>

Answer 1

In your first validation, I think you forgot to exit, you probably meant:

if(!isset($_POST['submit']))
{
    //This page should not be accessed directly. Need to submit the form.
    echo "error; you need to submit the form!";
    exit;
}

To validate email addresses, you can use the filter_var method, like this:

function is_valid_email($str) {
    return filter_var($str, FILTER_VALIDATE_EMAIL);
}

For example:

function check_email($email) {
    printf("checking '%s': %s\n", $email, is_valid_email($email) ? 'OK' : 'INVALID');
}
check_email('[email protected]');
check_email('bogus');

will output:

checking '[email protected]': OK
checking 'bogus': INVALID

For more details and (caveats!), see the docs. Also some more examples here.

Your original IsInjected method might still be useful for other purposes, that's why I created a new method for validating emails. If you keep it I would rename it to hasInjectedChars or has_injected_chars. And these lines can be improved:

  if(preg_match($inject,$str))
    {
    return true;
  }
  else
    {
    return false;
  }

you could write simply:

return preg_match($inject, $str);

Answer 2

Just a short answer, because I've already posted a number of detailed reviews that deal with mail injection.

To validate an email address, all you really need to do is this:

if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    throw new RuntimeException('invalid email address');

Optionally sanitizing that same email address first, to remove some, possibly harmful characters:

$email = filter_var($email, FILTER_SANITIZE_EMAIL);

That said, checking the email for dangerous characters is all very well, but don't forget about the rest of the user input. I'm not going to repeat myself here, just a couple of links to previous answers that deal with mail injection in detail:

• PHP form with bot deterrent
• Security of a "contact us" form
• Critique sanitized user email PHP Script

And a fairly detailed article on this topic can be found here. It discusses libs and tools that are already out there, free for you to use. It shows their features and, more importantly, the caveats.

On your code:

if (!isset($_POST['submit']))
    echo '';

This basically checks if the form was submitted, but if it wasn't your code still is executed as if the form was posted. Don't do that. Either redirect, throw an exception or exit.
isset is indeed the best way to check if a post parameter exists, which is why it's odd to see that you don't check the other paremeters:

$name = $_POST['name'];

Should be:

$name = !isset($_POST['name']) ? null : $_POST['name'];

and so on. If you don't put that isset language construct (not a function, constructs are faster), then your code might emit notices:

$foo = array();
echo $foo['nonExistantIndex'];

Will issue a notice, because you're trying to access a non-existing index. to see the notices, set your ini file to the only correct development settings:

display_errors=1
error_reporting=E_STRICT | E_ALL

that way, any problem that exists in your code will be visible to you. This enables you to develop better code. Good practice should be the default/norm, not the goal.