patternphpMinor
Contact Form Sanitized Inputs
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 (
```
// 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
… it will come across as
And for what gain?
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
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
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 annoyingAnd 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.