0

My PHP contact form was recently being used to send spam. Some security measures have since been put in place (please refer to the comments below) and I'm seeking the collective wisdom of others to review the code and to check to make sure it is secure from injection attacks.

Thank you in advance for taking the time to review.

<?php

/* method for validate each input values in case any injection scripts it will ignore */

function test_input($data) {
  $data = trim($data);
  $data = stripslashes($data);
  $data = htmlspecialchars($data);
  return $data;
}

/* honeypot - if hidden field is completed discard form content */

if(!isset($_POST['honeypot']) || $_POST['honeypot'] != '')
{
     die("You spammer!\n");
}
else
{
     // define variables and set to empty values
    $subject = $id = $subcategory = $subcategory = $subcategory_email = $to = $descError = $error =
    $remarks = $response= $message= $name = $from = $phone ="";

if(isset($_REQUEST['category']) && $_REQUEST['category']!="")
{
       //validate each input values for any injection attacks 
        $id = test_input($_REQUEST['category']);            
        $subcategory = test_input($_REQUEST['subcategory']);

             $emails = array
      (
      array("0",""),
      array("1","email1@yahoo.com","email2@yahoo.com"),
      array("2","email1@yahoo.com","email2@yahoo.com"),
      array("3","email1@yahoo.com","email2@yahoo.com"),
      array("4","email1@yahoo.com","email2@yahoo.com"),
      array("5","email1@yahoo.com","email2@yahoo.com")
      );
            $value = explode(",", $subcategory);                  
            $subcategory_email = $emails[$id][$value[0]];

            $remarks = test_input($_REQUEST['remarks']);

        $message = '<html><body>';
        $message .= '<table rules="all" style="border-color: #666;" border="1" cellpadding="10">';
        $message .= "<tr style='background-color:#F5F5F5;'><th width=25%>Heading </th><th width=75%>Content</th></tr>";
        $message .= "<tr><td><b>Category </b></td><td>".$category[$id-1]."</td></tr>";
        $message .= "<tr><td><b>SubCategory </b></td><td>".$value[1]."</td></tr>";
        $message .= "<tr><td><b>Comments</b></td><td><pre>".$remarks."</pre></td></tr>";                    


        if($response==0)
        {
             $name = test_input($_REQUEST['name']);  
            $from = test_input($_REQUEST['email']);

            if (!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/",$from)) 
            {
                $emailErr = "Invalid email format";
            }

             $phone = test_input($_REQUEST['phone']);
            $message .= "<tr><td><b>Would you like a response?  </b></td><td>Yes</td></tr>";

            $message .= "<tr><td><b>Name</b></td><td>".$name."</td></tr>";
            $message .= "<tr><td><b>E-Mail</b></td><td>".$from."</td></tr>";
            $message .= "<tr><td><b>Telephone</b></td><td>".$phone."</td></tr>";
        }
        else
        {
            $from = "noreply@test.com";
            $message .= "<tr><td><b>Would you like a response? </b></td><td>No</td></tr>";
        }

        $subject = "SubCategory ".$value[1];       
        //Normal headers
       $headers = "From: " . strip_tags($from) . "\r\n";
$headers .= "Reply-To: ". strip_tags($subcategory_email) . "\r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";

        $message .= "</table>";

       if(mail($subcategory_email, $subject, $message, $headers))
       {           
           include("thanks.php");
            $error=6;
       }
       else
       {
           echo "mail not sent";
       }
}
else
{
    echo "<br/>";
    $subject = "Sub Category";

    $to = "Email1@yahoo.com";        
    if(empty($_REQUEST['remarks']))
    {
      $descError = "Enter Description";   
      $error = 5;
    }
    else
    {
          $remarks = test_input($_REQUEST['remarks']);
    }               

   if(test_input($_REQUEST['response'])=="0")
    {       
        $yesDIV = "checked";
        $response = "Yes";
    if(empty($_REQUEST['name']))
    {
      $nameError = "Name Required"; 
        $error = 5;   
    }
    else
    {
        $name = test_input($_REQUEST['name']);
    }
    $from = $_REQUEST['email'];
    if(empty($_REQUEST['email']))
    {
      $emailError = "Email Required";
          $error = 5;     
    }
        else if (!filter_var($from, FILTER_VALIDATE_EMAIL)) {
    $emailError = "Valid Email Required";
          $error = 5;   
}
    }
    else
    {
      $noDIV = "checked";
      $response = "No";
      $bodyDIV = "style='display:none;'";
    }

if($error!=5)
{   
     $phone = test_input($_REQUEST['phone']);

    $message = '<html><body>';

        $message .= '<table rules="all" style="border-color: #666;" border="1" cellpadding="10">';
        $message .= "<tr style='background-color:#F5F5F5;'><th width=25%>Heading </th><th width=75%>Content</th></tr>";
        $message .= "<tr><td><b> Comments</b></td><td ><pre>".$remarks."</pre></td></tr>";    
    $message .= "<tr><td><b>Would you like a response?  </b></td><td>".$response."</td></tr>";

    $message .= "<tr><td><b>Name</b></td><td>".$name."</td></tr>";
    $message .= "<tr><td><b>E-Mail</b></td><td>".$from."</td></tr>";
    $message .= "<tr><td><b>Telephone</b></td><td>".$phone."</td></tr>";
    $message .= "</table>";
     //Normal headers
       $headers = "From: noreply@test.com \r\n";
$headers .= "Reply-To: ". strip_tags($from) . "\r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";

    if(mail($to, $subject, $message, $headers))
       {           
           include("thanks.php");
           $error=6;
       }
       else
       {
           echo "mail not sent";
       }
}
}
}
?>
formguy
  • 3
  • 2
  • 3
    This question appears to be off-topic and better suited for http://codereview.stackexchange.com – deceze May 07 '14 at 14:06
  • And there, to get a precise answer, you should say what you want to prevent. Inject what into what. – deviantfan May 07 '14 at 14:08
  • I recommend to you strip_tags as well. in test_input function. – Павел Иванов May 07 '14 at 14:08
  • try using a CAPTCHA, it's really the best way to keep these forms free from spam – fire May 07 '14 at 14:09
  • First off, injection attacks are SQL Injection Attacks. I feel like that is your concern you are asking for. Since I see no SQL queries here (there could be, it's early for me still), there's no "injection" attack. Secondly, this is better suited for codereview instead of stack overflow. Lastly, you need to use CAPTCHA's for forms like these. It stops a good majority of spammers. – lxndr May 07 '14 at 14:09
  • 1
    My two cents - Your `test_input()` function, which applies random escaping functions to random user input, suggests that you don't really understand the security vulnerability that lead to becoming a spam source. There's no way to fix something if you can't figure out what's broken. – Álvaro González May 07 '14 at 14:10
  • @lxndr There are all sorts of injection attacks. Here a *header injection* is a concern. – deceze May 07 '14 at 14:11
  • @lxndr - That's simply false. Ever heard about e.g. XSS? Mail header injections? The SQL language is not the only computer language on earth that's subject to code injection. – Álvaro González May 07 '14 at 14:12
  • @lxndr The spam was resolved with the honeypot technique (captcha is too instrusive). Now my goal is to make sure the form is not being manipulated in any way to send spam to any email addresses that are NOT specified in the mailer file. And you're right, there are no SQL queries but I read that a simple PHP form is still at risk of header injection. – formguy May 07 '14 at 14:13
  • @Álvaro G. Vicario I'm just hoping to get an overall assessment of the code; i.e. does it seem reasonable? It's not that easy to get to the bottom of what was causing the spam in the first place. – formguy May 07 '14 at 14:18
  • Given that you're stuffing `$name` and the like directly into your emails, yes, it's still "injectable". – Marc B May 07 '14 at 14:18
  • @Marc B: can you elaborate on that? – formguy May 07 '14 at 14:21

1 Answers1

4

The term "injection" refers to code injection, with code referring to any computer language. Since every computer language is different, the problems and solutions are also different and need to be addressed in a per-language basis. However, you have a generic function that tries to prevent all kind of injections at once and, often, using the worst technique: removing user data.

For instance:

$headers = "From: " . strip_tags($from) . "\r\n";

What sense does it make to take an e-mail address and remove HTML tags from it to compose an e-mail header?

$data = htmlspecialchars($data);

You apply this to e.g. $_REQUEST['email']. Why would you want to insert HTML entities in an e-mail address?

In your code I see two potential sources for injection:

  • HTML - When you inject user data into HTML you need to ensure that user data is handled as plain text (i.e. whatever the user typed is not rendered as HTML). You can use htmlspecialchars(). You kind of do that but it's really hard to be sure.

  • E-mail headers - mail()'s fourth argument allows to define mail headers. Injecting raw user input there (which is possibly what's happening now) allows to hide the complete message body, replace it with anything else and even select new recipients. You basically have to strip new lines (again, it's hard to say whether you're doing it right...).

Sending e-mail with PHP is hard. It's better to skip good old mail() and use a third-party library like PHPMailer or Swift Mailer.

Álvaro González
  • 128,942
  • 37
  • 233
  • 325