Page 1 of 2

Mail form: Validating and avoiding header injection problems

Posted: Mon Feb 27, 2006 3:14 am
by Toneboy
I've always had the simplest mail forms before, so as validating e-mail addresses and avoiding the header injection problem seems to be a big issue with my host at the minute I thought I ought to make sure I implement the solution correctly.

Current simple solution is this:

Code: Select all

<?php
$pagetitle='Feedback';
include("$DOCUMENT_ROOT/templates/header1.php");
include("$DOCUMENT_ROOT/templates/menu.php");
 
if(!empty($message)){ // only send if the form has been filled out.
  $mailTo="feedback@domain.com"; 
  $mailHeaders="From : $realname"; 
  $mailSubject="$subject"; 
  $mailBody="Sent by $realname ($email)\n\n"; 
  $mailBody.="Message : $message"; 


  mail($mailTo, $mailSubject, $mailBody, $mailHeaders);
  echo "<b>Your email has been sent</b><p>"; 
}


print ("<form method=post> 
Your name: <input type=text name=realname SIZE=40 MAXLENGTH=80><br> 
Your email: <input type=text name=email SIZE=40 MAXLENGTH=80><br>
Subject: <input type=text name=subject SIZE=40 MAXLENGTH=80><br>
Your message:<br> <textarea name=message ROWS=5 COLS=40></TEXTAREA><br> 
<input type=hidden name=sent value=1> 
<input type=submit value=Send>
<input type=reset value=Clear>
</form>");

include("$DOCUMENT_ROOT/templates/footer1.php");
?>
So according to http://www.jellyandcustard.com/2006/02/ ... ion-in-php (the link given to me by my host) these are the two pieces of code to add:

Code: Select all

<?php
/**
* Check single-line inputs:
* Returns false if text contains newline character
*/
function has_no_newlines($text)
{
   return preg_match("/(%0A|%0D|\n+|\r+)/i", $text);
}

/**
* Check multi-line inputs:
* Returns false if text contains newline followed by
* email-header specific string
*/
function has_no_emailheaders($text)
{
   return preg_match("/(%0A|%0D|\n+|\r+)(content-type:|to:|cc:|bcc:)/i", $text);
}
?>
and...

Code: Select all

<?php
if(!preg_match("/^([0-9a-zA-Z]([-.w]*[0-9a-zA-Z])*@([0-9a-zA-Z][-w]
*[0-9a-zA-Z].)+[a-zA-Z]{2,9})$/",$_POST["from"])) {
//email address is invalid
die("Invalid Email");
}
?>
So should the final implementation be this?

Code: Select all

<?php
$pagetitle='Feedback';
include("$DOCUMENT_ROOT/templates/header1.php");
include("$DOCUMENT_ROOT/templates/menu.php");

// Call procedures before going to mail form - is this right?

// Procedure 1.
/**
* Check single-line inputs:
* Returns false if text contains newline character
*/
function has_no_newlines($message)
{
   return preg_match("/(%0A|%0D|\n+|\r+)/i", $message);
}

/**
* Check multi-line inputs:
* Returns false if text contains newline followed by
* email-header specific string
*/
function has_no_emailheaders($message)
{
   return preg_match("/(%0A|%0D|\n+|\r+)(content-type:|to:|cc:|bcc:)/i", $text);
}

// Procedure 2.
if(!preg_match("/^([0-9a-zA-Z]([-.w]*[0-9a-zA-Z])*@([0-9a-zA-Z][-w]
*[0-9a-zA-Z].)+[a-zA-Z]{2,9})$/",$_POST["email"])) {
//email address is invalid
die("Invalid Email");
}
 
if(!empty($message)){ // only send if the form has been filled out.
  $mailTo="feedback@domain.com"; 
  $mailHeaders="From : $realname"; 
  $mailSubject="$subject"; 
  $mailBody="Sent by $realname ($email)\n\n"; 
  $mailBody.="Message : $message"; 


  mail($mailTo, $mailSubject, $mailBody, $mailHeaders);
  echo "<b>Your email has been sent</b><p>"; 
}


print ("<form method=post> 
Your name: <input type=text name=realname SIZE=40 MAXLENGTH=80><br> 
Your email: <input type=text name=email SIZE=40 MAXLENGTH=80><br>
Subject: <input type=text name=subject SIZE=40 MAXLENGTH=80><br>
Your message:<br> <textarea name=message ROWS=5 COLS=40></TEXTAREA><br> 
<input type=hidden name=sent value=1> 
<input type=submit value=Send>
<input type=reset value=Clear>
</form>");

include("$DOCUMENT_ROOT/templates/footer1.php");
?>
Will that work?

Posted: Mon Feb 27, 2006 3:26 am
by matthijs
You might be interested in this viewtopic.php?t=42190&highlight= thread.

The problem is not the $message, but the $mailHeaders="From : $realname"; Make sure that the headers are filtered well and cannot contain newlines and carriage returns. ctype_print() is a good and easy function for that. (see the above link).

Posted: Mon Feb 27, 2006 4:46 am
by duk
if we make a check like maximum number of caracters ex: 20? we should prevent someone to inject more emails... ???

Posted: Mon Feb 27, 2006 8:23 am
by feyd
character length checking would generate more false positives than not. And can easily miss real ones.

Posted: Mon Feb 27, 2006 8:32 am
by duk
what i understand was that the user could inject more emails in the field from and with a simple string we could sned thousands of emails correct ???

like: mail@mail;mail@mail and using cc: and bcc:

but if you do a check of characters lenght could help to not inject more emails to be sent... ??? could this be correct ??

Posted: Mon Feb 27, 2006 8:49 am
by feyd
What if I have a long email address? What if I, as the injector, use several small email addresses? See why it's fairly useless?

Posted: Mon Feb 27, 2006 9:10 am
by neophyte
The injection problem is with the 4th parameter of mail() and it's only exploitable if don't check to see if user submitted data contains \n new lines. This problem was covered in detail in the last issue of phparch. But as always a good policy to follow in dealing with user submitted data is to filter everything.

Posted: Mon Feb 27, 2006 9:11 am
by neophyte
The end of this podcast discusses the issue at length:

http://podcast.phparch.com/podcast/audio/20060224.mp3

Posted: Mon Feb 27, 2006 3:29 pm
by duk
:)

Posted: Tue Feb 28, 2006 4:06 am
by Toneboy
So if the only user input that is being put directly into the $mailHeaders is the $realname field then I should run the checks on that? Have I got the principle correct?

I'll try coding it again. Does this look right?

Code: Select all

<?php 
$pagetitle='Feedback'; 
include("$DOCUMENT_ROOT/templates/header1.php"); 
include("$DOCUMENT_ROOT/templates/menu.php"); 

// Call procedures before going to mail form - is this right? 

// Procedure 1. 
/** 
* Check single-line inputs: 
* Returns false if text contains newline character 
*/ 
function has_no_newlines($realname) 
{ 
   return preg_match("/(%0A|%0D|\n+|\r+)/i", $realname); 
} 

/** 
* Check multi-line inputs: 
* Returns false if text contains newline followed by 
* email-header specific string 
*/ 
function has_no_emailheaders($realname) 
{ 
   return preg_match("/(%0A|%0D|\n+|\r+)(content-type:|to:|cc:|bcc:)/i", $realname); 
} 

// Procedure 2. 
if(!preg_match("/^([0-9a-zA-Z]([-.w]*[0-9a-zA-Z])*@([0-9a-zA-Z][-w] 
*[0-9a-zA-Z].)+[a-zA-Z]{2,9})$/",$_POST["email"])) { 
//email address is invalid 
die("Invalid Email"); 
} 
  
if(!empty($message)){ // only send if the form has been filled out. 
  $mailTo="feedback@domain.com";  
  $mailHeaders="From : $realname";  
  $mailSubject="$subject";  
  $mailBody="Sent by $realname ($email)\n\n";  
  $mailBody.="Message : $message";  


  mail($mailTo, $mailSubject, $mailBody, $mailHeaders); 
  echo "<b>Your email has been sent</b><p>";  
} 


echo ("<form method=post>  
Your name: <input type=text name=realname SIZE=40 MAXLENGTH=80><br>  
Your email: <input type=text name=email SIZE=40 MAXLENGTH=80><br> 
Subject: <input type=text name=subject SIZE=40 MAXLENGTH=80><br> 
Your message:<br> <textarea name=message ROWS=5 COLS=40></TEXTAREA><br>  
<input type=hidden name=sent value=1>  
<input type=submit value=Send> 
<input type=reset value=Clear> 
</form>"); 

include("$DOCUMENT_ROOT/templates/footer1.php"); 
?>

Posted: Tue Feb 28, 2006 4:28 am
by matthijs
You are correct about the newlines and $realname. But in the script you forget to do anything with the function has_no_newlines.

I would choose the simpler ctype_print function. You could use it like this:

Code: Select all

if(!empty($message)  && ctype_print($realname)   ){ // only send if the form has been filled out.
From the manual:
ctype_print
Returns TRUE if every character in text will actually create output (including blanks). Returns FALSE if text contains control characters or characters that do not have any output or control function at all.

Posted: Tue Feb 28, 2006 4:40 am
by Toneboy
Thanks matthijs, that's extremely helpful.

I had already pieced this together before checking back here, using ctype_print. I've added in your suggestion for the requirements to send the message. First time I've used ctype_print, hope I've got this right.

Code: Select all

<?php  
$pagetitle='Feedback';  
include("$DOCUMENT_ROOT/templates/header1.php");  
include("$DOCUMENT_ROOT/templates/menu.php");  

// Call procedures before going to mail form - is this right?  

// Procedure 1.  
/**  
* Check single-line inputs:  
* Returns false if text contains newline character  
*/  

   if (ctype_print($realname)) {
       // Text here if entry is suitable.
   } else {
       // Text here if entry is not suitable.
   die("Goodbye Mister Spammer");
   }


/**  
* Check multi-line inputs:  
* Returns false if text contains newline followed by  
* email-header specific string  
*/  
function has_no_emailheaders($realname)  
{  
   return preg_match("/(%0A|%0D|\n+|\r+)(content-type:|to:|cc:|bcc:)/i", $realname);  
}  

// Procedure 2.  
if(!preg_match("/^([0-9a-zA-Z]([-.w]*[0-9a-zA-Z])*@([0-9a-zA-Z][-w]  
*[0-9a-zA-Z].)+[a-zA-Z]{2,9})$/",$_POST["email"])) {  
//email address is invalid  
die("Invalid Email");  
}  
   
if(!empty($message)  && ctype_print($realname)   ){ // only send if the form has been filled out. 
  $mailTo="feedback@domain.com";   
  $mailHeaders="From : $realname";   
  $mailSubject="$subject";   
  $mailBody="Sent by $realname ($email)\n\n";   
  $mailBody.="Message : $message";   


  mail($mailTo, $mailSubject, $mailBody, $mailHeaders);  
  echo "<b>Your email has been sent</b><p>";   
}  


echo ("<form method=post>   
Your name: <input type=text name=realname SIZE=40 MAXLENGTH=80><br>   
Your email: <input type=text name=email SIZE=40 MAXLENGTH=80><br>  
Subject: <input type=text name=subject SIZE=40 MAXLENGTH=80><br>  
Your message:<br> <textarea name=message ROWS=5 COLS=40></TEXTAREA><br>   
<input type=hidden name=sent value=1>   
<input type=submit value=Send>  
<input type=reset value=Clear>  
</form>");  

include("$DOCUMENT_ROOT/templates/footer1.php");  
?>
I guess using ctype_print eliminates the need for the previously used function has_no_newlines. Have I got that right? Is it a good idea to run ctype_print over $message as well?

Posted: Tue Feb 28, 2006 4:47 am
by matthijs
Yes, it eliminates the need for that hasnonewlines function. I would not run ctype_print over the message, because a) it's not unsafe for the message to contain newlines b) you might want people to be able to format their message a bit (with the newlines of course).

Posted: Tue Feb 28, 2006 4:55 am
by matthijs
Also, as a tip: i would choose some kind of system to structure your code a bit. Start with an empty clean array, and then put in the variables one by one after checking they are what you want them to be.
Something like:

Code: Select all

<?php
// declare empty clean array
$clean = array();

// first check for example the posted name
if (ctype_print($_POST['name']) )
{
    $clean[‘name’] = $_POST[‘name’];

}

// Then the email, is_valid_email would be a function to check the format of the emailadress
if (is_valid_email($_POST['email'] )
{
    $clean[‘email’] = $_POST[‘email’];
}

// etc .....
then, when it's time to process the variables further (in this case with the mail() function) you only use the clean array. That way, you are sure you never use tainted data.

Posted: Tue Feb 28, 2006 6:07 am
by duk
this email validation is done by javascript, was needed to update a script not created by me and i try to implement what i was trying to find out on this thread to see if its correct or no, ok goes a litle example...

Code: Select all

$recipientmail = $p_inquiry;
$name = $p_name;
$subject = "";//$p_subject;
$message = "Contact Phone ".$p_phone."\n\n".$p_message."\n\n";
$yourmail = $p_email;

if ((strlen($yourmail) < 30 ) && ( strlen($name) < 20 )) { //this was just what i have add to this script
mail("$recipientmail", "$subject", "$message",
     "Return-Path: $yourmail\nFrom: $name <$yourmail>\nReply-To: $yourmail\nMIME-Version: 1.0\nContent-Type: text/plain;\n\tcharset=\"windows-1252\"\n\tContent-Transfer-Encoding: 8bit\nX-Mailer: PHP/" . phpversion());

Header("Location: page.html");
} else {
  Header("Location: page.html");
}
about the email lenght 30 is just a example, im thinking in just calculate, the number of caracter before the @ and then after the @ calculate the number of caracters and then limit in if statment

but the way it is could be exploited ??