Role names are plain text and should be checked with check_plain()
Heine - May 25, 2009 - 19:58
| Project: | Content Access |
| Version: | 6.x-1.1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Justin C. Klein Keane reported that content_access does not check_plain role names before displaying them on the 'Access Control' screen of managed content types.
We do not consider this a vulnerability because this can only be exploited by users with the "administer permissions" permission. As users with the "administer permissions" can already elevate their permissions via normal means, an XSS attack is just a more complicated way of doing something that is already possible.
It is a bug however; As role names are plaintext, they cannot be passed as-is in HTML. If you do not check_plain rolenames you may end up with invalid HTML or break something (& vs. &).
#1
Is this duplicate with this? #316136: New role name not filtered into admin/user/permissions
If not, it's certainly related.
#2
Related yes, duplicate no, because this is about output from content access.
#3
Attached patch should address the issue in 5.x-1.5. Thanks.
#4
Attached patch should address the issue in 6.x-1.1. Thanks.
#5
Changing status from active to patch needs work. Haven't tried the patches, but they need to be cleaned up according to Drupal coding standards for control structures.
#6
I agree they should be changed to follow the Drupal coding standards; in the specific case, the block of code executed from the foreach statement should be in a new line.
--- sites/all/modules/content_access/content_access.admin.inc 2009-03-17 12:04:15.000000000 -0400
+++ sites/all/modules/content_access/content_access.admin.inc 2009-06-01 15:21:43.000000000 -0400
@@ -249,6 +249,7 @@ function content_access_role_based_form(
}
$roles = user_roles();
+ foreach ($roles as $key => $val) {
$roles[$key] = check_plain($val);
}
// Per type:
$form['per_role'] = array(
'#type' => 'fieldset',
#7
As core doesn't check_plain() either I don't think it makes much sense doing so in contrib. So I just fixed it to go through filter_xss_admin().
#8
If core makes a mistake (see #316136: New role name not filtered into admin/user/permissions), there's no reason to repeat it. Rolenames are not HTML and need to be converted by check_plain to get valid HTML.
#9
I agree with you that 'check_plain' would be the best option, however as we discussed in IRC I think the strings should be handled in a consistent way. Thus I use filter_xss_admin() just to be safe but follow the (bad) way of core to display HTML. If core changes to use check_plain() I'm happy to follow that.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.