Mail form: Validating and avoiding header injection problems

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

User avatar
Toneboy
Forum Contributor
Posts: 102
Joined: Wed Jul 31, 2002 5:59 am
Location: Law, Scotland.
Contact:

Mail form: Validating and avoiding header injection problems

Post 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?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post 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).
duk
Forum Contributor
Posts: 199
Joined: Wed May 19, 2004 8:45 am
Location: London

Post by duk »

if we make a check like maximum number of caracters ex: 20? we should prevent someone to inject more emails... ???
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

character length checking would generate more false positives than not. And can easily miss real ones.
duk
Forum Contributor
Posts: 199
Joined: Wed May 19, 2004 8:45 am
Location: London

Post 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 ??
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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?
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post 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.
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post by neophyte »

The end of this podcast discusses the issue at length:

http://podcast.phparch.com/podcast/audio/20060224.mp3
duk
Forum Contributor
Posts: 199
Joined: Wed May 19, 2004 8:45 am
Location: London

Post by duk »

:)
User avatar
Toneboy
Forum Contributor
Posts: 102
Joined: Wed Jul 31, 2002 5:59 am
Location: Law, Scotland.
Contact:

Post 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"); 
?>
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post 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.
User avatar
Toneboy
Forum Contributor
Posts: 102
Joined: Wed Jul 31, 2002 5:59 am
Location: Law, Scotland.
Contact:

Post 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?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post 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).
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post 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.
duk
Forum Contributor
Posts: 199
Joined: Wed May 19, 2004 8:45 am
Location: London

Post 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 ??
Post Reply