Are there any open vulnerabilities in this mailer script?

Question

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="[email protected]";
      $headers="From: {$email}\r\nReply-To: {$donot}"; //create headers
      mail('[email protected]',$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('[email protected]',$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.

Answer 1

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="[email protected]";
      $headers="From: {$email}\r\nReply-To: {$donot}"; //create headers
      mail('[email protected]',$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 = "[email protected]";
$headers = "From: {$email}\r\n"
         . "Reply-To: {$return_address}";
mail('[email protected]', $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”).

Answer 2

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

[email protected]%0d%0aCc:%[email protected]

… then you will compose a message with headers

From: [email protected]
Cc: [email protected]
Reply-To: [email protected]
Cc: [email protected]

PHP's mail() function will "helpfully" add [email protected] as a spam recipient. The spam will look like it came from [email protected].

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" <[email protected]>

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?=" <[email protected]>

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"),
                       "[email protected]");

If you write your code as

$email = '"François Müller" <[email protected]>';
$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 = '<[email protected]>';
$from_header = mb_encode_mimeheader("From: $email", "UTF-8", "Q");

… passes the text through unchanged, producing

From: <[email protected]>

---

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.

Answer 3

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('[email protected]',$subject,$content,$headers); //mails it
    $success = "Thank you! You're email has been sent.";

Answer 4

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.

Answer 5

You set Reply-To: to "[email protected]". 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.

Answer 6

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