2
\$\begingroup\$

I'm new to hand coding a PHP form/email submission. Are the security checks I've included here good enough? Or is this super vulnerable to attacks?

My main concern is protecting the email login credentials below. Any feedback much appreciated.

Thank you!

   // IF WE'VE SUBMITTED A FORM
        if(
            !empty($_POST) &&
            isset( $_POST['moveDate'] ) &&
            isset( $_POST['email'] ) && !empty( $_POST['email'] ) &&
            isset( $_POST['emailConfirm'] ) && !empty( $_POST['emailConfirm'] ) &&
            $_POST['email'] === $_POST['emailConfirm'] &&
            filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)
        ) {
            // Make sure there are no fields posted that are not in the white list
            foreach ($_POST as $key=>$item) {
                if (!in_array($key, $whitelist)) {
                    die('<div class="validation-message warning">Please use only the fields in the form</div>');
                }

                if( $_POST[$key] && is_array($_POST[$key]) ) {
                    if(count($_POST[$key]) > 5) {
                        die('<div class="validation-message warning">Please use only the fields in the form</div>');
                    }
                }
            }

            $form_output = array();

            $move_date = $_POST['moveDate'];
            $move_type = $_POST['moveType'];
            $whered = $_POST['wheredYouFindUs'];
            $name = $_POST['moveName'];
            $email = $_POST['email'];
            $phone = $_POST['phone'];

            $multistop_type = '';

            if( !empty($_POST['multistopType']) ) { 
                $multistop_type = ' / ' . $_POST['multistopType'];
            }

            $form_output[] = '<strong style="color: #666 !important">Move Date:</strong> ' . $move_date;
            $form_output[] = '<strong style="color: #666 !important">Move Type:</strong> ' . $move_type . $multistop_type;

            $form_output[] = '<strong style="color: #666 !important">Name:</strong> ' . $name;
            $form_output[] = '<strong style="color: #666 !important">Email:</strong> ' . $email;
            $form_output[] = '<strong style="color: #666 !important">Phone:</strong> ' . $phone;
            $form_output[] = '<br><hr>';

            $pickup1 = '';
            $pickup2 = '';
            $pickup3 = '';
            $dropoff1 = '';
            $dropoff2 = '';
            $dropoff3 = '';

            $pickup1_stairs = '';
            $pickup2_stairs = '';
            $pickup3_stairs = '';

            $dropoff1_stairs = '';
            $dropff2_stairs = '';
            $dropoff3_stairs = '';

            $pickup1_dropoff1Items = '';
            $pickup2_dropoff1Items = '';
            $pickup3_dropoff1Items = '';
            $pickup1_dropoff2Items = '';
            $pickup1_dropoff3Items = '';

            $pickup1_dropoff1List = '';
            $pickup2_dropoff1List = '';
            $pickup3_dropoff1List = '';
            $pickup1_dropoff2List = '';
            $pickup1_dropoff3List = '';

    // PICKUP AND DROPOFF ADDRESSESS + STAIRS
            include('assets/includes/quote-email-addresses.php');
            $form_output[] = '<br><hr>';

    // ITEMS
            include('assets/includes/quote-email-items.php');

            $form_output[] = '<hr>';
    // COMMENTS     
            if(isThere($_POST['comments'])) {
                $form_output[] = '<strong style="color: #666 !important">Additional Items/Comments:</strong><br> ' . wordwrap (  stripcleantohtml( strip_tags( $_POST['comments'] ) ), 90, "<br>" ) . '<br>';           
            }

            $form_output[] = '<strong style="color: #666 !important">Inventory:</strong><br>' . implode('<br>', $inventory);
            $form_output[] = '<br><strong style="color: #666 !important">Total Quote</strong> = $' . $price . '<br>';

            if($whered !== '' && !empty($whered)) {
                $form_output[] = '<strong style="color: #666 !important">How did you find us? </strong> ' . $whered;        
            }

            $breaks = array("\r\n", "\n", "\r");
            $email_output = strip_tags( implode('<br>', $form_output), '<em><br><hr><strong><table><tr><td>');      
            $email_output = str_replace($breaks, "", $email_output);

            require("class.phpmailer.php");

            $a52D22xux = new PHPMailer();

            $a52D22xux->IsSMTP();
            $a52D22xux->SMTPSecure = "tls";
            $a52D22xux->Host = 'smtp.gmail.com';
            $a52D22xux->Port = 587;
            $a52D22xux->SMTPAuth = true;
            $a52D22xux->Username = '***';
            $a52D22xux->Password = '***';

            $a52D22xux->From = $email;
            $a52D22xux->FromName = $name;
            $a52D22xux->AddAddress('***');
            $a52D22xux->AddReplyTo($email, $name);

            $a52D22xux->IsHTML(true);

            $a52D22xux->Subject = 'Quote Request ' . date('m/d H:i');

            $a52D22xux->Body = sanitize_output( $email_output );

            $a52D22xux->Send();     
    ?>

                            <div class="inner">
                                <div class="feedback-success feedback pr tac">
                                    <h2 class="futura">Success!</h2>
                                    <p>Good news! Your quote request has been submitted successfully.</p>
                                </div>
                            </div>
                        </div>
                    </section>  

    <?php
        } else {
        // Show the form
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
  1. The whitelist check is giving a false sense of security. Consider its use case: There is no way a normal user will ever submit more parameters than those which are in the form. Therefore, you can safely assume that anyone submitting other parameters is an attacker, who should receive as little information as possible. The fact that you check parameters against a whitelist is information that could conceivably be used by an attacker to focus their efforts. Rather than a check, you could use the $whitelist to assign POSTed values to a "safe" dictionary and empty $_POST after that to ensure that the code doesn't ever touch any other values.
  2. You want to simplify security-related code to the max, to avoid any bugs. For example, make sure you get rid of any variables you are not using (such as pickup1_dropoff2List).
  3. You should never store credentials in code. Use PKI if you can, and store credentials (or references to them) in configuration files.
  4. Put warning verbosity to the max, and verify that you don't get any errors or warnings from the code. For example, isset( $_POST['moveDate'] ) should probably be changed to array_key_exists('moveDate', $_POST).
  5. Any user input must be escaped on output. You should probably use htmlspecialchars (but check for any caveats) for the form output.
\$\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.