3
\$\begingroup\$

I recently had my site hacked, with the main index file being overwritten (nothing else was touched). I'm assuming they hijacked my form input.

Are there any vulnerabilities in the code or does this look to be safe now?

<?php
  $myemail = "...@gmail.com";
  $subject = "Form Submitted";
  $name = $_POST['name'];
  $email = $_POST['email'];
  $number = $_POST['number'];
  $comments = $_POST['comments'];
  $feedback = "";
  $message  = <<<EMAIL

Name:   $name
Email:  $email

Message:

$comments

EMAIL;


/////////////////////////////////////////////////////////////////////////////
//                          Form input validation
/////////////////////////////////////////////////////////////////////////////
if ($_SERVER["REQUEST_METHOD"] == "POST") {
  if (empty($_POST["name"])) {
    $nameErr = "Name is required";
    $feedback = "";
  } else {
    $name = test_input($_POST["name"]);
    // check if name only contains letters and whitespace
    if (!preg_match("/^[a-zA-Z ]*$/",$name)) {
      $nameErr = "Only letters and white space allowed";
      $feedback = "";
    } else {
      $nameErr = "";
    }
  }

  if (empty($_POST["comments"])) {
    $commentsErr = "Message is required";
    $feedback = "";
  } else {
    $comments = test_input($_POST["comments"]);
    $commentsErr = "";
  }
} // End input validation

/////////////////////////////////////////////////////////////////////////////
//                       Successful form submission
// Neither (name and nameErr) nor (comments and commentsErr) could possibly
// be empty strings. This prevents the form from being submitted as soon as
// the page is loaded. It also clears all fields when the form is successful.
// All field contents will remain until form is successfully submitted.
/////////////////////////////////////////////////////////////////////////////
if($commentsErr == "" && $nameErr == "" && $name != "" && $comments != ""){
  mail($myemail,$subject,$message);
  $feedback = "Thanks for contacting us ! We'll be in touch soon.";
  $_POST['name'] = "";
  $_POST['email'] = "";
  $_POST['number'] = "";
  $_POST['comments'] = "";
}

// Function strips any dangerous characters from input
function test_input($data) {
   $data = trim($data);
   $data = stripslashes($data);
   $data = htmlspecialchars($data);
   return $data;
}

?>
\$\endgroup\$
  • \$\begingroup\$ Speaking from experience, the mail() function in PHP is unreliable. It will sometimes send the email, sometimes it won't. Also, Email Providers do not like it and will sometime mark mail mail sent this way as Spam, or Block it entirely. I recently ran across a great company called MailGun. They are free up to 10,000 emails per month and implementation is fairly simple. Have a look to increase reliability in your system: mailgun.com \$\endgroup\$ – Dave Mar 1 '15 at 10:20
  • \$\begingroup\$ Cool, I'll check them out. I know I don't get anywhere near 10k even yearly lol. \$\endgroup\$ – travisw Mar 1 '15 at 11:39
  • \$\begingroup\$ THen you should be fine. Sending mail is fairly easy. You will need composer, but that is the only requirement. I don't know if you have heard of Alex from PHPAcademy but he has a great tut on youtube. youtube.com/playlist?list=PLfdtiltiRHWFxboMVRMelHYreAJO73DPe \$\endgroup\$ – Dave Mar 1 '15 at 17:48
3
\$\begingroup\$

I don't see vulnerabilities here:

  • The script is not executing shell commands
  • The script is not executing database queries
  • The only "external" call is to mail(...), the arguments are reasonably sanitized, and in any case I would expect the mail(...) function to do its own sanitization, and not erase the hard disk if something really evil is embedded $message

But I see some dangerous practices.

You create $message from input that's not yet validated:

  $name = $_POST['name'];
  $email = $_POST['email'];
  $comments = $_POST['comments'];
  $feedback = "";
  $message  = <<<EMAIL

Name:   $name
Email:  $email

Message:

$comments

EMAIL;

At this point all the $_POST fields are not validated yet. I don't know if this is intentional, for example because you want to preserve the original inputs instead of using the sanitized ones. This is a bit confusing.

The script uses $_POST["something"] repeatedly. It would be better to create cleaned copies early on, and work with the cleaned versions, for example:

$cleaned_name = clean_input($_POST['name']);
$cleaned_email = clean_input($_POST['email']);
$cleaned_comments = clean_input($_POST['comments']);

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

test_input was not a good name for this purpose, so I renamed it to clean_input.

I find it odd that you clear the values of the $_POST:

  mail($myemail,$subject,$message);
  $feedback = "Thanks for contacting us ! We'll be in touch soon.";
  $_POST['name'] = "";
  $_POST['email'] = "";
  $_POST['comments'] = "";

This seems to suggest that you don't trust the rest of the program might use these values by accident. It would be better to reorganize your code to render such accidental uses impossible, something you can trust, so you don't need to write paranoid code.

For example it would be better to reorganize your code to functions, and have a better control of the flow of the program and the actions taken. A common practice is to use a Form class, so that you can have a main flow like this:

$form = new Form;
if ($form->is_valid()) {
    // send email, taking values from $form->cleaned_data
} else {
    // print errors, taking values from $form->errors
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the input. I've incorporated the "cleaned_" variable names and function name for consistency. The reason I clear the values of the $_POST is to delete the information from the screen so that the user is assured that their contact form has been sent. There's inline PHP on the HTML side that makes the information persistent: <?php if (isset($_POST['name'])) echo 'value="'.$_POST['name'].'"';?>> This is to keep the form from clearing if the user clicks submit but the form is filled improperly. \$\endgroup\$ – travisw Mar 1 '15 at 3:51
  • 1
    \$\begingroup\$ @travisw don't do that. If you echo unsanitized user input, your code will be open to XSS attacks, which can be used to phish you, steal cookies, deface your website, log keystrokes, etc. When echoing user input, always prevent against xss. Also, your code is probably open to email injection. \$\endgroup\$ – tim Mar 10 '15 at 23:31
  • \$\begingroup\$ @tim So, do I want to set those values equal to the sanitized versions of the variables in order to prevent that? Should I alter the function to be something like Darik suggested? \$\endgroup\$ – travisw Mar 11 '15 at 13:00
  • 1
    \$\begingroup\$ @travisw yes, that seems like an okay approach. I would use ENT_QUOTES as well (it's not needed in this case, but if you ever use single quotes instead of double quotes for values it will be needed). Trim and stripslashes aren't actually needed though, so your code could just look like this: <?php if (isset($_POST['name'])) echo 'value="'.htmlspecialchars($_POST['name'], ENT_QUOTES, 'UTF-8').'"';?>>. \$\endgroup\$ – tim Mar 11 '15 at 13:28
1
\$\begingroup\$
<?php

    //@author Darik


    $myemail = "...@gmail.com";
    $subject = "Form Submitted";
    $feedback = "";
    $commentsErr = '';
    $nameErr = ''; 



    //                          Form input validation
    if (isset($_POST["name"])) {
        $name = sanitize($_POST["name"]) ?:         $nameErr= "Your name is required";
        ctype_alpha($name) ?:                       $nameErr = "Only letters and white space are allowed";  
        $comments = sanitize($_POST["comments"]) ?: $commentsErr= "Please type your massage";
    }

    if(empty($commentsErr) && empty($nameErr)){  
         //I'm not sure about this area but i moved $message  down cos it needs to take $name & $comments values
         $message  = '';

        mail($myemail,$subject,$message);
        $feedback = "Thanks for contacting us ! We'll be in touch soon.";
        unset($_POST);
    }

    // Function strips any dangerous characters from input
    function sanitize($data) {
       $data = trim($data);
       $data = stripslashes($data);
       $data = htmlspecialchars($data);
       return ($data)?: false;
    }

?>

I wouldn't say I've made your code totally secure since I'm not a security expert but I'd say I've made your code more optimized and improved its security by avoiding unwanted round trips;

The function 'test_input' which I renamed to sanitize returns false so no need for all those if else processes

I've replaced preg_match with php's native ctype_alpha() function

for mailing part use PhpMailer library, download it from github, they will take care of most mailing related problems

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