Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I made a PHP mailer script does the basic validation of fields, return errors, else submit if all is good. But it also has a honeypot field that is not required to be filled in (I'm assuming by hiding it using CSS a spambot will fill in the field anyway). If the field is not empty, it opens a text file and writes/appends the attempt on it, and it also sends an email alert of the attempt.

<?php 
//print_r($_POST);
$error['name'] ="";
$error['company']="";
$error['email'] ="";
$error['subject'] ="";
$error['message'] ="";
$error['website'] ="";
$success = "";
$thistime = time();
$current_date = date('m/d/Y/T ==> H:i:s');

if(isset($_POST['_save'])) {
    $name = stripslashes($_POST['name']);
    $email = stripslashes($_POST['email']);
    $company = stripslashes($_POST['company']);
    $message = stripslashes($_POST['message']);
    $subject = stripslashes($_POST['subject']);
    $website = stripslashes($_POST['website']);

    if (empty($name) || empty($email) || empty($subject) || empty($message) ||
            !empty($website)) {
    if (empty($name))
       $error['name'] = "Please enter your Full Name";
    if (empty($email))
       $error['email'] = "Please enter a valid Email Address";
    if (empty($company))
       $error['company'] = "Please enter Your Company Name";
    if (empty($subject))
       $error['subject'] = "Please Write a Subject";
    if (empty($message))
       $error['message'] = "Please write a message, inquiries or
               other concerns above";
    if (!empty($website)) // this is the honeypot
       $error['subject'] = "Opps looks like you're a spambot. You 
              just filled in a not required field.;
      $myFile = "botlog.txt";
      $fh = fopen($myFile, 'a') or die("can't open file");
      $stringData = "bot trapped" . " " . "-"  . " " . $website . " " . "-
                    " . " " . $current_date . "\r\n";
      fwrite($fh, $stringData);
      fclose($fh);
      $donot="donotreply@example.com";
      $headers="From: {$email}\r\nReply-To: {$donot}"; //create headers
      mail('opps@example.com',$headers,$stringData);
    }
    else { //if not empty
    stripslashes($headers);
    $headers="From: {$email}\r\nReply-To: {$email}"; //create headers
    $content="Name: ".$name."\r\n\r\nCompany: "
               .$company."\r\n\r\nSubject: ".$subject."\r\n\r\nMessage: ".$message;
    mail('opps@example.com',$subject,$content,$headers); //mails it
    $success = "Thank you! You're email has been sent.";
    #done;
    }
}
?>

Am I doing it right? Does this open any vulnerabilities? I'm open to any suggestions and improvements.

share|improve this question
    
You have some mismatched double quotes. Please recheck your code? –  200_success Feb 28 at 16:52
    
Your comment to @amon's answer says that you've deleted some "unnecessary" code to keep the fragment relatively short for your post. Especially since the deletion seems to have introduced some bugs, how do you know that the code you deleted doesn't contain security flaws? If you want to review code for security, you need to review all the code. When you delete code because "that part couldn't possibly contain a security issue" -- how do you know? If you were perfect at identifying security issues, you wouldn't be asking here! –  David Richerby Mar 1 at 11:58

6 Answers 6

up vote 12 down vote accepted

There are no security vulnerabilities per se present, as far as I can see (however, 200_success did find one rather important one). But if you're security-conscious, you would probably want to practice defensive programming, which implies careful input validation and eschewing potentially buggy constructs.

One of the potentially buggy constructs in many languages is using an if with a single statement: Instead of

if (!empty($website)) // this is the honeypot
       $error['subject'] = "Opps looks like you're a spambot. You 
              just filled in a not required field.;
      $myFile = "botlog.txt";
      $fh = fopen($myFile, 'a') or die("can't open file");
      $stringData = "bot trapped" . " " . "-"  . " " . $website . " " . "-
                    " . " " . $current_date . "\r\n";
      fwrite($fh, $stringData);
      fclose($fh);
      $donot="donotreply@whatever.com";
      $headers="From: {$email}\r\nReply-To: {$donot}"; //create headers
      mail('opps@gmail.com',$headers,$stringData);

use the (mostly) equivalent:

// this is the honeypot
if (!empty($website)) {
    $error['website'] = "Opps looks like you're a spambot. You just filled in a not required field.";
}
$myFile = "botlog.txt";
$fh = fopen($myFile, 'a') or die("can't open file");
$stringData = "bot trapped - $website -        \n"
            . "$current_date\r\n";
fwrite($fh, $stringData);
fclose($fh);
$return_address = "donotreply@whatever.com";
$headers = "From: {$email}\r\n"
         . "Reply-To: {$return_address}";
mail('opps@gmail.com', $headers, $stringData);

Oh! This means that we log any error as a “trapped bot”. Is this what you intended? Surely not.

Some other things I improved:

  • Don't contain literal newlines in your strings. Each string literal should not span multiple lines. If you need a newline inside a string, use the escaped version. If a string is too long for one line, then use one string literal per line and concatenate them.

  • Do not use excessive concatenation without any use (i.e. "bot trapped" . " " . "-" . " " . $website . " " . "- " . " " . $current_date . "\r\n"). Joining those literals makes your source code more readable.

  • You logged an website error as $error['subject'], not as $error['website']. This indicates you've never tested your code to see how it handles various kinds of errors.

  • You forgot the closing quote for the string literal "Opps looks like ... field.;. Did this code ever run?

And now regarding your anti-bot measures: A hidden honey-pot field is not a good solution:

  • A sufficiently advanced bot may very well be able to detect that it's hidden via CSS (e.g. style="display:none" is as obvious as it gets).
  • A normal user might be using a browser with limited support for CSS, and may subsequently see the honeypot field and decide to fill it out. Examples where this is likely is a visually impaired visitor using assistive technologies, or someone using a command-line browser like lynx for some reason.

Instead, use a proper captcha, either from an external provider (e.g. recaptcha), or use a very simple task for the challenge (e.g. “Check the radio button with the value three: o 1+3 o 4-1 o 6”).

share|improve this answer
    
Well said #amon. You have a keen eye. Yes I have tested it and it runs. I made an error as I deleted some unnecessary stuff like images and I deleted the end of statement as well. I didn't want to put those on here as they lengthen the codes. Sorry about that. $error['subject'] still displays the error though. So it worked. Very good pointers. Actually I like replies like yours as they take every detail into account. That's what makes it more interesting as I begin to trace the errors I did as a newbie to php. Still learning and it's been fun. Thanks. –  wolverene Feb 28 at 10:34
    
This is actually a test project trying to using honeypots. I will however use recaptcha. But my question is this. Do honeypot traps really work? Some of them say that captchas don't work anymore as there are more advanced scripts that get away with it. Furthermore, what can we do about the actual humans working in China's sweatshops sending spam for cents? I know that may have to be in a different discussion or forum altogether but.. just saying. –  wolverene Feb 28 at 10:38
    
@wolverene Food for thought: Who are you defending against? Captchas are intended as an automated Turing test, and are used to defend against bots. If you want to defend against spam, a spam filter which processes the message contents would be more useful. Also: what failures are acceptable? Is it an success when you've intercepted 50% of bots? 90%? Would your defenses still work if I write a bot specifically for your site? (Unfortunately, such questions about security are off topic on this site. You may have more luck on Information Security) –  amon Feb 28 at 10:50
    
Point taken. All thoughts digested. I appreciate all the info. Thanks. –  wolverene Mar 1 at 14:53
    
Will go fix up some things from all the opinions and see. –  wolverene Mar 1 at 14:59

Your mailer is vulnerable to a header-splitting attack! (Also known as a CRLF-injection attack.)

Basically, if one of the fields contains a CRLF, the attacker can insert any arbitrary header into the generated message. For example, if the email parameter is

victim1@example.com%0d%0aCc:%20victim2@example.net

… then you will compose a message with headers

From: victim1@example.com
Cc: victim2@example.net
Reply-To: victim1@example.com
Cc: victim2@example.net

PHP's mail() function will "helpfully" add victim2@example.net as a spam recipient. The spam will look like it came from victim1@example.com.

The simplest mitigation, I believe, is to pass all of the header fields through mb_encode_mimeheader() first.


General warning regarding string concatenation and interpolation

Any time you compose a string that will be interpreted by another computer system, be it an e-mail message, HTML page, or SQL query, assume that you are vulnerable to some kind of injection attack. Every single one of these "human-friendly" languages (as opposed to binary formats such as JPEG images) will have delimiters of special significance. Header-splitting, HTML/JavaScript injection, and SQL injection attacks all have the same root cause: careless string concatenation or interpolation.

Therefore, before concatenating or interpolating strings, stop and think: "What is the appropriate escaping mechanism that I should be using?" Better yet, ask: "Is there a higher-level library that will let me accomplish the task without low-level string manipulation?"


RFC time!

I'm diving into a deeper technical discussion about why mb_encode_mimeheader() is my choice for escaping. If you get confused by this technical discussion, you can safely ignore this explanation and just filter all the input through mb_encode_mimeheader().

There are actually two relevant standards for e-mail headers. Mail Transfer Agents (MTAs) care about RFC 2822. Mail User Agents (MUAs) care about RFC 2047.

MTAs are completely oblivious to MIME encoding. As long as they see the headers they need (e.g. "From", "To", "Return-Path"), and the headers contain a safe subset of ASCII, they are happy.

MUAs, on the other hand, are more sophisticated, in that they have to handle non-ASCII text for the benefit of non-English users. For example, a "Subject" header could contain arbitrary text, such as "¡Hóla!". Such a Subject line should be encoded using

mb_encode_mimeheader("¡Hóla!", "UTF-8", "Q");

as

=?UTF-8?Q?=C2=A1H=C3=B3la!?=

That is, by the way, the way to ensure that the $subject parameter is RFC 2047-compliant, as specified in the mail() documentation.

What about headers that are relevant to both MTAs and MUAs, such as the "From" header? One might want to generate a header that looks to the user like

From: "François Müller" <francois.mueller@example.com>

Over the wire, such a header must be encoded, since it contains non-ASCII characters. One acceptable encoding would be

From: "=?UTF-8?Q?Fran=C3=A7ois=20M=C3=BCller?=" <francois.mueller@example.com>

Such an encoding satisfies both RFC 2822 (since the MTA sees an opaque ASCII string between the double-quotes) and RFC 2047 (since the MUA should know how to decode the MIME-encoded string for display). To achieve such a result, you would need PHP code such as

$from_header = sprintf('From: "%s" <%s>',
                       mb_encode_mimeheader("François Müller", "UTF-8", "Q"),
                       "francois.mueller@example.com");

If you write your code as

$email = '"François Müller" <francois.mueller@example.com>';
$from_header = mb_encode_mimeheader("From: $email", "UTF-8", "Q");

… you'll get

From: =?UTF-8?Q?=22Fran=C3=A7ois=20M=C3=BCller=22=20=3Cfrancois=2Emueller?=
 =?UTF-8?Q?=40example=2Ecom=3E?=

… which is wrong, since the MTA sees one opaque ASCII string that does not look like a valid e-mail address. However, the main point is that such code, though simplistic, is not vulnerable to header injection. As long as the $email variable contains simple ASCII text, it will work. For example,

$email = '<francois.mueller@example.com>';
$from_header = mb_encode_mimeheader("From: $email", "UTF-8", "Q");

… passes the text through unchanged, producing

From: <francois.mueller@example.com>

PHP-bashing time!

As is often the case with PHP, the mail() function is poorly designed, such that it is easy to fall into this trap. Expecting programmers to know the RFCs and take the time to encode all of the headers properly is pure madness! If mail() were designed such that it accepted an associative array of (header name → header value) pairs, then it could automatically apply mitigation against header-splitting attacks.

share|improve this answer
    
jeepers, it gets to be more interesting than ever. But very keen. I don't even understand most of them but looking up more info on them would prove to be fruitful. Much appreciated. –  wolverene Mar 1 at 15:02
2  
I know, it's not an obvious bug, but it is extremely important to fix it. Think over the advice, and feel free to ask for clarification. I encourage you to post a follow-up question with your revised code to make sure you got it right. –  200_success Mar 1 at 21:01

The mail function has a return value which the code could handle:

Returns TRUE if the mail was successfully accepted for delivery, FALSE otherwise.

So, when it returns FALSE the following success message definitely is not right:

    mail('opps@gmail.com',$subject,$content,$headers); //mails it
    $success = "Thank you! You're email has been sent.";
share|improve this answer
1  
Yes, I have jumbled up too much having it make no sense at all. I'm trying to clean up the code as I go. Thank you. –  wolverene Mar 1 at 15:03

This call to stripslashes() makes no sense:

stripslashes($headers);                            # ← Calling stripslashes() on an
                                                   # undefined variable
$headers="From: {$email}\r\nReply-To: {$email}";   # ← Why bother, if you're going to
                                                   # assign to $headers immediately
                                                   # afterwards anyway?

Either you misunderstand what stripslashes() is for, or you're being careless.

I'm dissatisfied with all these calls to stripslashes() as well:

$name = stripslashes($_POST['name']);
$email = stripslashes($_POST['email']);
$company = stripslashes($_POST['company']);
$message = stripslashes($_POST['message']);
$subject = stripslashes($_POST['subject']);
$website = stripslashes($_POST['website']);

The magic_quotes_gpc debacle is a fundamental problem with PHP, and therefore deserves to be dealt with systemically rather than on an ad hoc basis. If you think that you might run this code on a server with magic quotes enabled, take this suggestion to include a preamble code block that conditionally calls stripslashes():

if (get_magic_quotes_gpc()) {
    function stripslashes_gpc(&$value)
    {
        $value = stripslashes($value);
    }
    array_walk_recursive($_GET, 'stripslashes_gpc');
    array_walk_recursive($_POST, 'stripslashes_gpc');
    array_walk_recursive($_COOKIE, 'stripslashes_gpc');
    array_walk_recursive($_REQUEST, 'stripslashes_gpc');
}

Calling stripslashes() unconditionally, as you have done, is bad practice. If your server has magic quotes disabled (as is recommended these days), then you'll mangle your input, removing backslashes that should be there. If your server has magic quotes enabled (which is considered an obsolescent practice), then reconfiguring your server to conform to modern recommendations will break your code. Either way, you lose! Keep in mind that magic quotes are deprecated as of PHP 5.3 and removed altogether as of PHP 5.4.

share|improve this answer
    
Yes, I am being careless as a newbie. Glad I shared this on here to get all the opinions from experts. Will try all the suggestions to fit. Thanks. –  wolverene Mar 1 at 14:58

You set Reply-To: to "donot@whatever.com". whatever.com is registered to a company called Dreamissary in Montebello, California. Is that you? If not, you have no business to be redirecting email to them. A user who replies to the mail, not realising that their reply will be sent to a third party may well breach confidentiality of whatever information was sent in the message so this is a security issue. It's also obnoxious to direct junk mail at a third party. You should use an invalid address at your own domain for this purpose.

share|improve this answer
    
Opps, sorry that is purely coincidental. I didn't realize there was such a company. I will edit the preview code accordingly. –  wolverene Mar 1 at 14:49
3  
@wolverene That one's registered, too. :-) The correct domain to use here is "example.com" (per RFC2606), and then use an address at your actual domain in the production code. –  David Richerby Mar 1 at 15:16

One thing you will also need to worry about is CSRF attacks

The most basic implementation to protect against that is to generate a random token everytime the form is loaded, save that into $_SESSION, put that token into a hidden field in the form, then checking that the form submission has that token and matches the one saved in $_SESSION

share|improve this answer
    
hmmm. this honeypot thing is opening a can of worms. thanks. –  wolverene Mar 1 at 14:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.