Posted by greggles on October 30, 2008 at 11:49pm
| Project: | Coder |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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));| Attachment | Size |
|---|---|
| common_set_failures.patch | 2.2 KB |
Comments
#1
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.
#2
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.
#3
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
#4
#5
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.
#6
Here's a slightly modified version of the patch. I've added in the following changes:
format_plural()into list of allowed functions.function drupal_set_message(...)aren't matched.t() !placeholder gives a warning - we should alert on this if we're going to alert ondrupal_set_title($message);t(), we also allow their counterpartsst()and$t()($t() is set by get_t())Cheers,
Stella
#7
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.
#8
revised rules and tests committed to 6.x-2.x and 7.x branches.
#9
In here: http://drupal.org/cvs?commit=404008 and http://drupal.org/cvs?commit=404032
Thanks, Stella!
#10
I strengthened the rules for
drupal_set_message($tainted_data);anddrupal_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);#11
Automatically closed -- issue fixed for 2 weeks with no activity.