1
\$\begingroup\$

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.

<?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";
       }
}
}
}
?>
\$\endgroup\$
  • \$\begingroup\$ doing a security audit of this code is a bit difficult, as it features inconsistent formatting and a high cyclomatic complexity. Consider improving the formatting, and splitting the code into multiple helper functions that can be reviewed for security independently. It would be also good to use some naming convention to mark which values have not yet been validated and sanitized. \$\endgroup\$ – amon May 7 '14 at 17:15
  • \$\begingroup\$ @amon I'm just looking to get a general sense. i.e. Does anything "pop out" at you? \$\endgroup\$ – formguy May 7 '14 at 17:25
  • 1
    \$\begingroup\$ The problem is, the code is too messy for anything to "pop out". Issues that might be obvious in cleaner code are hidden in the spaghetti. \$\endgroup\$ – Andrew Medico May 8 '14 at 1:27
  • \$\begingroup\$ Unless you're implementing this for learning purposes, I'd suggest swiftmailer or another such open-source library. It has already been fine tuned and looked over for security holes. No need to reinvent the wheel! \$\endgroup\$ – Matthew Johnson May 9 '14 at 19:34
1
\$\begingroup\$

Some things I've noticed:

  • subcategory_email might be empty if a user would supply an index out of the array bounds.
  • Why use $_REQUEST? It's almost always better to use $_GET or $_POST if you know what to expect (POST would probably be best for a 'committing' action).
  • I think it's weird to have a function called test not return true or false, but modify the value. I would call test_input, cleanup_value or something like that instead.
  • strip_tags($from) seems a bit excessive as you've already validated it.
  • You don't have to validate email address yourself. PHP has a built in function filter_var($email, FILTER_VALIDATE_EMAIL);
  • The operations in test_input seem a bit excessive,

I did not look at code style, etcetera, but you should make an effort to clean it up a bit. It makes problems in general a lot easier to spot. I also did not look at HTML code.

You're also spoofing the from address in some cases, this might make your email end up in the spam folder or even not get delivered at all if the user has strict DMARC checks on their email domain. It might be better to set from to your email address, but instead set Reply-To to the user's address.

\$\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.