3
\$\begingroup\$

A day ago I have asked a question on here about Preventing email injection. I had some feedback and worked on it, and below is the latest update.

Could anyone please share their opinion? Is it secure enough? Is there a way to short-cut the code?

<?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'];

         $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
            switch (true){
            case !filter_var($email, FILTER_VALIDATE_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!</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";

                    if(mail($to, $subject, $message, $headers)){
                        $response = "<h2 style='color: green'>Success! Your submission has been sent.</h2>";
                    }else{
                        $response = "<h2 style='color: blue'>Error! There was a problem with sending.</h2>";
                    }
            break;
               }
            }
         }
         else {
            $response = "<b style='color: red'>Error</b>";
         }
      ob_flush();
   }
?>
\$\endgroup\$
4
\$\begingroup\$
  • Due to the nested and long conditions the code becomes hard to follow at some points. Early returns prevent nesting of conditions.
  • Some code is duplicate (step 9)
  • Possibly some values could be overwritten by url-forging (depending on the server-configuration). (step 5)

I'll do a step-by-step refactoring here with some explanations.

  1. Move everything in a function. This eases early-return.

    function sendMail() {
        // your code here. 
    }
    
  2. While not necessary, it's I like to invert the conditions so the static expression stands on the left side. This avoids accidental assignment in conditions. Avoids tricky to detect bugs (though most IDEs and codesniffer emit a warning).

  3. Using the === operator for comparison avoids bugs to unforeseen casting

    function sendMail() {
        if ('POST' !== $_SEVER['REQUEST_METHOD']) {
            return '';
        }
    
        if ($_SESSION['token'] !== $_POST['token']) {
            return '0';
        }
        // remaining code
    }
    
  4. What's the purpose of your output-cachting? I can't see any outputs being made? Trying to catch any errors? Hiding them is a bad idea, catch and handle them. If simply removed them.

  5. You are using $_REQUEST instead of $_POST. Sure you do check if the request type. But this doesn't prevent anyone from submitting both, GET and POST values. And here lies a dangerous problem: If I send you a link to form.php?message=something this does get send to the server as well when you submit the form. Now GET and POST both having the key name collide. Depending on the variables_order the GET value I set overwrites your message. If you use $_POST to access your variables instead this issue can be prevented.
  6. I have extracted a simple accessor function to check for existence of a $_POST variable and return else:

    function getPostValue($key) {
        return isset($_POST[$key]) ? $_POST[$key] : null;
    }
    
    function sendMail() {
        if ('POST' !== $_SEVER['REQUEST_METHOD']) {
            return '';
        }
    
        if ($_SESSION['token'] !== $_POST['token']) {
            return '0';
        }
    
        $_SESSION['token'] = '';
    
        $name = getPostValue('name');
        $email = getPostValue('email');
        $message = getPostValue('message');
        $number = getPostValue('number');
        $date = getPostValue('date');
        $select = getPostValue('select');
        $radio = getPostValue('radio');
        $checkbox = getPostValue('checkbox');
    
        if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
            $response = "<b style='color: red'>Error</b>";
        }
    }
    
  7. All those input validations go with early return again (snippet only):

        $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
        if (!preg_match($spam_pattern, $name) 
            || !preg_match($spam_pattern, $number)
            || !preg_match($spam_pattern, $date)
            || !preg_match($spam_pattern, $radio)
            || !preg_match($spam_pattern, $checkbox)
            || !preg_match($spam_pattern, $select)
            || !preg_match($spam_pattern, $message)) {
            return '<b style="color: red">Invalid Request!</b>';
        }
    
  8. The $to variables and others you use as parameters for mail are only used once. Furthermore the $message variable is overwritten, removing the original content. Variable reuse makes stuff hard to track though. I just do this all as arguments in the mail function (snippet):

        $sendResult = mail(
            'support@loaidesign.co.uk', 
            'New Message From: ' . $name, 
            "Name: <br/>
               number: $number<br/>
               date: $date<br/>
               select: $select<br/>
               radio: $radio<br/>
               checkbox: $checkbox<br/>
               Email: $email<br/>
               Message: $message", 
            "MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
        );
    
  9. All those returns have their html-markup in common. We sure can extract this into a common helper:

    function getResponse($message, $isError) {
        return '<b style="color: ' . ($isError ? 'red' : 'green') . '">' . $message . '</b>';
    }
    
  10. Final remarks & further points of improvement:

    • Indention of three characters is unusual. Most times its two or four.
    • This can of course be further refactored: Especially creating local variables from $_POST is a point of repetation. Maybe it could be handled as an array. That'd make mass-checking (preg_match) easier: just loop over all input variables.
    • If desired, this could of course be moved into a dedicated mailer class
    • The functions depend on the global state ($_POST). A next step would be to pass the required input variables as argument (maybe an array :)). This would make this function reusable as well.
    • Returning formated strings limits reusability to HTML output. Throwing expections or error codes puts this into the responsibility of the caller, not the callee.

My final result:

function getPostValue($key) {
    return isset($_POST[$key]) ? $_POST[$key] : null;
}

function getResponse($message, $isError) {
    return '<b style="color: ' . ($isError ? 'red' : 'green') . '">' . $message . '</b>';
}

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }

    $_SESSION['token'] = '';

    $name = getPostValue('name');
    $email = getPostValue('email');
    $message = getPostValue('message');
    $number = getPostValue('number');
    $date = getPostValue('date');
    $select = getPostValue('select');
    $radio = getPostValue('radio');
    $checkbox = getPostValue('checkbox');

    if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
        $response = getResponse('Error', true);
    }

    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        return getResponse('Invalid Email Address!', true);
    }

    $spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
    if (!preg_match($spam_pattern, $name) 
        || !preg_match($spam_pattern, $number)
        || !preg_match($spam_pattern, $date)
        || !preg_match($spam_pattern, $radio)
        || !preg_match($spam_pattern, $checkbox)
        || !preg_match($spam_pattern, $select)
        || !preg_match($spam_pattern, $message)) {
        return getResponse('Invalid Request!', true);
    }

    $sendResult = mail(
        'support@loaidesign.co.uk', 
        'New Message From: ' . $name, 
        "Name: <br/>
           number: $number<br/>
           date: $date<br/>
           select: $select<br/>
           radio: $radio<br/>
           checkbox: $checkbox<br/>
           Email: $email<br/>
           Message: $message", 
        "MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
    );

    if (true === $sendResult) {
        return getResponse('Success! Your submission has been sent.', false);
    } else {
        return getResponse('Error! There was a problem with sending.', true);
    }
}
\$\endgroup\$
0
\$\begingroup\$

Your switch block makes my head hurt. It involves a true constant, non-constant case expressions (something that is forbidden in many other languages), fall-through cases, and negated regular expression matches — each of which would have hurt code clarity, but the combination is really hard to understand.

In the end, I distilled the code down to a simple test case to see what would happen.

<?php
$name = "Ha ha\r\nCc: victim@example.com";

$spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
switch (true) {
  case !preg_match($spam_pattern, $name):
    $response = "Invalid request";
    break;

  default:
    $response = "Sending mail to $name";
}

echo $response, "\n";

Running that produces

Sending mail to Ha ha
Cc: victim@example.com

I think that that indicates that your exploit-mitigation measure doesn't work.

Instead of restricting the inputs you accept, a better exploit countermeasure is to escape your header values using mb_encode_mimeheader(). According to RFC 2047, you ought to be encoding your header values anyway. As it happens such encoding will also protect you against CR-LF header injections.

\$\endgroup\$

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.