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\$
1
  • 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, 2014 at 16:16

2 Answers 2

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       = "[email protected]";
                $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\$
3
  • \$\begingroup\$ There is few errors man, it wont work :/ \$\endgroup\$
    – Leo
    Feb 21, 2014 at 23:20
  • \$\begingroup\$ @Loai Could you elaborate? What errors are being produced? \$\endgroup\$
    – Alex L
    Feb 22, 2014 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, 2014 at 4:29

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.