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","[email protected]","[email protected]"),
      array("2","[email protected]","[email protected]"),
      array("3","[email protected]","[email protected]"),
      array("4","[email protected]","[email protected]"),
      array("5","[email protected]","[email protected]")
      );
            $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 = "[email protected]";
            $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 = "[email protected]";        
    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: [email protected] \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\$
4
  • \$\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, 2014 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, 2014 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\$
    – nobody
    May 8, 2014 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, 2014 at 19:34

1 Answer 1

Reset to default
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 and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.