Problem: I could not find any documentation about whether Drupal developers should sanitize email bodies and subjects to prevent XSS when the mail is read on a mail client.
I ran into this problem a couple of times, but never found the time to fully explore it. From my understanding it is the responsibility of the displaying email client to make sure that no evil JavaScript is executed in HTML mails. Which means that Drupal should not run filter_xss(), check_plain() and friends before passing data to the message transfer agent.
We assume that HTML mails are enabled, see http://drupal.org/node/900794
Example which is wrong:
<?php
function mymodule_mail($key, &$message, $params) {
$message['subject'] = 'News: ' . check_plain($params['user_supplied1']);
$message['body'][] = '<html><table><td><tr>' . check_plain($params['user_supplied12']) . '</td></tr></table></html>';
}
?>Example which is correct:
<?php
function mymodule_mail($key, &$message, $params) {
$message['subject'] = 'News: ' . $params['user_supplied1'];
$message['body'][] = '<html><table><td><tr>' . $params['user_supplied12'] . '</td></tr></table></html>';
}
?>I'd like to create a documentation page in http://drupal.org/writing-secure-code that basically says "XSS in outgoing emails not considered a weakness"
I'm not 100% sure about this, so I would like to hear your input. Some links:
http://security.stackexchange.com/q/12568 (read all comments!)
Comments
I would definitely think that
I would definitely think that the functions filter_xss() and check_markup() should be typically used? check_plain() should be avoided since it would convert things that would have HTML into un-renderable plain text that e-mail clients would not be able to display as intended.
Senior Drupal Developer for Lullabot | www.davereid.net | @davereid
Right, my mistake in the
Right, my mistake in the example. Changed it so that check_plain() is used embedded as usual.
I guess check_markup() and filter_xss() are a different use case here, since they also strip out unwanted tags. So that could be a legitimate case for emails.
I agree check_markup is
I agree check_markup is potentially useful, but I don't see how/why filter_xss would be appropriate on its own.
knaddison blog | Morris Animal Foundation
Erm. Let's step back. When
Erm. Let's step back.
When you output some plain-text value in HTML, you have to use
check_plain(). It is not about security, but simply about correctness. Many characters have to be encoded when output in HTML because they have special meanings (mainly<,>and&).On the other hand,
filter_xss()andfilter_xss_admin()are security mechanisms that can be avoided in some specific cases. Those specific cases are few and far apart, so I would recommend not trying to avoid them.Note that, since Drupal 7, the emails are always prepared in HTML. They are only (by default) downcased to plain-text just before sending. You should apply the same rules to email that you apply to outputting any HTML.
Damien Tournoud
IMO the question is: Does it
IMO the question is: Does it break anything to apply the same rules that are applied for HTML sent to a browser to the HTML sent to a MUA? I do not think that it does.
If this is correct, we should ask people to apply these rules anyway, just for consistency and so that people get used to think about output sanitation all the time.
We would probably not issue SAs for any missing output sanitation in email sending code.
So if I follow the answers
So if I follow the answers here, it seems we should add something on https://www.drupal.org/node/28984 and/or the phpdoc for drupal_mail()/hook_mail() mentioning that messages MUST be sanitized using check_plain(), should we not ?