Is this mail() function safe from header injection

Question

I'm building a simple contact form for a website. It does not connect to a database, it just sends the email. Will this code prevent spammers from using header injections? Are there any vulnerabilities I'm not seeing?

//create short variable names
$name= filter_var($_POST['Name'],FILTER_SANITIZE_STRING);
$email= filter_var($_POST['Email'],FILTER_SANITIZE_STRING, FILTER_VALIDATE_EMAIL);
$subject= filter_var($_POST['Subject'],FILTER_SANITIZE_STRING);
$message= filter_var($_POST['Message'],FILTER_SANITIZE_STRING);

//set up some static information
$toaddress = '[e-mail],[e-mail]';

$mailcontent = "Customer name: ".$name."\n".
            "Customer email: ".$email."\n".
            "Subject: ".$subject."\n\n".
            $message;

$fromaddress = "From:" . $email;

//invoke mail() function to send mail
mail($toaddress, "Website Contact Form",$mailcontent, $fromaddress);
?>

Answer 1

IMHO, your code is not secure, as you miss \r and \n characters. filter_var() only kills those, if FILTER_SANITIZE_STRING is used in conjunction with FILTER_FLAG_STRIP_LOW, which will also filter out any characters below ASCII 32:

$message= filter_var($_POST['Message'], 
                     FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_LOW);

Also, FILTER_VALIDATE_MAIL will return a true or false, which you also do not account for. I recommend to check out this excellent source for filter_var(), as the main PHP manual is very short on information.

---

Update: As Alnitak pointed out, through the \n\n in the code, this actually doesn't matter.

Answer 2

Nope, that doesn't sanitize anything.

It would be very very easy to fudge up that mailer.

I can add anything in a post value (that you read) to manipulate the mailer.

Answer 3

Header injection relies on being able to insert additional newlines into header variables, which makes the string look like a new header.

For example, allowing a subject value of Testing\nCc: [e-mail]\n\nSome body text would result in a message header containing:

Subject: Testing
Cc: [e-mail]

Some body text

i.e. the abuser has not only added additional recipients, but they've managed to supply their own body text too.

However in your case the $toaddress is constant, and even if $toaddress had been user-supplied it should be correctly sanitised by the mail() function.

Your subject header is similarly constant

The $message variable is safe because by definition that's the body text and only sent after the real headers.

That only leaves $fromaddress, and you're already using FILTER_VALIDATE_EMAIL on that which should also reject anything with a newline in it.

However you should strictly be checking the result of that test, and aborting the whole thing if the result is FALSE. As it is if the validation fails then mail() will complain about being given a blank From: address, but there's no header injection opportunity there.

As far as I can tell, then, this code is actually secure.

---

Also, IMHO, you shouldn't send the emails from the user-supplied email address. That would fall foul of anti-spam mechanisms such as SPF.

You should use a constant From: value belonging to your own domain. If you like you could then use a correctly sanitised value in the Reply-To header to make it easier to have the subsequent reply go to the desired address.