Jump to content

Coding help: Prevent...

Miss-Ruth

By Miss-Ruth
in PHP Coding Help

 Share

Recommended Posts

Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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); 

Link to comment
Share on other sites
requinix Prolific Member

requinix

Posted
Posted

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".

Link to comment
Share on other sites
Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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); 

Link to comment
Share on other sites
gizmola Prolific Member

gizmola

Posted
Posted

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.

Link to comment
Share on other sites
requinix Prolific Member

requinix

Posted
Posted

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.

Link to comment
Share on other sites
Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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.

Link to comment
Share on other sites
Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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); 

Link to comment
Share on other sites
gizmola Prolific Member

gizmola

Posted
Posted

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');
}

Link to comment
Share on other sites
Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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');
}

Link to comment
Share on other sites
Miss-Ruth Member

Miss-Ruth

Posted
  • Author
Posted

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;

Link to comment
Share on other sites
gizmola Prolific Member

gizmola

Posted
Posted

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.

Link to comment
Share on other sites
This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.