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. &).

CommentFileSizeAuthor
#4 content_access_6.x-1.1.patch447 bytesJustin_KleinKeane
#3 content_access_5.x-1.5.patch500 bytesJustin_KleinKeane
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Is this duplicate with this? #316136: New role name not filtered into admin/user/permissions

If not, it's certainly related.

Heine’s picture

Heine CreditAttribution: Heine commented

Related yes, duplicate no, because this is about output from content access.

Justin_KleinKeane’s picture

Justin_KleinKeane CreditAttribution: Justin_KleinKeane commented
Version: 6.x-1.1 » 5.x-1.5
FileSize
content_access_5.x-1.5.patch500 bytes

Attached patch should address the issue in 5.x-1.5. Thanks.

Justin_KleinKeane’s picture

Justin_KleinKeane CreditAttribution: Justin_KleinKeane commented
Version: 5.x-1.5 » 6.x-1.1
FileSize
content_access_6.x-1.1.patch447 bytes

Attached patch should address the issue in 6.x-1.1. Thanks.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Status: Active » Needs work

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.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno commented
Title: rolenames are plaintext and should be check_plained » Role names are plain text and should be checked with check_plain()

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',
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs work » Fixed

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().

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Fixed » Needs work

As core doesn't check_plain() either I don't think it makes much sense doing so in contrib.

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.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs work » Fixed

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.

Status: Fixed » Closed (fixed)

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