3
\$\begingroup\$

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@website.com", $subject, $message, $from);
        echo "Email sent!";
        }
    }  
?> 
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – icktoofay May 16 '14 at 1:43
  • \$\begingroup\$ What would happen if they had new lines? How would I protect this form? \$\endgroup\$ – mccjeff May 16 '14 at 1:46
  • \$\begingroup\$ 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. \$\endgroup\$ – icktoofay May 16 '14 at 1:49
  • \$\begingroup\$ 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. \$\endgroup\$ – Robbie Averill May 16 '14 at 1:56
  • \$\begingroup\$ @icktoofay I'm sorry but I really don't understand. I don't know how to fix either of those problems. \$\endgroup\$ – mccjeff May 16 '14 at 1:57
3
\$\begingroup\$

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:

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\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.