2
\$\begingroup\$

I have made slight edits to a simple PHP contact form that I have found online. Can you see anything else that I could do to ensure that I am protected from email injections and spam? I have used trim, stripslashes and htmlspecialchars and made a simple question to act as spam prevention.

<?php

$myemail = "YOUR EMAIL ADDRESS";

$name = check_input($_POST['name'], "Enter your name");
$email = check_input($_POST['email']);
$message = check_input($_POST['message'], "Write your message");
$captcha = check_input($_POST['captcha']);

if (!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/", $email))
{
show_error("E-mail address not valid");
}

if (!preg_match("/5/", $captcha))
{
show_error("Check your math");
}


$message = "

Name: $name
E-mail: $email

Message:
$message

";

mail($myemail, $subject, $message);

header('Location: thanks.html');
exit();

function check_input($data, $problem='')
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);

if ($problem && strlen($data) == 0)
{
show_error($problem);
}
return $data;
}

function show_error($myError)
{
?>
<html>
<body>
<p>Please correct the following error:</p>
<strong><?php echo $myError; ?></strong>
<p>Hit the back button and try again</p>

</body>
</html>
<?php
exit();
}
?>
\$\endgroup\$
4
\$\begingroup\$

Most of my comments are unrelated to security. A few down at the bottom.

$myemail = "YOUR EMAIL ADDRESS";

Elsewhere you use snake_case names. If you did that here, you'd have $my_email. I'm assuming that you removed the read email address for privacy reasons.

if (!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/", $email))
{
show_error("E-mail address not valid");
}

The indenting here is not standard. Usually it would have been written

if ( ! preg_match('{([\w\-]+\@[\w\-]+\.[\w\-]+)}', $email) )
{
    show_error("E-mail address not valid");
}

I also added some whitespace in the if because I find it easier to read.

I changed to a single quoted string and used a different regular expression delimiting pair.

Another problem is that this will match some invalid emails. For example, -@-.- is valid in this pattern. So if those five characters appear in the field, it will be recognized as valid. Note that you don't restrict characters that appear before and after that sequence.

if (!preg_match("/5/", $captcha))

This would match 15, 51, etc. as well as 5. You don't need to use preg_match here. You can just say something like

if ( '5' != $captcha )

or

if ( 5 != (int)$captcha )

Note that if you want the exact original functionality (match any string that contains the number 5), you could say

if ( false === strpos($captcha, '5') )

That would be much faster than the regular expression version.

$message = "

Name: $name
E-mail: $email

Message:
$message

";

This would more commonly be written with a heredoc rather than a multiline double quoted string.

$message = <<<EOM

Name: $name
E-mail: $email

Message:
$message

EOM;

You don't need a final

?>

The usual convention is to leave it off if your script is not supposed to produce output. Technically speaking show_error does produce output, but it's not supposed to be called by the script. I'd leave off the final ?>.

Security

You don't show how the $subject is set, so it's unclear if it's safe or not. If this is the whole script, it will send with a blank subject.

Note that trim and stripslashes do not add security. They help fix the input to make it display properly. The normal use of stripslashes is to undo a PHP security protection that didn't quite work right. The htmlspecialchars is the security function. I'm not sure that email is vulnerable to HTML characters. Perhaps your email client is though.

\$\endgroup\$
  • \$\begingroup\$ Thank you for taking time out to look at this, do you by any chance know of a php contact form that is known to be safe 'straight out of the box'? \$\endgroup\$ – Andy Penn Jan 9 '15 at 21:17
2
\$\begingroup\$

I think @Brythan did a fine job covering syntax and beautification, so I'll cover the security aspects.

Can you see anything else that I could do to ensure that I am protected from email injections and spam?

Yes. In fact, you've exposed yourself to multiple security threats. Let's break down your code, section by section.

First off, we have:

$name = check_input($_POST['name'], "Enter your name");
$email = check_input($_POST['email']);
$message = check_input($_POST['message'], "Write your message");
$captcha = check_input($_POST['captcha']);

If we follow execution order, that will then take us to:

function check_input($data, $problem='')
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);

if ($problem && strlen($data) == 0)
{
show_error($problem);
}
return $data;
}

This here presents an issue. One large security flaw you're open to is CRLF-injection, a great answer can be read about this on another Code Review Question. That same link can also tell you that you're open to strip_slash() flaws (specifically found in this answer).

Here is an excellent Code Review answer which fully covers what you need to know. I recommend reading the sources linked from that answer as well. In fact, this is probably a must read.

Up next, I notice you're using a regular expression to check emails. Very, very wrong. Please use filter_var() to validate emails.

Soon after that, I see you've made a nice little captcha to prevent spamming! However, I discourage this completely. First off, what you've made does absolutely no good against spamming bots. And secondly, it creates a major inconvenience for users (UX.SE gives us some alternatives (read all of the answers please) and more information on the subject).

Basically, your captcha is useless. An intelligent bot can easily learn and defeat this within seconds. I suggest you read a few pieces, such as this Carnegie Mellon educated publication and this Programmers.SE question.

From the first publication:

We do not allow captchas to base their security in the secrecy of a database or a piece of code.

So your hard-coded answer unfortunately won't cut it in this modern day and age.

The second resource, the question on Programmers.SE has some great info on the threats that you face with a custom captcha, solutions to that, and even the benefits of having a custom captcha (however they're assuming something more robust than yours).

And that sums it up for security. Hopefully I have opened you (and maybe others) up to a new perspective on mail security!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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