HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Security of a "contact us" form

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactformsecurity

Problem

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 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("test@example.com");
            $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?

Solution

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:

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


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

!#$%&'*+-=?^_`{}|~@example.org
"()<>[]:,;@"!#$%&'*+-=?^_`{}| ~.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.

Code Snippets

$email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
//a bit later on:
$email = str_replace(array("\r", "\n", "%0a", "%0d"), '', stripslashes($email));
!#$%&'*+-/=?^_`{}|~@example.org
"()<>[]:,;@\\\"!#$%&'*+-/=?^_`{}| ~.a"@example.org
"very.(),:;<>[]\".VERY.\"very@\\ \"very\".unusual"@strange.example.com
!#$%&'*+-=?^_`{}|~@example.org
"()<>[]:,;@"!#$%&'*+-=?^_`{}| ~.a"@example.org
"very.(),:;<>[]".VERY."very@ "very".unusual"@strange.example.com
$email = filter_var($_POST['email'], FILTER+_SANITIZE_EMAIL);
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    echo $jsonHelper->encode($email.' is an invalid address'));
}

Context

StackExchange Code Review Q#40766, answer score: 5

Revisions (0)

No revisions yet.