2

I've been using the same php script to send emails from contact forms for years. But when my web server upgraded to php 5.3 the call to eregi was causing deprecated errors to display.

After a Google search I learned that I could use stristr instead of eregi.

When I make this simple switch everything works just fine, but I'm no php wizard so I want to know if my script is still secure from header injections.

Can someone please put my mind at ease and confirm that this script is safe (or at least safe enough) to use for sending emails from a contact form?

Here is an example of the current script with stristr in use:

    <?

$to="myemail@gmail.com";

// the $Name is the PHP variable, the _Post['Name'] should match the name of the input boxes in the form

$Name=$_POST['Name'];

$Email=$_POST['Email'];

$Phone=$_POST['Phone'];

$Message=$_POST['Message'];

// you can format the email anyway you want.

$message="Form submitted by $Name

Applicant Information:\n

Name: $Name

Email: $Email

Phone: $Phone

Message: $Message";

// Check for script HiJack

$arBadStr = array("Content-Type:", "MIME-Version:", "Content-Transfer-Encoding:", "bcc:", "cc:");

foreach($_POST as $tName => $tVal){

foreach($arBadStr as $tStr){

if(stristr($tStr, $tVal)){

$fSub = "Failed: Header Injection.";

reportError($fSub); 

}}}





if(mail($to,"mywebsite.com contact Form Submission",$message,"From: $Name <$Email>")) {

echo "Thank you $Name for your interest. We will contact you shortly";

} else {

echo "There was a problem sending the mail. Please check that you filled in the form correctly.";

}



// Report error function called when test detects hijacking. Mails report to webmaster and kills process.

function reportError($fSub) {

while(list($name, $value) = each($_POST)) {

$eBody .= "$name : $value \n\r"; }

mail( "myemail@gmail.com", $fSub, $eBody, "From: Webmaster <myemail@gmail.com>");

exit(header("Location: http://www.mywebsite.com")); }

?>

UPDATE

Based on the ever-so-gracious help from cryptic this is what my new script looks like. As you can see I've stripped out some of the header validation functions in place of simply sanitizing the input fields.

    <?
$to="myemail@gmail.com";

// the $Name is the PHP variable, the _Post['Name'] should match the name of the input boxes in the form

$Name = str_replace(array("\n", "\r"), '', $_POST['Name']);
$Email = str_replace(array("\n", "\r"), '', $_POST['Email']);
$Phone = str_replace(array("\n", "\r"), '', $_POST['Phone']);
$Message = str_replace(array("\n", "\r"), '', $_POST['Message']);


function clean_string($string) {
      $bad = array("content-type","bcc:","to:","cc:","href");
      return str_replace($bad,"",$string);
    }

$Name = clean_string($Name);
$Email = clean_string($Email);
$Phone = clean_string($Phone);
$Message = clean_string($Message);


// you can format the email anyway you want.

$message="Form submitted by $Name

Applicant Information:\n

Name: $Name

Email: $Email

Phone: $Phone

Message: $Message";


if(mail($to,"mywebsite.com contact Form Submission",$message,"From: $Name <$Email>")) {

echo "Thank you $Name for your interest. We will contact you shortly";

} else {

echo "There was a problem sending the mail. Please check that you filled in the form correctly.";

}
?>
Evster
  • 2,552
  • 2
  • 16
  • 14

1 Answers1

4

You try to blacklist headers like BCC, CC but fail to block TO, FROM.

RFC 822 on page 16 in section 4.1 it states:

This specification permits multiple occurrences of most fields. Except as noted, their interpretation is not specified here, and their use is discouraged.

So an attacker would be able to manipulate the message to add additional recipients and senders. You should really just be checking for newlines and carriage returns or just sanitize all the $_POST values by stripping out \r and \n characters.

<?php

function clean_string($string) 
{
    return str_replace(array("\n", "\r"), '', $string);
}

$to = 'myemail@gmail.com';

// the $Name is the PHP variable, the _Post['Name'] should match the name of the input boxes in the form
$Name    = clean_string($Name);
$Email   = clean_string($Email);
$Phone   = clean_string($Phone);
$Message = clean_string($Message);

// you can format the email anyway you want.
$message = "Form submitted by $Name

Applicant Information:\n

Name: $Name

Email: $Email

Phone: $Phone

Message: $Message";

if (mail($to, 'mywebsite.com contact Form Submission', $message, "From: $Name <$Email>"))
{
    echo 'Thank you ' . htmlspecialchars($Name) . ' for your interest. We will contact you shortly';
}
else
{
    echo "There was a problem sending the mail. Please check that you filled in the form correctly.";
}

?>
kittycat
  • 14,983
  • 9
  • 55
  • 80
  • Thanks for the quick response! I added in the code you suggested and tested my form and it seems to be working just fine. I was worried that your code was going to cause problems if the user skips lines by hitting return/enter in the textarea box of the form, but apparently the script simply strips them out. So, with this input sanitation would you consider this form "safe" to use? Sorry for a lame question like this, but I come from a primarily front-end dev background. – Evster Dec 08 '12 at 01:55
  • The main concern when making email forms is preventing users from passing \r or \n which can be used to manipulate the data where they can add additional headers which the mailer will interpret as legit. By sanitizing the user input by removing those characters you make that attack no longer possible. – kittycat Dec 08 '12 at 02:00
  • Thanks for the easy to understand explanation. What you're saying makes total sense. However when I did another test and added several instances of \n to the textarea, the form was still submitted as normal and the email I received did not have the \n stripped out. Should I be concerned? – Evster Dec 08 '12 at 02:19
  • Actually, I should note that all instances of \n were actually converted to \\n in the email I received. (Note the additional slash.) – Evster Dec 08 '12 at 02:21
  • btw dont use the while(list($name, $value) = each($_POST)) { it will bypass your checks and allow the $_POST to be sent to mail() unsafely. As for the \\n that should not be happening since we are not replacing the chars with anything else, but rather just removing them. – kittycat Dec 08 '12 at 02:28
  • Are you adding \n as text to the form? If so that is not the same thing as the \n I am referring to. \n is a newline character, the character that is invisible that makes a new line when you press ENTER. I am using PHP in above code to remove the newline not any text that is input as \n, those will have no effect on the security as they are not the same thing. – kittycat Dec 08 '12 at 02:30
  • https://en.wikipedia.org/wiki/Newline will explain what a newline is. The PHP code above when \n is placed in double quotes like "\n" it interprets it as a true newline, if its is in single quotes '\n' it will intepret it as the text \n. – kittycat Dec 08 '12 at 02:33
  • Aahhh!! Yes, I was simply adding \n right into the textarea form element and clicking submit. Invisible newline entries that are made by hitting the return/enter key are indeed stripped out. – Evster Dec 08 '12 at 02:53
  • I am going to paste the latest version of what I'm using as an edit to my original post. – Evster Dec 08 '12 at 02:54
  • OK new code as been pasted into my original post. Please let me know what you think. Many thanks for the help! – Evster Dec 08 '12 at 03:01
  • please see my edited post. You do not need to check for BCC: CC: etc fields since even if attacker put them in they won't work since the newlines and carriage returns are stripped. I converted that into a function instead for cleaning those chars. Also sanitized the $Name variable which was being echoed out to the page. That would have otherwise caused an XSS vulnerability. – kittycat Dec 08 '12 at 04:49
  • That's great! I understand what's going on now. Thanks for your patience and help. One final question before I mark as resolved. Should I also be stripping out instances of %0a and %0d as in this example: http://www.dreamincode.net/forums/topic/228389-preventing-php-mail-header-injections/? I did a test and I don't think it's necessary because those bits of code are converted to a \n, but wanted to double check. – Evster Dec 09 '12 at 01:02
  • You shouldn't have to as they are converted, but if you want you can always do: return str_replace(array("\n", "\r"), '', urldecode($string)); – kittycat Dec 09 '12 at 01:25
  • Thank you! I feel confident about deploying this script. Marking this question as answered. – Evster Dec 09 '12 at 17:58