This test will look for common XSS mistakes with drupal_set_title and drupal_set_message.

My goal is to prevent things like

drupal_set_message($tainted_data);
drupal_set_message('Here is some '. $tainted_data);
drupal_set_message("Here is some $tainted_data");
drupal_set_message(t("Here is some ". $tainted_data));

and allow

drupal_set_message(t("Here is some @safe_data", array('@safe_data' => $tainted_data));
CommentFileSizeAuthor
#6 coder_328362.patch6.01 KBstella
#5 coder_328362_round_2.patch4.32 KBgreggles
#3 coder_328362.patch3.81 KBstella
#2 328362_common_set_failures.patch2.24 KBgreggles
common_set_failures.patch2.2 KBgreggles

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

This patch isn't perfect - it doesn't have knowledge of, for example:

$safe = check_plain($tainted_data);
drupal_set_message($safe);

But I think that's pretty uncommon anyway and is worth a second look by the maintainer.

However, perhaps the comment about the security test that "What it catches is good" should be changed if this patch is accepted.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 328362_common_set_failures.patch2.24 KB

chx pointed out that it's more important to have t( than it is to have an array. So, I added the t( check to the #never.

stella’s picture

stella commented
Status: Needs review » Needs work
StatusFileSize
new coder_328362.patch3.81 KB

The patch needs a bit more work as it is incorrectly flagging lines like drupal_set_title(check_plain($title));. I've attached a new version of the patch, which doesn't fix this issue, but adds in the beginnings of a series of tests for these new rules. It might be an idea to add more of these tests - it should help you verify the rules work in all scenarios.

Cheers,
Stella

stella’s picture

stella commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new coder_328362_round_2.patch4.32 KB

And here's an updated patch.

My feeling on this is that coder will never be able to properly categorize all the weaknesses here and so we should err on the side of showing some false positives that require human investigation rather than having false negatives (i.e. real weaknesses that are not identified as such). The current code will not identify a lot of things that are actually vulnerabilities, but it should be pretty good about not having false positives.

stella’s picture

stella commented
StatusFileSize
new coder_328362.patch6.01 KB

Here's a slightly modified version of the patch. I've added in the following changes:

  • Additional simple tests for both testing drupal_set_message() and drupal_set_title() - we need one for every scenario we can think (both good and bad) for both functions.
  • Added in format_plural() into list of allowed functions.
  • Changed regex so lines like function drupal_set_message(...) aren't matched.
  • Changed regex so the use of the t() ! placeholder gives a warning - we should alert on this if we're going to alert on drupal_set_title($message);
  • Changed regex so in addition to allowing t(), we also allow their counterparts st() and $t() ($t() is set by get_t())

Cheers,
Stella

Melissamcewen’s picture

Melissamcewen commented

Great idea!

I got a problem when applying the last patch:
patching file includes/coder_security.inc
patching file tests/coder_security.test
Hunk #1 FAILED at 5.
1 out of 3 hunks FAILED -- saving rejects to file tests/coder_security.test.rej

***************
*** 5,11 ****
function __construct($id = NULL) {
parent::__construct('security', $id);
}
-
function getInfo() {
return array(
'name' => t('Coder Security Tests'),
--- 5,11 ----
function __construct($id = NULL) {
parent::__construct('security', $id);
}
+
function getInfo() {
return array(
'name' => t('Coder Security Tests'),

Perhaps that didn't mean anything? I tested it on this module before I made the sanitized title patch and it didn't catch it.

stella’s picture

stella commented
Status: Needs review » Fixed

revised rules and tests committed to 6.x-2.x and 7.x branches.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

In here: http://drupal.org/cvs?commit=404008 and http://drupal.org/cvs?commit=404032

Thanks, Stella!

stella’s picture

stella commented

I strengthened the rules for drupal_set_message($tainted_data); and drupal_set_title($tainted_data); so it now checks the previous lines of the same function to see if the $tainted_data variable was ever sanitized. It should help reduce the false positives but won't eliminate them.

See http://drupal.org/cvs?commit=404380 and http://drupal.org/cvs?commit=404378

Still gives a false positive on the following line though because it doesn't know $error is an array:

drupal_set_message(t('%type: %message in %function (line %line of %file).', $error), $class);

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.