Stopping form to email hijackers

How can I improve this code?

[thesheep]

I have a function to validate email addresses like this:

function validateEmailAddress($email) { 
 if( !preg_match( "/^([a-zA-Z0-9])+([a-zA-Z0-9._-])*@([a-zA-Z0-9_-])+([a-zA-Z0-9._-]+)+$/", $email)) { return false; } 
 return true; 
} 

Then something to take a form input and generate a couple of emails:

if(validateEmailAddress($email)) {
$body = "Please add the following email address to your mailing list:\n$email";
mail([e-mail], 'Add to mailing list request', $body);
$body2 = "A request was made to add you to the mailing list. If you did not make that request or wish
 to be removed from our list, please send an email to [e-mail] with the subject 'remove'. Thanks.";
mail($email, 'Mailing list', $body2, 'From: [e-mail]');
 }

Spammers seem to be hijacking this and sending out spam. But I can't see how they might be doing that... is there anything obviously insecure about my form processing?

[MatthewHSE]

Since you've hard-coded everything except the user's email address, that's obviously the point of entry. Sometimes spammers will insert newlines into your form fields and use that to inject further header information. I'm not sure if that would pass your regex or not. I always use preg_replace to remove all whitespace from email fields, then do the actual validation.

[thesheep]

Well, I can't read the regex I have there. I must have nicked it from somewhere.

I'll take your advice and pass the variable through this first and see if that helps:

function makesafe($str) {
  $injections = array('/(\n+)/i', '/(\r+)/i', '/(\t+)/i', '/(%0A+)/i', '/(%0D+)/i', '/(%08+)/i', '/(%09+)/i');
  $str= preg_replace($injections,' ',$str);
  return $str;
}

[MatthewHSE]

Well, I can't read the regex I have there. I must have nicked it from somewhere.

Heh, I do that a lot too. Tools like Regex Buddy help a lot! ;)

Also, I think you could simplify your regex by using

\S

instead of that whole

$injections

array.

[rocknbil]

First, I'm curious about this.
preg_match( "/^([a-zA-Z0-9])+([a-zA-Z0-9._-])*@([a-zA-Z0-9_-])+([a-zA-Z0-9._-]+)+$/", $email)

your regexp appears to say

1. ^begins with

2. ([a-zA-Z0-9])+ one or more of only letters and numbers

3. ([a-zA-Z0-9._-])* followed by zero or more of only letters or numbers, dot, underscore or dash,

4. followed by @

5. followed by the same pattern in #3,

6. and again, followed by the same pattern in #3,

$ end of match.

It seems like you get the exact same thing with this,

preg_match( "/^([A-Z0-9\._-])+@([a-zA-Z0-9\._-])+$/i", $email)

1. ^ begins with

2. ([A-Z0-9\._-])+ One or more letters, numbers, dot, underscore, dash

3 Followed by @

4 Followed by one or more of the same chars in 2

5 $ End match

6. i case insensitive

But this allows a few other nasties to go through. BCC still matches /a-z/i. Chasing what they might be using for carriage returns is an endless maintenance task.

Mine might not be any better, but appears to work. What I do is first split any possible multiple addresses and use only the first one, silently. Then I perform several checks on it, although it's more complex overall my regexps are a bit more explicit. :-)

Then instead of a return false I return a clean address (no sense in pi**ing off legitimate customers who have problems with typos,) and a success and error string. Success to let the client know it's OKAY, we fixed the problems, error to error out.

function email_validate ($chk_add) { 
 $fails=0; 
 $err=''; 
 $multiples=$emailReturn=Array (); 
 $emailReturn[0]=$chk_add; 
 $emailReturn[1]=$emailReturn[2]=''; 
 // Silently kill multiple addresses 
 if ($multiples = preg_split("/[,;:]+/",$emailReturn[0])) { $emailReturn[0] = $multiples[0]; } 
 if (! preg_match("/\@/",$emailReturn[0])) {  
 $emailReturn[1] = "<li>The address $chk_add fails because of a missing @ sign</li>\n";  
$fails = 1;  
 } 
 if (! preg_match("/.*\@.*[.].*/",$emailReturn[0])) {  
 $emailReturn[1] .= "<li>The address $chk_add fails because it does not have the following  
 pattern: #*$!#*$!@#*$!#*$!.tld</li>\n";  
 $fails = 1;  
 } 
 if (preg_match("/[~`!#\$%^&*()+=\¦\\{}\[\]:;\"'\<\>\?\/]/",$emailReturn[0])) {  
 $emailReturn[1] .= "<li>The address $chk_add fails because it contains  
 any of the following: ~ ` ! # \$ % ^ & * ( ) + = \¦ \\ { } \[ \] : ; \" ' < > ? / </li>\n";  
 $fails = 1;  
 } 
 if ($fails == 0) { 
 // Silently remove white space 
 $emailReturn[0] = preg_replace("/[\s\n\r]+/",'',$emailReturn[0]); 
 } 
 return $emailReturn; 
} 

But there's more . . . before even cleaning the email, I log the raw, and in that process do stuff like this . . .

(note that "example.com" is one of my bad patterns. A common attack is to send email from your domain.com, which only creates problems for internal in initial testing.)

$bad_patterns = Array (
'b*cc\s*:',
'to\s*:',
'content\-type',
'\[\s*URL.*\]*',
'\[\s*LINK.*\]*',
'\%5B\s*URL.*(\%5D)*',
'\%5B\s*LINK.*(\%5D)*',
'\[\s*a\s*href.*\]*',
'\%5B\s*a\s*href.*(\%5B)*',
'\<\s*a\s*href.*\>*',
'\%3C\s*a\s*href.*(\%3E)*',
'example.com',
'viagra',
'pharm',
'male\s+enhance'
);

function log_email () { 
 global $bad_patterns; 
 $key=$value=$trap=''; 
 $spam_in = 0; 
 $ip = getenv('REMOTE_ADDR'); 
 $currDate=date("D, m-d-Y h:i:s A"); 
 $mail_log = MAIL_LOG; 
 $filemode = (filesize($mail_log) >= $max_log_size)?"w":"a"; 
 $input_content = " 
============================================ 
DATE/TIMETIME: $currDate IP: $ip 
============================================ 
 "; 
 // Look for matches on any of $bad_patterns, st $spam_in to 1 if found. 
 foreach ($_POST as $key => $value) { 
 if ($key != preg_match("/pass/i",$_POST[$key])) { $input_content .= $key . ": " . $value . "\n"; } 
 foreach ($bad_patterns as $v) { 
 if (preg_match("/$v/i",$_POST[$key])) {  
  $trap .= "SPAM: $value found in " . $key . " field.\n"; 
  $spam_in = 1; 
 } 
 } 
 $input_content .= "$key: $value\n"; 
 } 
 $input_content .= " 
Fields used for spam: 
$trap 
============================================ 
END $currDate/$ip ENTRY 
============================================ 
 "; 
 // PHP 5 only, recoded for 4+ compatibility 
 //file_put_contents($mail_log,$input_content,FILE_APPEND); 
 if (is_writable($mail_log)) { 
 if (!$file = fopen($mail_log,$filemode)) { exit_prog_error("Cannot open $mail_log in $filemode mode"); } 
 if (fwrite($file, $input_content) === FALSE) { exit_prog_error("Cannot write to $mail_log"); } 
 fclose($file); 
 } 
 else { error("Mail log is not writable"); } 
 return $spam_in; 
} 

if this routine returns 1 - exit no email.

[thesheep]

Thanks for sharing that rocknbil. Looks very useful.