4
\$\begingroup\$

I've just downloaded a script and would like to use it as a contact form on my site. The problem is I'm a front-end dev with little knowledge of securing PHP code. So, could you please have a look and let me know if there are any glaring problems with security with this script that uses phpMailer to send?

As you can see, I haven't done much with the SMTP setting at the bottom, but it still seems to work ok on my server.

<?php

$name = trim($_POST['name']);
$email = $_POST['email'];
$comments = $_POST['comments'];

$site_owners_email = '[email protected]'; // Replace this with your own email address
$site_owners_name = 'Name'; // replace with your name

if (strlen($name) < 2) {
    $error['name'] = "Please enter your name";    
}

if (!preg_match('/^[a-z0-9&\'\.\-_\+]+@[a-z0-9\-]+\.([a-z0-9\-]+\.)*+[a-z]{2}/is', $email)) {
    $error['email'] = "Please enter a valid email address";    
}

if (strlen($comments) < 1) {
    $error['comments'] = "Please leave a comment.";
}

if (!$error) {

    require_once('class.phpmailer.php');
    $mail = new PHPMailer();

    $mail->From = $email;
    $mail->FromName = $name;
    $mail->Subject = "Website Contact Form";
    $mail->AddAddress($site_owners_email, $site_owners_name);
    $mail->AddAddress('[email protected]', 'Name');
    $mail->Body="
    Name: $name
    Email: $email
    Comments: $comments";


    $mail->SMTPAuth = true; // turn on SMTP authentication
    $mail->Username = "[email protected]"; // SMTP username
    $mail->Password = ""; // SMTP password    

    $mail->Send();

    echo "<li class='success' id='successMessage' > Congratulations, " . $name . " We've received your email. We'll be in touch as soon as we possibly can!</li><li id='closeBtn'><a href='#'>close</a></li><li id='homeLink'><a href='http://mosaicwebstudio.com/sandbox/forms/mod-ajax2' >return to home page</a></li>";

} # end if no error
else {

    $response = (isset($error['name'])) ? "<li>" . $error['name'] . "</li> \n" : null;
    $response .= (isset($error['email'])) ? "<li>" . $error['email'] . "</li> \n" : null;
    $response .= (isset($error['comments'])) ? "<li>" . $error['comments'] . "</li>" : null;

    echo $response;
} # end if there was an error sending

?>
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

As @e-sushi remarked in his answer: you're still vulnerable to header injections. Though his approach is rather complex, and (when applied to $email, not required if you validate the email address the way I do (explained further down), but for the subject, and message body, you could use these simpler, and (IMO, therefore) better ways to tackle the header-injection issue:

$mail->Body = str_ireplace(
    array("%0a", "%0d", '0x0A'), '',//PHPMailer replaces \r\n already for you
   'your string here...'
); 
//a Core-function alternative:
$body = 'your string here';
filter_var($body, FILTER_SANITIZE_EMAIL);//will alter $body
$mail->Body = $body;

More details on the caveats/benefits of each approach can be found here. Because you're setting the $name along side the email address, too as the from of the email, you should pass the $name value to either one of these filters, too.

Your code is attempting to validate an email address using a regular expression that is, at best, slightly off. There is a built-in function that can validate email addresses, without an inaccurate, too restrictive and slow regular expression, called filter_var.

if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{
    exit('Invalid email');
}

The result is that, while your regex doesn't allow for: [email protected] to be used as an email address (even though it's valid), filter_Var will accept it.
Unlike your regex, which, because it uses the s modifier allows for line-breaks in your email (for some reason), this won't... not blindly.
If, however, you wish to keep on using a regular expression, here's a PCRE expression that is (almost) fully RFC822 compliant:

/(?:(?:\r\n)?[ \t])*(?:(?:(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t]
)+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:
\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(
?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ 
\t]))*"(?:(?:\r\n)?[ \t])*))*@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\0
31]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\
](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+
(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:
(?:\r\n)?[ \t])*))*|(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z
|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)
?[ \t])*)*\<(?:(?:\r\n)?[ \t])*(?:@(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\
r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[
 \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)
?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t]
)*))*(?:,@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[
 \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*
)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t]
)+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*)
*:(?:(?:\r\n)?[ \t])*)?(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+
|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r
\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:
\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t
]))*"(?:(?:\r\n)?[ \t])*))*@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031
]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](
?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?
:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?
:\r\n)?[ \t])*))*\>(?:(?:\r\n)?[ \t])*)|(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?
:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?
[ \t]))*"(?:(?:\r\n)?[ \t])*)*:(?:(?:\r\n)?[ \t])*(?:(?:(?:[^()<>@,;:\\".\[\] 
\000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|
\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>
@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"
(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*))*@(?:(?:\r\n)?[ \t]
)*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\
".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?
:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[
\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*|(?:[^()<>@,;:\\".\[\] \000-
\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(
?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)*\<(?:(?:\r\n)?[ \t])*(?:@(?:[^()<>@,;
:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([
^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\"
.\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\
]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*(?:,@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\
[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\
r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] 
\000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]
|\\.)*\](?:(?:\r\n)?[ \t])*))*)*:(?:(?:\r\n)?[ \t])*)?(?:[^()<>@,;:\\".\[\] \0
00-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\
.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,
;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|"(?
:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*))*@(?:(?:\r\n)?[ \t])*
(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".
\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t])*(?:[
^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\]
]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*\>(?:(?:\r\n)?[ \t])*)(?:,\s*(
?:(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\
".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)(?:\.(?:(
?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[
\["()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t
])*))*@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t
])+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?
:\.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|
\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*|(?:
[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".\[\
]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)*\<(?:(?:\r\n)
?[ \t])*(?:@(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["
()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)
?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>
@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*(?:,@(?:(?:\r\n)?[
 \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,
;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\.(?:(?:\r\n)?[ \t]
)*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\
".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*)*:(?:(?:\r\n)?[ \t])*)?
(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\["()<>@,;:\\".
\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])*)(?:\.(?:(?:
\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z|(?=[\[
"()<>@,;:\\".\[\]]))|"(?:[^\"\r\\]|\\.|(?:(?:\r\n)?[ \t]))*"(?:(?:\r\n)?[ \t])
*))*@(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])
+|\Z|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*)(?:\
.(?:(?:\r\n)?[ \t])*(?:[^()<>@,;:\\".\[\] \000-\031]+(?:(?:(?:\r\n)?[ \t])+|\Z
|(?=[\["()<>@,;:\\".\[\]]))|\[([^\[\]\r\\]|\\.)*\](?:(?:\r\n)?[ \t])*))*\>(?:(
?:\r\n)?[ \t])*))*)?;\s*)/

Source: ex-parrot

I think it's clear which one of the validations is the more elegant one...

The name ($name) is a trimmed version of the POST var. That's good, because you know that, if the strlen($name) > 2, you're not just counting whitespace. However, you still are counting possible whitespace-only input when validating the $comment variable.

I'd suggest additional processing on both of these values:

$name = trim(html_entity_decode($_POST['name']));
if (strlen($name) < 2 || !preg_match('/^[^\d]+\s*[^\d]*/',$name))
{//nobody is called R2D2 in real-life!
    exit('name is too short ( < 2 chars) or contains numeric chars');
}
$comment = trim(strip_tags(html_entity_decode($_POST['comment'])));
if (!strlen($comment))
{//empty comment
   exit('No comment supplied');
}

As you can see, I've also used html_entity_decode and, for the comments at least strip_tags to remove any markup that might have been passed. This is often overlooked, so if I were to post a comment saying:

&nbsp;<br/>&nbsp;

Even trim would see this as a valid comment, whereas it actually is little more than whitespace

\$\endgroup\$
1
  • \$\begingroup\$ //nobody is called R2D2 in real-life! – Well, let’s just hope Kenny Baker never tries to contact him using that contact form. ;) \$\endgroup\$ Commented May 27, 2014 at 15:57
0
\$\begingroup\$

You are pushing whatever users enter in the comment form via email unfiltered.

In simpler words: you will need to filter all data the user enters, to make sure you are safe from the usual mail injects.

Example:

function my_filter($data)
{
    $data = preg_replace('=((<CR>|<LF>|0x0A/%0A|0x0D/%0D|\\n|\\r)\S).*=i', '', $data);
    $data= trim(preg_replace('=[\r\n]+=', '', $data)); 
    return $data;
}

At least run the user-defined data trough the filter to be safe from email header injects etc. just in case the PHP mailing class you are using doesn't do that for you.

$name = my_filter($_POST['name']);
$email = my_filter($_POST['email']);
$comments = my_filter($_POST['comments']);

Then, it should be somewhat safe.

Another thing you might considder adding is some kind of captcha to your mailing/comment form to prevent bots to auto-post thousands of messages without anyone stopping them.

\$\endgroup\$
4
  • \$\begingroup\$ filter_var to validate the email is all you need. PHPMailer does sanitize (not enough, though) the input, but your approach is, IMO, a tad over the top... \$\endgroup\$ Commented Oct 11, 2013 at 6:51
  • \$\begingroup\$ @EliasVanOotegem Note that OP did not specify which version of PHP is used. filter_var is only available in PHP 5.2.0 and later versions. Also, filter_var has a lot of problems! Eg: will not validate valid domain names with foreign characters, there are email addresses FILTER_VALIDATE_EMAIL rejects but RFC5321 specifically permits, it will not protect you from injection-attacks, and much more… \$\endgroup\$ Commented May 27, 2014 at 16:17
  • \$\begingroup\$ Come on, 5.2 was released about 8 years ago, is no longer actively maintained either. The common and safe assumption to make is people are working with PHP 5.3 and up (oh, and filter_var has its problems, true enough, but since its release, there have been a lot of changes and tweaks to the email filter) \$\endgroup\$ Commented May 28, 2014 at 7:02
  • \$\begingroup\$ Your “common and safe assumption” breaks as soon as you hit Murphy's law in relation to production servers. Actually, you´re saying it yourself: “since its release, there have been a lot of changes and tweaks to the email filter”, which already hints at the differences and issues of different PHP versions. Practically, the “changes and tweaks” contradict your ”safe assumption”. Anyway – since OP asked about potentially securing his/her PHP code, my answer that OP doesn't use the needed filtering to prevent injections is no where near “a tad over the top”. Instead, it's pretty on-point. \$\endgroup\$ Commented May 18, 2015 at 9:50

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.