6
\$\begingroup\$

I have a form, which uses AJAX to send POST data to the following controller method:

public function action_sendmail() {
        $jsonHelper=Loader::helper('json');
        $answer = (int) $_POST['security'];
        if($answer <= 0)
            $answer = 1;

        if($answer == 4){

            $name = filter_var($_POST['name'], FILTER_SANITIZE_STRING);
            $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
            $message = filter_var($_POST['message'], FILTER_SANITIZE_STRING);

            $name = strip_tags($name);
            $email = strip_tags($email);
            $message = strip_tags($message);

            $name = str_replace(array("\r", "\n", "%0a", "%0d"), '', stripslashes($name));
            $email = str_replace(array("\r", "\n", "%0a", "%0d"), '', stripslashes($email));
            $message = str_replace(array("\r", "\n", "%0a", "%0d"), '', stripslashes($message));

            if(!filter_var($email, FILTER_VALIDATE_EMAIL)){
                echo $jsonHelper->encode(t("Wrong email address."));
                die;
            }
            if(preg_match("/^[+]?[0-9]*$/", $_POST['phone'])) {
                $phone = strip_tags($_POST['phone']);
            }
            else {
                echo $jsonHelper->encode(t("Wrong phone number format."));
                die;
            }

            $message = t('Phone number').": ".$phone."\n\n".$message;

            $mail = Loader::helper('mail');
            $mail->setBody($message);
            $mail->from($email, $name);
            $mail->to("[email protected]");
            $mail->to($email);
            $mail->setSubject(t('Example subject'));

            $sent = true;

            $mail->sendMail();

            if($sent) {
                echo $jsonHelper->encode("sent");
                die;
            }
            else {
                echo $jsonHelper->encode(t("Could not send mail. Please try again."));
                die;
            }

        }
        else{
            echo $jsonHelper->encode(t("Wrong security answer!"));
            die; 
        }
    }

The script sanitizes different attributes (name, email and the message) and sends an email to a specified address and the one from the HTML form. My question: Is the form secure?

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Secure: Not yet, but it's a start. Your validation is too strict in some places, and may result in the wrong email address being used. In particular here:

$email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
//a bit later on:
$email = str_replace(array("\r", "\n", "%0a", "%0d"), '', stripslashes($email));

If the value of $email is one of these examples of valid email addresses:

!#$%&'*+-/=?^_`{}|[email protected]
"()<>[]:,;@\\\"!#$%&'*+-/=?^_`{}| ~.a"@example.org
"very.(),:;<>[]\".VERY.\"very@\\ \"very\".unusual"@strange.example.com

you're stripping the slashes, which'll yield:

!#$%&'*+-=?^_`{}|[email protected]
"()<>[]:,;@"!#$%&'*+-=?^_`{}| ~.a"@example.org
"very.(),:;<>[]".VERY."very@ "very".unusual"@strange.example.com

Effectively changing the actual email address, and this may in turn turn a valid, though outlandish email address into an invalid one.
BTW: source for these email addresses is the email wiki

When it comes to email addresses: use filter_validate($emai, FILTER_VALIDATE_EMAIL) and rely on that function's ability to validate the string. Don't do any processing of your own, especially not after having sanitized and validated the email address.
Bottom line, if you want to validate an email address, while allowing users to pass an (accidental) \r char, use this:

$email = filter_var($_POST['email'], FILTER+_SANITIZE_EMAIL);
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    echo $jsonHelper->encode($email.' is an invalid address'));
}

On the other hand, you're not sanitizing the message and name as well as maybe you should. It's not unlikely your script is vulnerable to mail injection. A must-read on this subject can be found here.

Since we're all in the business of programming, I hope you'll understand I'm not going to repeat myself here, but I've dealt with mail injection previously on this site: read my answer to this question, which focusses on injection a bit more.

Some other niggles:

  • Please, as ever, try to stick to the coding standards as described by PHP-FIG. Most, if not all, major players subscribe to this standard, so should you. Check the standard here.
    Things like public function foobar(){ are not accepted, the opening brace should go on the next line. It may seem silly to stumble over this, but really: it isn't.
  • don't use die in a method, and avoid using echo. I take it you're using a framework of sorts. Check if there's an ajax helper that outputs, or use the view to echo JSON content. And from within an action (controller method), don't call die, but rather return, signaling an uneventfull end of the controllers' job. die breaks the normal way of processing a request, which may cause problems down the road.
\$\endgroup\$
13
  • 1
    \$\begingroup\$ @user3001234: Not quite: you're not removing the line-endings, but merely percent-encoding them. Looking at the PEAR _sanitizeHeaders method, I'd assume that alone does not suffice, since they remove %0a, too. I'm only guessing here, but I take it that, seeing as they remove this, there is a way to exploit encoded line-endings, too. You're also not checking for <CR> and the like. Perhaps consider calling strip_tags on the message, and send it with explicit text headers \$\endgroup\$ Feb 4, 2014 at 10:11
  • 1
    \$\begingroup\$ @user3001234: You can use it for inspiration. Personally, I tend to avoid PEAR packages, because they do tend to raise notices, because of their tendency to try to be as backward-compatible as possible. Running PHP5.5, methods that don't specify their being public, private or protected and methods that are being called both as member functions and statics... that's just messy PHP4-style coding. The main point is: the sanitizing has all been written out, copy with pride (and in accordance with the licence) \$\endgroup\$ Feb 4, 2014 at 10:20
  • 1
    \$\begingroup\$ @user3001234: not for $email, for the email address filter_validate($email, FILTER_SANITIZE_EMAIL) already removes any non-valid address chars. and I believe "<CR>MyAddress"@example.org is a valid email address, because the <> is contained within quotes. Just call the preg_replace on the name and message and you should be fine \$\endgroup\$ Feb 4, 2014 at 10:24
  • 1
    \$\begingroup\$ Okay, thanks for pointing me in the right direction. I used the dream.in.code method because I wasn't aware of <CR>. The table in the page you provided pretty much sums it up - the PEAR method is the safest one as far as I can see. \$\endgroup\$
    – Onion
    Feb 4, 2014 at 10:26
  • 1
    \$\begingroup\$ @user3001234: AFAIKT, yes, I'd say it's pretty secure. You don't seem to be storing the data in a DB anywhere (if you do, use prepared statements, and you're good). So the only things you need to check are: 1) validation and 2) mail injection. You've addressed both sufficiently, I do think \$\endgroup\$ Feb 4, 2014 at 10:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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