2
\$\begingroup\$

So, I have a basic 'contact us' form that i have built, and I do some jQuery checking first (that a phone number is only numbers, email address doesn't contain unneeded characters, etc), but I also wanted to do some sanitization before sending out the email.

After reading many questions on SO (such a great service!) then doing some more reading on PHP.net and seeing some 'in use' code on W3Schools this is what I came up with. Is it secure? Is there more I should be doing? Is this overkill?

It should be said that the user content lives on my server for a very short amount of time and then is sent off to email services (GMail and HostGator)

<?
$name = filter_var($_POST[name], FILTER_SANITIZE_STRING);
$email = filter_var($_POST[email], FILTER_SANITIZE_EMAIL);
$phone = filter_var($_POST[phone], FILTER_SANITIZE_NUMBER_INT);
$usermessage = filter_var($_POST[message], FILTER_SANITIZE_STRING);
$businessmessage = filter_var($_POST[businessmessage], FILTER_SANITIZE_STRING);


$to = "spokedcouriers@gmail.com, tom@spokedcouriers.com";
$subject = "Incoming from SpokedCouriers.";
$message =
"<b>Submitted From: </b>".$name."<br>".
"<b>Email Address: </b>".$email . "<br>" . 
"<b>Phone Number: </b>".$phone . "<br>" .
"<b>Message: </b>".$usermessage . "<br>" . 
"<b>Possible Business Message From: </b>".$businessmessage."<br>";

$headers  = 'MIME-Version: 1.0' . "\r\n"
    .'Content-type: text/html; charset=iso-8859-1' . "\r\n"
    .'From: ' . $email . "\r\n"
    .'Reply-To: ' . $email . "\r\n";

//GO AWAY.
mail($to, $subject, $message, $headers);
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ "PHP Notice: Use of undefined constant name - assumed 'name'" and "PHP Notice: Use of undefined constant email - assumed 'email' and so on... edit your ini file, and set it to E_STRICT | E_ALL for debugging, and show the errors! The notices are there to help you, don't ignore them \$\endgroup\$ – Elias Van Ootegem Nov 29 '13 at 10:17
7
\$\begingroup\$

Ok, this may come across as blunt or harsh, but IMO, that's what CodeReview has to do, as I explained earlier here.
So please, keep that in mind while reading this post.

Before I even start dealing with your code, I'd like to point out to you that you're using an unreliable, incomplete and misleading resource: w3schools. Sure, it may get some things right, but IMO, it's a misleading site. Many think the site is condoned, or even part of the W3C org. This is not the case, Please check the criticisms.
You are, of course, free to use w3schools, but be aware that it's not the best resource out there.

Is it secure?
No. I'd like to be kind about this, but that wouldn't help you. No, your code is still vulnerable to things like mail injection. I could easily copy paste some mail headers into your form and have your server, seemingly, forward a message sent yesterday by your rich, recently so tragically deceased alienated uncle in Barbados who left you his entire fortune.
I've included a link on mail injection in the bottom of this answer. This is an important issue that you need to address, still.

Now, the code:

I'll be going over your code block by block, sometimes even line-by-line:

<?

Don't use the short tag! Never. Period. It's not a lot of effort to write <?php instead, and it's far more reliable (<?php won't be confused with the odd <? xml...), and doesn't rely on the short-tags being enabled on your server.

$name = filter_var($_POST[name], FILTER_SANITIZE_STRING);
$email = filter_var($_POST[email], FILTER_SANITIZE_EMAIL);
$phone = filter_var($_POST[phone], FILTER_SANITIZE_NUMBER_INT);
$usermessage = filter_var($_POST[message], FILTER_SANITIZE_STRING);
$businessmessage = filter_var($_POST[businessmessage], FILTER_SANITIZE_STRING);

Ok, this block is essentially doing the same thing several times, and suffers from the same issue throughout.
First off, when setting the error reporting level to the Correct debugging levels of E_STRICT | E_ALL, you'll see a ton of notices and warnings pop up:

  • PHP Notice: Use of undefined constant name - assumed 'name'

This means you forgot to quote the array keys, PHP will helpfully assume you meant 'name', but it will issue a notice. This makes your code look sloppy and amateurish. More over, it costs resources, slowing your code down.

  • PHP Notice: Undefined index: name

If your script is being executed and none, or not all of the post parameters have been sent, you're still accessing those keys in the $_POST super global. At no point are you checking if there has been a POST request.
This can produce the notice above, for each time you access the POST array. Add some basic checks:

<?php//full tag
if (!$_POST) exit();//no post request, no magic!
$name = isset($_POST['name']) ? filter_var($_POST['name'], FILTER_SANITIZE_STRING) : null;
  • Don't "over-filter"!

Where your usage of filter_var($email, FILTER_VALIDATE_EMAIL) is to be commended, your code:

$phone = filter_var($_POST[phone], FILTER_SANITIZE_NUMBER_INT);

is not the best of ideas. Now I don't know how you get the input from the user, but if I were to punch in "+10800123456", I'd translate that + to double 0, to standardize the input.
Apart from that FILTER_SANITIZE_NUMBER_INT will clean up input like "123.313.456,1", removing the comma and dots, but it will leave the dashes and plusses in "+123-313+456-1" untouched. If I were you, I'd simply do something like:

if ($phone{0} == '+')
    $phone = '00'.$phone;
preg_replace('/[^0-9]+/','',$phone);

This expands a leading + to double zeroes and removes everything Except digits from the string. You can then easily check the length, to determine (roughly) the validity of the input. If it's 3 digits long, it probably isn't valid.

  • mail is good for testing, less so for production

Sure mail is easy (once you get your server settings worked out), and does what it says on the tin: it sends an email.
However, sometimes you might require an alt-body (text and HTML), sometimes you may want to add attachments. Writing those headers all the time is a bit of a faff, and has been done before. Good programmers are lazy: they won't bother writing code that exists, and is free to use. The PHPMailer class is in common usage and is very easy to set up, and quite versatile. Though, as you can see here, still somewhat open to mail injection.

In terms of security, PEAR::Mail is still the better option (though not in terms of clean code IMO). Read through the link, and learn about mail injection some more.

\$\endgroup\$
  • \$\begingroup\$ I could easily copy paste some mail headers into your form - really? The only variable in the headers is $email which is filter_vared as an email address - if you believe the code to be exploitable a specific example for the code posted would be appropriate. +1 anyway as the general advice is sound. \$\endgroup\$ – AD7six Dec 3 '13 at 14:40
  • \$\begingroup\$ @AD7six: Try posting a raw email message in various online mail providers. Only a year ago, most of these services (yes, including microsoft's hotmail) failed to deal with that properly... It wasn't shown as simple copy-paste flat text, but it was actually rendered as a series of forwarded messages (I noticed this when I tried to help someone who was the victim of a scam. I needed the headers to track the origin of the messages) \$\endgroup\$ – Elias Van Ootegem Dec 3 '13 at 15:02
  • \$\begingroup\$ That is a none-answer \$\endgroup\$ – AD7six Dec 3 '13 at 15:28
  • \$\begingroup\$ @AD7six: What I was hinting at is that $usermessage may contain html or mail headers, which may "confuse" a mail client \$\endgroup\$ – Elias Van Ootegem Dec 3 '13 at 16:03
  • \$\begingroup\$ Maybe we aren't talking about the same thing then. email header injection is what the answer suggests is possible ("No, your code is still vulnerable") whereas the misrepresentation of text in a mail client is simply not the same sort of problem - it's not like you can exploit that to send mails as someone else. \$\endgroup\$ – AD7six Dec 3 '13 at 16:07
1
\$\begingroup\$

Is it secure?

Let me answer that question with a question, are you using the HTTPS protocol to send the form data from the user to the server? If not, anyone 'listening' on the connection could potentially read the mail-data that the user is sending to the server.

As for the PHP script itself, well, you sure do sanitize it, but that's not a real issue with mail, the main problem with mail is having it exploited to the extent of someone spamming with it. So if the location of the PHP file containing the script is available to the world wide web, anyone can send mails with it, allowing us to exploit it.

Try reading up on sessions, or maybe using a simple .htaccess file to avoid unwelcome users.

GO AWAY. What is that part in your code? Did you even notice that it's there?

Also, this is how you fetch data from $_POST: $_POST["key_here"];

\$\endgroup\$
  • \$\begingroup\$ I noticed the 'go away' blip right after I posted the question and commented it out in my code. How would sessions help with this form? I do have a simple .htaccess blocking public view of my PHP files. I am not using HTTPS to send the data. I would have to do some reading up on using that over my current scheme. Any helpful links? \$\endgroup\$ – tbremer Nov 29 '13 at 2:48
  • \$\begingroup\$ Sessions would be a go-to instead of .htaccess login, but seeing as you're using that, you wont need it. HTTPS is a whole different matter, but it's worthwhile reading up on if you're interested in having secure web applications. I'd suggest a google of: "What is SSL?" or "What is HTTPS?", gives some interesting results, just take into account that most of the answers are supplied by SSL cert. supplicants, so the answers may be biased. \$\endgroup\$ – Andreas Bjørn Nov 29 '13 at 2:51

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.