PHP form-to-email script

Question

Someone says that my PHP code is vulnerable to XSS. I asked them what I should do to fix it, but now they don't want to help.

$action=$_REQUEST['action'];
 /* send the submitted data */
 {
 $name=$_REQUEST['name'];
 $email=$_REQUEST['email'];
 $message=$_REQUEST['message'];
 if (($name=="")||($email=="")||($message==""))
 {
 echo "All fields are required, please fill <a href=\"\">the form</a> again.";
 }
 else{ 
 $from="From: $name<$email>\r\nReturn-path: $email";
 $subject="Message sent using your contact form";
 mail("[email protected]", $subject, $message, $from);
 echo "Email sent!";
 }
 } 
?> 

• icktoofay: This probably isn't what they were referring to, but also consider what would happen if $_REQUEST['name'] and/or $_REQUEST['email'] contained newlines. Also, you may want to consider protecting this form against Cross-Site Request Forgery if you aren't already doing so elsewhere.
• mccjeff: What would happen if they had new lines? How would I protect this form?
• icktoofay: If they had newlines, they might be able to insert an extra header. That might or might not be harmful, but you probably don't want to let that happen. To protect against CSRF, the standard solution is to have the page with the form on it to include a random token as a hidden field and store it as a cookie or in the session. When you process the submitted form, you check that the hidden field exists and matches the cookie. If they do not match, you reject the request.
• scrowler: It is very vulnerable to spam injection of any kind as you are doing minimal validation (a single space in the input fields would get past this validation). I also suggest you be more specific about your request method, if you're expecting POST data, use $_POST, if you're expecting GET data, use $_GET. Also consider using a CAPTCHA of some sort to prevent people spamming your contact form - otherwise your emails will inevitably end up in a junk mail filter somewhere.
• mccjeff: @icktoofay I'm sorry but I really don't understand. I don't know how to fix either of those problems.

Answer 1

Sanitize the user input, but first and foremost: make sure there actually is any user input to sanitize:

$foo = $_POST['bar'];

might issue a notice, if that post variable doesn't exist, that's why it's considered good practice to use isset:

if (!isset($_POST['email']))
 exit('No email address provided');//don't use exit, redirect or something
$email = $_POST['email'];

Next, you sanitize and validate the input. For email addresses, that's easily done:

if (!filter_var($email, FILTER_VALIDATE_EMAIL))
 exit('Invalid email address given');//again, use redirect here

But all the other stuff has to be processed to avoid something you are wide open to:

Mail injection attacks

I've reviewed a couple of code snippets, in detail. Instead of copy-pasting my existing answers here, or typing them a second, third or fourth time, I'll just list a couple of links to some of these answers:

• Critique sanitized user email PHP Script (Has most on mail injection, check the links in that answer, too)
• Security of a "contact us" form Focusses more on email address validation
• How can I make this code safer against XSS attacks? more general on XSS attacks, but make sure to check the links at the bottom.

Side-notes:
If a script contains nothing but PHP code, then the closing ?> tag is best omitted. This advice is given everywhere, including the official php.net site. Google as to why (whitespacing, parser tokens, ...)

I have the feeling you're not showing us the full code, because you have an opening and closing {} around everything, except the first statement.
If this code is the actual code, then please remove those redundant brackets: PHP isn't block-scoped, so they serve no purpouse, other than to confuse anyone else who might ever gaze at your code.

Answer 2

Short version: Yes.

Long version:

• If the recepient opens the email using a browser.
• You only guard your inputs using empty checks.
  • You didn't check for malicious scripts. One can easily drop a script, which opens an iframe on the entire page and have the user think it's a legit site.
  • You didn't check for invalid HTML which could break the surrounding HTML of the email provider of the recipient, and do some stuff with it.
  • bla bla bla...

XSS doesn't have an over-the-counter fix and is hard to spot doing manually. If you want to find out how to prevent XSS, then check out OWASP's XSS (Cross Site Scripting) Prevention Cheat Sheet.