4
\$\begingroup\$

I have the following as my Ajax / PHP contact form on a site I am building. I have sanitized the inputs but just wanted to make sure I havent missed anything...

    // Only process POST reqeusts.
    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        // Get the form fields and remove whitespace.
        $name = strip_tags(trim($_POST["name"]));
        $name = str_replace(array("\r","\n"),array(" "," "),$name);
        $phone = trim($_POST["phone"]);
        $phone = filter_var($phone, FILTER_SANITIZE_NUMBER_INT);
        $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
        $subject = strip_tags(trim($_POST["subject"]));
        $subject = str_replace(array("\r","\n"),array(" "," "),$subject);
        $message = strip_tags(trim($_POST["message"]));
        $message = str_replace(array("\r","\n"),array(" "," "),$message);

        // Check that data was sent to the mailer.
        if ( empty($name) OR empty($phone)) {
            // Set a 400 (bad request) response code and exit.
            http_response_code(400);
            echo "Oops! There was a problem with your submission. Please complete the form and try again.";
            exit;
        }

        // Set the recipient email address.
        $recipient = "email@domain.com";

        // Set the email subject.
        $emailsubject = "New contact from $name";

        // Build the email content.
        $email_content = "Name: $name\n";
        $email_content .= "Email: $email\n\n";
        $email_content .= "Phone:\n$phone\n";
        $email_content .= "Subject:\n$subject\n";
        $email_content .= "Message:\n$message\n";

        // Build the email headers.
        $email_headers = "From: $name <$email>";

        // Send the email.
        if (mail($recipient, $emailsubject, $email_content, $email_headers)) {
            // Set a 200 (okay) response code.
            http_response_code(200);
            echo 'Thank You! Your message has been sent.';
        } else {
            // Set a 500 (internal server error) response code.
            http_response_code(500);
            echo "Oops! Something went wrong and we couldn't send your message.";
        }

    } else {
        // Not a POST request, set a 403 (forbidden) response code.
        http_response_code(403);
        echo "There was a problem with your submission, please try again.";
    }
\$\endgroup\$
  • \$\begingroup\$ Don't sanitize input, validate input. \$\endgroup\$ – Grumpy Jun 27 '17 at 18:10
2
\$\begingroup\$

The good news is that your script doesn't look exploitable. In particular, stripping out line break characters protects against header-splitting attacks. Good job there.

The bad news is that it can mangle the text unnecessarily. For example, if a user submits the form with the subject line

Your <blink> tag is annoying

… it will come across as

Subject: Your tag is annoying

And for what gain? strip_tags() is meant as a feeble defense against inappropriate HTML tags, but here you are sending plain text mail — a problem that has absolutely nothing to do with HTML.

It also probably does not make sense to strip all line termination characters from the message.


A more general concern I have is your use of the term sanitize, as it leads to confusion. I recommend striking that word from your programming vocabulary (even if the PHP documentation uses it), to be replaced by three specific terms:

  1. Canonicalization (or "Normalization"): transforming input from multiple representations of the same data into one preferred form.

    For example, if your form accepts a credit card number, then you should strip out all whitespace, because they are not a meaningful part of the data. Another example would be to lower-case an e-mail address.

  2. Validation: rejecting input that violates your rules. Validation failure should cause the user to have to resubmit after fixing the errors.

    For example, if your form accepts a credit card number, then you should reject any submission that does not contain the right number of digits, the correct leading digits for the accepted card types, and a correct Luhn checksum.

    For an e-mail form, your validation rules might require a name to be non-empty, a subject line that is one line of a reasonable length, and a plausible-looking e-mail address.

  3. Escaping: transforming strings so that one kind of string can be safely embedded inside another kind of string without being misinterpreted.

    Any time you compose a string that will be interpreted by another computer system, be it an e-mail message, HTML page, or SQL query, assume that you are vulnerable to some kind of injection attack. Every single one of these "human-friendly" languages (as opposed to binary formats such as JPEG images) will have delimiters of special significance. Header-splitting, HTML/JavaScript injection, and SQL injection attacks all have the same root cause: careless string concatenation or interpolation.

    Therefore, before concatenating or interpolating strings, stop and think: "What is the appropriate escaping mechanism that I should be using?" In the case of e-mail headers, RFC 2047 suggests that the relevant escaping mechanism is mb_encode_mimeheader(). Sometimes, the answer is that no escaping is required — the e-mail message body in this example is one of those rare occasions where that's true.

Canonicalization provides user-friendliness. Validation enforces your business logic. Escaping, not canonicalization or validation, upholds security. The term "sanitize" conflates the three mechanisms, leading you do write improperly engineered code.


Suggested solution

function canonicalize(&$params) {
    $params['email'] = filter_var($params['email'], FILTER_SANITIZE_EMAIL);
    $params['phone'] = filter_var($params['phone'], FILTER_SANITIZE_NUMBER_INT);
}

function validate(&$params) {
    $errors = array();
    if (empty($params['name'])) {
        $errors['name'] = 'A name is required';
    }
    if (empty($params['email'])) {
        $errors['email'] = 'Invalid e-mail address';
    }
    if (empty($params['phone'])) {
        $errors['phone'] = 'A phone number is required';
    }
    return $errors;
}

function recipient(&$params) {
    return "email@example.com";
}

function subject(&$params) {
    return mb_encode_mimeheader("New contact from " . $params['name'], 'UTF-8', 'Q');
}

function body(&$params) {
    // Don't bother escaping e-mail body; it's for human consumption.
    return sprintf(
        "Name: %s\n" .
        "Email: %s\n" .
        "Phone: %s\n" .
        "Subject: %s\n" .
        "Message:\n%s\n",
        $params['name'], $params['email'], $params['phone'],
        $params['subject'], $params['message']);
}

function headers(&$params) {
    return sprintf(
        "From: %s <%s>",
        mb_encode_mimeheader($params['name'], 'UTF-8', 'Q'),
        $params['email']
    );
}

// Only process POST reqeusts.
if ($_SERVER["REQUEST_METHOD"] != "POST") {
    // Not a POST request, set a 405 (Method Not Allowed) response code.
    http_response_code(405);
    echo "There was a problem with your submission, please try again.";
} else {
    canonicalize($_POST);

    if (($errors = validate($_POST))) {
        display_form($errors);
    } elseif (!mail(recipient($_POST), subject($_POST), body($_POST), headers($_POST))) {
        // Set a 500 (Internal Server Error) response code.
        http_response_code(500);
        echo "Oops! Something went wrong and we couldn't send your message.";
    } else {
        // Set a 200 (Success) response code.
        http_response_code(200);
        echo 'Thank You! Your message has been sent.';
    }
}

In the rewrite above, I also proposed:

  • Breaking up the code into functions.
  • Rearranging the flow such that all of the error handlers come first. (There are many ways to fail along the one true path to success.)
  • Changed the HTTP status code for non-POST requests to 405.
  • A stub (to be implemented) so that the user can get more informative feedback on validation failure.
\$\endgroup\$
1
\$\begingroup\$

I have sanitized the inputs but just wanted to make sure I havent missed anything

If I pass name=test@fake.com\nTo: spam@someone.com the newline will not be filtered out. You should use \\n instead of \n, or just nl2br.

Misc

  • return early to reduce nesting for your POST check the same way you do it for your $name check: if ($_SERVER["REQUEST_METHOD"] != "POST") { http_response_code(403); exit; }
  • be consistent with your variable names. You use underscore for most of them, so use it for emailsubject as well.
  • some of your comments aren't necessary, as they just repeat the code itself (such as // Only process POST reqeusts.).
\$\endgroup\$
  • \$\begingroup\$ I had not seen the filter_var functions before, thanks. \$\endgroup\$ – user602088 Mar 1 '15 at 18:05

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.