Preventing email injection

Question

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?

Answer 1

@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>";
}

Answer 2

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();
}