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!";
        }
    }  
?> 

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.