4
\$\begingroup\$

I have the following contact form, using PHP, JS and a bit of Ajax. I want to make sure that it is secure.

<?php
session_start();
if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
    ob_start();
        if(isset($_POST['name']) && isset($_POST['email']) && isset($_POST['message']) && isset($_POST['token']))
        {
            if($_SESSION['token'] != $_POST['token'])
            {
                $response = "0";
            }
            else
            {
                $_SESSION['token'] = "";

                $name = $_POST['name'];
                $email = $_POST['email'];
                $message = $_POST['message'];

                $to = "[email protected]";
                $subject = "New Message From: $name";
                $message = "$message";

                $headers  = 'MIME-Version: 1.0' . "\r\n";
                $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
                $headers .= 'From: '.$email . "\r\n";
                $mailed = ( mail($to, $subject, $message, $headers) );

                if( isset($_POST['ajax']))
                    $response = ($mailed) ? "1" : "0";
                else
                    $response = ($mailed) ? "<h2>Success!</h2>" : "<h2>Error! There was a problem with sending.</h2>";

                echo $response;
            }
        }
        else
        {
            echo "Form data error!";
        }
    ob_flush();
    die();
}
?>
<!DOCTYPE html>
<html class="no-js">

    <head>
        <title>Contact us | Website Name</title>
        <meta name="description" content="">
        <?php include ("assets/includes/header.inc"); ?>
    </head>

    <body>
        <header id="headerWrapper">
            <div id="headerContent">

                <div id="headerLogo"><!--Logo-->
                    <?php include ("assets/includes/header-logo.inc"); ?>
                </div>

                <nav><!--Main Menu-->
                    <ul id="mainMenu">
                        <?php include ("assets/includes/menu.inc"); ?>
                    </ul>
                </nav>

            </div>
        </header>

        <!--Header (small screens only)-->
        <?php include ("assets/includes/second-header.inc"); ?>

        <div id="page"><!--Page Container-->

            <div id="contactPage" class="wrapper">
                <div class="content">

                    <!--Contact Form-->
                    <?php
                        $token = md5(uniqid(rand(), TRUE));
                        $_SESSION['token'] = $token;
                    ?>
                    <form id="contactForm" action="contact.php" method="post" name="contactForm">
                        <input id="token" type="hidden" value="<?php echo $token; ?>" />

                        <div class="name">
                            <label>Your Name</label>
                            <input id="name" type="text" placeholder="Enter Name" required>
                        </div>

                        <div class="email">
                            <label>Email Address</label>
                            <input id="email" type="email" placeholder="Enter Email" required>
                        </div>

                        <div class="message">
                            <label>Message</label>
                            <textarea id="message" required></textarea>
                        </div>

                        <button id="submit" type="submit">Send</button>

                    </form>
                    <script type="text/javascript">
                    $("#contactForm").submit(function(event) {

                        /* stop form from submitting normally */
                        event.preventDefault();

                        /* get some values from elements on the page: */
                        var $form = $(this),
                            $submit = $form.find('button[id="submit"]'),
                            token_value = $form.find('input[id="token"]').val(),
                            name_value = $form.find('input[id="name"]').val(),
                            email_value = $form.find('input[id="email"]').val(),
                            message_value = $form.find('textarea[id="message"]').val(),
                            url = $form.attr('action');

                        /* send the data using post */
                        var posting = $.post(url, {
                            token : token_value,
                            name : name_value,
                            email : email_value,
                            message : message_value,
                            ajax : 1
                        });

                        posting.done(function(data) {
                            $form.find('span.error').remove();

                            if (data == "1") {

                                /*Change the button text.*/
                                $submit.text('Sent. Thank You!');
                                $submit.addClass("sent");
                                $("#submit").attr('disabled', 'disabled');

                            } else {
                                $submit.after('<span style="display: inline-block; padding: 20px 5px; color: #bd3d3d" class="error">Failed to send the message, please try again later.</span>');
                                /*Change the button text.*/
                                $submit.text('Try Again');
                            }
                        });
                    });
                    </script>

                    <!--Contact Details Section-->
                    <div id="contactDetails">

                        <div>
                            <h3><strong>Contact Details</strong></h3>
                                <br>
                            <p><strong>Telephone:</strong> +44 00 0000000</p>
                            <p><strong>Email:</strong> [email protected]</p>

                            <!--Phone contact buttons-->
                            <div class="hidden">
                                <a href="tel:0044000000000">Tap to call us</a>
                                <a href="sms:0044000000000">Tap to send us a SMS</a>
                            </div>
                        </div>

                        <!--Social Icons-->
                        <br><div class="socialbar">
                            <p><strong>Find us on:</strong></p>
                            <a href="#" target="blank" class="icon-facebook"></a>
                            <a href="#" target="blank" class="icon-linkedin"></a>
                            <a href="#" target="blank" class="icon-twitter"></a>
                            <a href="#" target="blank" class="icon-youtube"></a>
                        </div>
                    </div>

                </div>
            </div>

            <!--Footer-->
            <footer id="footer">
                <?php include ("assets/includes/footer.inc"); ?>
            </footer>

        </div><!--The End Of The Page-->
        <!--Scripts Links-->
        <?php include ("assets/includes/scripts.inc"); ?>
    </body>
</html>
\$\endgroup\$
1
  • \$\begingroup\$ Why do you have quotes around your integers? \$\endgroup\$
    – user555
    Commented Jan 23, 2014 at 20:21

2 Answers 2

Reset to default
3
\$\begingroup\$

No, I've noticed several issues.

Make sure the stuff in assets/includes/ isn't world readable.

e.g. is it possible to view this code by going to www.example.com/assets/includes/second-header.inc (substituting in your domain name of course)?

Also, don't use rand() to generate tokens for secure use. rand() will generate predictable values that can be guessed by an attacker.

The manual states:

Caution This function does not generate cryptographically secure values, and should not be used for cryptographic purposes. If you need a cryptographically secure value, consider using openssl_random_pseudo_bytes() instead.

You need cryptographically secure values for use as tokens.

Also, make sure that the email you are entering in the headers

  $headers .= 'From: '.$email . "\r\n";

has new lines and carriage return characters stripped from it to prevent Email Header Injection.

Otherwise you're good to go.

\$\endgroup\$
2
\$\begingroup\$

The form seems to be OK, but mixing all of the code in one file is not a best-practice.

UPDATE

Some tips on code separation:

  • I would suggest using any template system for html/php code separation. For example Smarty.
  • Also, it would be easier for you to maintain your code if you follow the MVC pattern.
  • Moving the js code to a separate file and loading it would also separate you js logic from template.
  • As @SilverlightFox mentioned, make sure you protect you .inc file. This could be done like in this answer of mine.

Some other tips:

  • I would rather use $token = md5(time()); than $token = md5(uniqid(rand(), TRUE));
  • Using ob_start(); ... ob_end_flush(); functions is often confusing as it prevents other files from rendering any output.
  • Always use curly brackets for if constructions: if (isset($_POST['ajax'])) { ... } else { ... }. Otherwise you might confuse the logic/flow of the application when you'll be re factoring your own code like in 6 month, for example.
\$\endgroup\$
2
  • \$\begingroup\$ A note on MD5: Since the 90s issues have been found with that algorithm. Do not use it for anything else than interfacing with legacy systems. To checksum something, the SHA family is often a better solution. To scramble some secret, use a key-stretching function like bcrypt. Using md5(time()) as a token produces predictable tokens, and can issue multiple identical tokens – this is most obvious if you think of it as a concurrency bug. This is actually far worse in any respect than uniqid rand. Otherwise, nice review: +1 \$\endgroup\$
    – amon
    Commented Jan 17, 2014 at 18:14
  • \$\begingroup\$ @amon, this is a contact form, not a bank vault. I suppose this would be enough. \$\endgroup\$
    – t1gor
    Commented Jan 17, 2014 at 18:17

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.