HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Contact Form Sanitized Inputs

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactforminputssanitized

Problem

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 ";

// 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 (

Solution

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 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:

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

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

-
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 ",
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 you

Code Snippets

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.';
    }
}

Context

StackExchange Code Review Q#82361, answer score: 4

Revisions (0)

No revisions yet.