HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Possible security issues in email validation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
emailissuesvalidationpossiblesecurity

Problem

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 = 'someemail@some.com';//<== 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 = "someemail@some.com";//<== 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;
  }
}

?>

Solution

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('joe@example.com');
check_email('bogus');


will output:

checking 'joe@example.com': 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);

Code Snippets

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;
}
function is_valid_email($str) {
    return filter_var($str, FILTER_VALIDATE_EMAIL);
}
function check_email($email) {
    printf("checking '%s': %s\n", $email, is_valid_email($email) ? 'OK' : 'INVALID');
}
check_email('joe@example.com');
check_email('bogus');
checking 'joe@example.com': OK
checking 'bogus': INVALID
if(preg_match($inject,$str))
    {
    return true;
  }
  else
    {
    return false;
  }

Context

StackExchange Code Review Q#49282, answer score: 3

Revisions (0)

No revisions yet.