Coding help: Prevent...

Miss-Ruth · December 6, 2010

Is this a correct approach to prevent email injection?

$to: me@mydomain.com, myPartner@mydomain.com, $emailer;

//then the rest of the stuff.

$emailCheck = $_POST["emailer"];

if (eregi("(\r|\n)", $emailCheck)) {

die("Why ?? ");

}

mail($to, $subject, "", $headers);

Miss-Ruth · December 6, 2010

Please could someone advice... I even tried this... I'm not sure if what I've done is correct/effective.

$to: "me@mydomain.com, myPartner@mydomain.com,".$emailer;
//Here goes the rest of the data
//And at the end
if (eregi("(\r|\n)", $emailer) && eregi("(\r|\n)", $name) && eregi("(\r|\n)", $compan)) {
die("Why ??");
}

function safe( $newVariable ) {
   return( str_ireplace(array( "\r", "\n", "%0a", "%0d", "Content-Type:", "bcc:","to:","cc:" ), "", $newVariable ) );
}
mail($to, $subject, "", $headers);

requinix · December 6, 2010

How is $name being used? How is $compan being used? You can't just post a couple bits of code from different parts of your script and expect us to know the full story.

1. Your condition with ereg will only show the message if someone put a \r or \n in all three of the values.

2. You have a safe() function but never use it.

3. Assuming $emailer = $_POST["emailer"], no: you won't prevent injection that way. In that situation, "injection" would be somebody putting a second address in the field, like "me@example.com, injection@email.com".

Miss-Ruth · December 6, 2010

Thanks requinix. I'm not sure how to prevent email injection. I googled and red some postings. But I'm not sure how to do it as yet. Also you asek you need to get an idea of my full script.. it goes like this. (this php only fetch the posted data from another file and sends it via email to me@mydomain.com, myPartner@mydomain.com, and $emailer). The code below is the existing code and I need to make sure it's email injection proof. Highly appreciate your help.

$to: "me@mydomain.com, myPartner@mydomain.com,".$emailer;
$sender = "myCompany";
$subject = "mySubject - $company"; 
$message = "<HTML>

</HTML>";
$rPkl = PHP_EOL;
// a random hash will be necessary to send mixed content
$separator = md5(time());

$headers  = "From: ".$sender.$rPkl;
$headers .= "MIME-Version: 1.0".$rPkl; 
$headers .= "Content-Type: multipart/mixed; boundary=\"".$separator."\"".$rPkl.$rPkl; 
$headers .= "Content-Transfer-Encoding: 7bit".$rPkl;
$headers .= "This is a MIME encoded message.".$rPkl.$rPkl;
// message
$headers .= "--".$separator.$rPkl;
$headers .= "Content-Type: text/html; charset=\"iso-8859-1\"".$rPkl;
$headers .= "Content-Transfer-Encoding: 8bit".$rPkl.$rPkl;
$headers .= $message.$rPkl.$rPkl;

if (eregi("(,)", $emailer) && eregi("(\r|\n)", $name) && eregi("(\r|\n)", $company)) {
die("Why ??");
}

function safe( $newVariable ) {
   return( str_ireplace(array( "\r", "\n", "%0a", "%0d", "Content-Type:", "bcc:","to:","cc:" ), "", $newVariable ) );
}
mail($to, $subject, "", $headers);

gizmola · December 6, 2010

So originally, the code attempted to look for tampering back matching on the inclusion of a "\r\n". If those characters were included the assumption is that there was tampering, and you had a die(). Then you made it so that there had to be a "\r\n" in all of 3 variables. That is definately not what you want. You don't want to use && (AND) but rather use || (or).

So here's my points:

1. There's no reason to use eregi for this, when you can use the much faster strpos() to try and find the characters.

2. You don't stop someone from including a list of hundreds of emails, seperated by commas.

requinix · December 6, 2010

I could post a marked-up version of your code (and I even had it ready, too) but ultimately there's a better option:

Use something like PHPMailer to send the email for you. Don't try to do it yourself.

Those people have spent the time and effort into making sure there's something safe to use. It can handle all the bad things you don't even know about, and it can do it easily too.

Miss-Ruth · December 6, 2010

Thanks both of you. I appreciate your feedback.

strpos() - How can that help?

isn't there a simpler way for me to prevent this. I'm curious because I'm in a situation facing this right now. So I'm searching for an immediate fix and I'm seeking your expertise to help me code this (which might hopefuly save my job).

Thank you.

Miss-Ruth · December 6, 2010

Well... Can someone advise me please to prevent this... Also will this prevent the Bcc?

if (eregi("(\r|\n|%0a|%0d)", $emailer) || eregi("(\r|\n|%0a|%0d)", $name) || eregi("(\r|\n|%0a|%0d)", $company)) {
die("Why ??");
}

function safe( $newVariable ) {
   return( str_ireplace(array( "\r", "\n", "%0a", "%0d", "Content-Type:", "bcc:","to:","cc:" ), "", $newVariable ) );
}
mail($to, $subject, "", $headers);

gizmola · December 6, 2010

Thanks both of you. I appreciate your feedback.

strpos() - How can that help?

isn't there a simpler way for me to prevent this. I'm curious because I'm in a situation facing this right now. So I'm searching for an immediate fix and I'm seeking your expertise to help me code this (which might hopefuly save my job).

Thank you.

eregi is simply doing regex pattern matching. You don't need a regex here, because all your code was doing was looking for the newline. Here's a simple (but untested) function that might help you figure this out.


function checkEmailField($emailField) {  
  $emailField = trim($emailField);
  
  // Add anything to this array that you consider to be bogus
  $check = array("\r\n", "\r", "\n", "\t", ",");

  foreach($check as $ch) {
      if (strpos($emailField, $ch) !== false) {
          // Bad character found
            return false;
      }
  }
  return true;
}

You would use this at the top of your script, and have code like this:


if (checkEmailField($emailer) && checkEmailField($name) && checkEmailField($company)) {
  // Passed basic checks
  $emailer = safe($emailer);
  $name = safe($name);
  $company = safe($company);
  // Do rest of your email code here

} else {
  die('Invalid Input');
}

Miss-Ruth · December 6, 2010

Thank you gizmola. I Really appreciate your help.

Miss-Ruth · December 6, 2010

I'm getting an error for safe().

Fatal error: Call to undefined function safe()

$emailer = safe($emailer);

Miss-Ruth · December 6, 2010

bounce... could someone tell me the cause for this....

KevinM1 · December 6, 2010

Is your safe() function in the same file or include-ed in the email code you're using? Your main script needs to be able to 'see' the function.

Miss-Ruth · December 6, 2010

Thanks Nightslyr. this is my full code. What do you suggest?

<?php
function checkEmailField($emailField) {  
  $emailField = trim($emailField);
  $check = array("\r\n", "\r", "\n", "\t", ",", "%0a", "%0d");
  foreach($check as $ch) {
      if (strpos($emailField, $ch) !== false) {
            return false;
      }
  }
  return true;
}
if (checkEmailField($emailer) && checkEmailField($name) && checkEmailField($company)) {
  $emailer = safe($emailer);
  $name = safe($name);
  $company = safe($company);

$to = "me@mydomain.com, myPartner@mydomian.com,".$emailer;
$sender = "myCompany";
$subject = "Sent by - $company"; 
$message = "<HTML>

</HTML>";
$rPkl = PHP_EOL;
$separator = md5(time());

$headers  = "From: ".$sender.$rPkl;
$headers .= "MIME-Version: 1.0".$rPkl; 
$headers .= "Content-Type: multipart/mixed; boundary=\"".$separator."\"".$rPkl.$rPkl; 
$headers .= "Content-Transfer-Encoding: 7bit".$rPkl;
$headers .= "This is a MIME encoded message.".$rPkl.$rPkl;

$headers .= "--".$separator.$rPkl;
$headers .= "Content-Type: text/html; charset=\"iso-8859-1\"".$rPkl;
$headers .= "Content-Transfer-Encoding: 8bit".$rPkl.$rPkl;
$headers .= $message.$rPkl.$rPkl;

mail($to, $subject, "", $headers); 
} else {
  die('Invalid Input');
}

Miss-Ruth · December 6, 2010

Can someone tell me what's wrong here and how to fix it?

Miss-Ruth · December 7, 2010

Bounce...

Any help is very much appreciated.

Miss-Ruth · December 7, 2010

Ok. I tried removing $emailField = trim($emailField) and creating another variable to define the safe input. but it doesn't seem to work. Could someone help.

<?php

function checkEmailField($emailField) {

//$emailField = trim($emailField);

$check = array("\r\n", "\r", "\n", "\t", ",", "%0a", "%0d");

foreach($check as $ch) {

if (strpos($emailField, $ch) !== false) {

return false;

}

return true;

}

if (checkEmailField($emailer) && checkEmailField($name) && checkEmailField($company)) {

$emailer = safe($emaileR);

$name = safe($namE);

$company = safe($companY);

$to = "me@mydomain.com, myPartner@mydomian.com,".$emaileR;

gizmola · December 7, 2010

safe() was the function that you had included in your original code snippet. That function stripped out a lot of email headers that you would not want someone injecting. My code assumed that you would use that function, since you already had it in your code, even though you were not calling it. If you place that function back into your code as it was previously, things should work fine.

Just to be clear, the function I wrote is not tested. I just typed it in off the top of my head, so you should do some simple testing on it.

Sign In

Coding help: Prevent...

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information