3
\$\begingroup\$

I have asked a question on Stack Overflow and one of the guys actually managed to hack my contact form and inject a fake email into the $header of the PHP!

So after talking to him, he sent me a few articles, and I aimed to update the PHP to secure this gap and prevent the spam.

Here is my old PHP:

<?php
   session_start();

   if ($_SERVER['REQUEST_METHOD'] == 'POST'){
      ob_start();

      if(isset(
         $_REQUEST['name'],
         $_REQUEST['email'],
         $_REQUEST['message'],
         $_REQUEST['number'],
         $_REQUEST['date'],
         $_REQUEST['select'],
         $_REQUEST['radio'],
         $_REQUEST['checkbox'],
         $_REQUEST['token']
      )){

         if($_SESSION['token'] != $_POST['token']){
            $response = "0";
         } else {
            $_SESSION['token'] = "";
            $name = $_REQUEST['name'];
            $email = $_REQUEST['email'];
            $message = $_REQUEST['message'];
            $number = $_REQUEST['number'];
            $date = $_REQUEST['date'];
            $select = $_REQUEST['select'];
            $radio = $_REQUEST['radio'];
            $checkbox = $_REQUEST['checkbox'];

            $to = "";
            $subject = "New Message From: $name";
            $message = "Name: $name<br/>
                        number: $number<br/>
                        date: $date<br/>
                        select: $select<br/>
                        radio: $radio<br/>
                        checkbox: $checkbox<br/>
                        Email: $email<br/>
                        Message: $message";

            $headers  = 'MIME-Version: 1.0' . "\r\n";
            $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
            $headers .= 'From: '.$email . "\r\n";
            $mailed = (mail($to, $subject, $message, $headers));

            if( isset($_REQUEST['ajax']))$response = ($mailed) ? "1" : "0";
            else $response = ($mailed) ? "<h2>Success!</h2>" : "<h2>Error! There was a problem with sending.</h2>";
            echo $response;
         }

      } else {
        echo "Form data error!";
      }
      ob_flush();
      die();
   }
?> 

and here is the updated version:

<?php
   session_start();

   if ($_SERVER['REQUEST_METHOD'] == 'POST'){
      ob_start();

      if(isset(
         $_REQUEST['name'],
         $_REQUEST['email'],
         $_REQUEST['message'],
         $_REQUEST['number'],
         $_REQUEST['date'],
         $_REQUEST['select'],
         $_REQUEST['radio'],
         $_REQUEST['checkbox'],
         $_REQUEST['token']
      )){

         if($_SESSION['token'] != $_POST['token']){
            $response = "0";
         }
       else {
            $_SESSION['token'] = "";
            $name = $_REQUEST['name'];
            $email = $_REQUEST['email'];
            $message = $_REQUEST['message'];
            $number = $_REQUEST['number'];
            $date = $_REQUEST['date'];
            $select = $_REQUEST['select'];
            $radio = $_REQUEST['radio'];
            $checkbox = $_REQUEST['checkbox'];

         $email_pattern = "/([\w\-]+\@[\w\-]+\.[\w\-]+)/";
         $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";

         switch (true){
            case !preg_match($email_pattern, $email):
               $response = "<b style='color: red'>Invalid email address!</b>";
               break;

            case !preg_match($spam_pattern, $name):
            case !preg_match($spam_pattern, $number):
            case !preg_match($spam_pattern, $date):
            case !preg_match($spam_pattern, $select):
            case !preg_match($spam_pattern, $radio):
            case !preg_match($spam_pattern, $checkbox):
            case !preg_match($spam_pattern, $message):
               $response = "<b style='color: red'>Invalid request made!</b>";
               break;

            default:
               $to = "";
               $subject = "New Message From: $name";
               $message = "Name: $name<br/>
                           number: $number<br/>
                           date: $date<br/>
                           select: $select<br/>
                           radio: $radio<br/>
                           checkbox: $checkbox<br/>
                           Email: $email<br/>
                           Message: $message";

               $headers  = 'MIME-Version: 1.0' . "\r\n";
               $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
               $headers .= 'From: '.$email . "\r\n";
               $mailed = (mail($to, $subject, $message, $headers));

               $response = ($mailed) ?
                  "<h2 style='color: green'>Success!</h2>"
                     :
                  "<h2 style='color: blue'>Error! There was a problem with sending.</h2>";
               break;
            }
         }
      }
      else {
         $response = "<b style='color: red'> Form data error! </b>";
      }
      ob_flush();
   }
?>

Can you please share your opinion? Does it look secure?

\$\endgroup\$
  • 4
    \$\begingroup\$ You don't have to validate email address yourself. PHP has a built in function filter_var($email, FILTER_VALIDATE_EMAIL); \$\endgroup\$ – James Feb 20 '14 at 16:16
5
\$\begingroup\$

@James is correct, PHP has function to validate so you don't have to (and they're better).

Your switch (true) is bothering me. I think an if condition could work there instead, as that's not why switches were built.

$mailed = (mail($to, $subject, $message, $headers));

You don't need the parentheses in this line. In fact, you could just shorten it all a bit and use:

if (mail($to, $subject, $message, $headers)) {
    $response = "<h2 style='color: green'>Success!</h2>";
} else {
    $response = "<h2 style='color: blue'>Error! There was a problem with sending.</h2>";
}
\$\endgroup\$
1
\$\begingroup\$

I tried to clean up your code a bit and implemented what @Alex L and @James said.

<?php

session_start();

if ($_SERVER['REQUEST_METHOD'] === 'POST'){
    ob_start();

    $isRequestSet = true;
    $keysToTest = array('name', 'email', 'message', 'number', 'date', 'select', 'radio', 'checkbox', 'token');
    foreach($_REQUEST as $reqKey => $reqValue){
        if(!in_array($reqKey, $keysToTest) || !isset($reqValue)){
            $isRequestSet = false; break;
        }
    }

    if($isRequestSet){
        if($_SESSION['token'] !== $_POST['token']){
            $response = "0";
        } else {
            unset($_SESSION['token']);

            $name     = $_REQUEST['name'];
            $email    = $_REQUEST['email'];
            $message  = $_REQUEST['message'];
            $number   = $_REQUEST['number'];
            $date     = $_REQUEST['date'];
            $select   = $_REQUEST['select'];
            $radio    = $_REQUEST['radio'];
            $checkbox = $_REQUEST['checkbox'];

            $response = null;
            if(filter_var($email, FILTER_VALIDATE_EMAIL) === false){
                $response = "<b style='color: red'>Invalid email address!</b>";
            } else if(preg_grep("/[\r\n]|Content-Type:|Bcc:|Cc:/i", array($name, $number, $date, $select, $radio, $checkbox, $message))){
                $response = "<b style='color: red'>Invalid request made!</b>";
            } else {
                $to       = "support@loaidesign.co.uk";
                $subject  = "New Message From: " . $name;
                $message  = "Name:   " . $name   . "<br/> Number:  " . $number . "<br/> Date:     " . $date     . "<br/>" .
                            "Relect: " . $select . "<br/> Radio:   " . $radio  . "<br/> Checkbox: " . $checkbox . "<br/>" .
                            "Email:  " . $email  . "<br/> Message: " . $message;
                $headers  = 'MIME-Version: 1.0' . "\r\n" . 'Content-type: text/html; charset=utf-8' . "\r\n" .
                            'From: ' . $email   . "\r\n";

                if(mail($to, $subject, $message, $headers)){
                    $response = "<h2 style='color: green'>Success!</h2>";
                } else {
                    $response = "<h2 style='color: blue'>Error! There was a problem with sending.</h2>";
                }
            }
        }
    } else {
        $response = "<b style='color: red'> Form data error! </b>";
    }

    ob_flush();
}
\$\endgroup\$
  • \$\begingroup\$ There is few errors man, it wont work :/ \$\endgroup\$ – Leo Feb 21 '14 at 23:20
  • \$\begingroup\$ @Loai Could you elaborate? What errors are being produced? \$\endgroup\$ – Alex L Feb 22 '14 at 19:55
  • \$\begingroup\$ @AlexL - missed a closing parenthesis on line 33 (if-preg_grep). Edited to fix that (see i.imgur.com/2ceLiHI.png) \$\endgroup\$ – jsanc623 Feb 23 '14 at 4:29

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.