Download & Extend

New role name not filtered into admin/user/permissions

Project:Drupal core
Version:6.x-dev
Component:user.module
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs work
Issue tags:Security improvements

Issue Summary

If you add a javascript as new name of a role, this will be executed into admin/user/permission page.
I assume this is not a security hole because it's inside admin area.

Comments

#1

Version:6.4» 7.x-dev

This should be fixed, but is not really a security hole - to add new roles requires 'administer permissions' which already means you essentially can escalate your own permissions without limit.

#2

Status:active» needs review

Catching it in the user_roles() function also gets most places it's displayed - but might lead to double-encoding.

AttachmentSizeStatusTest resultOperations
role-names-filter-316136-2.patch1.55 KBIdlePassed: 11305 passes, 0 fails, 0 exceptionsView details | Re-test

#3

#4

Category:bug report» task
Status:needs review» needs work

This will break any select lists using the output of the user_roles() function, so guess we would need to track down uses.

However, it would perhaps make more sense to just validate on input as we do for translations.

#5

Right. We're back to the evil checkboxes are output unescaped, while select lists are escaped by FAPI. Ugh. Given that, I think we really do have to continue to output potentially malicious text from user_roles(), and check the call sites 1-by-1 to fix this.

I think it's reasonable to consider validation-on-input, too, but then it seems like we need a DB update to check for existing roles that aren't valid. Not sure what exactly to do with them during the DB update in that case. :( Given that, we should probably stick with escape-on-output...

While we're grepping, we should also make sure no one does anything stupid with $user->roles directly. The hunk in user_edit() is the only spot I can think of where we do anything with that, but there might be others, so we should check.

I'm *really* tempted to call this "minor". If you can't trust the users that have 'administer permissions', you're already in deep trouble.

#6

Is check_plain the right filter here? The issue we're trying to fix is XSS. Why not filter_xss_admin?

Validation on input goes against our overall filtering practice. It also won't save us when some contrib/install profile creates a role name based on user input. We filter on output.

#7

Because rolenames are plaintext (would you expect tags in them? (no) Ampersands ? (yes)), they need to be check_plained to get even valid HTML.

#8

Also from Handle text in a secure fashion:

Plain-text
This is simple text without any markup. What the user entered is displayed exactly on screen as is, and is not interpreted in any form. This is generally the format used for single-line text fields.

When outputting plain-text, you need to pass it through check_plain() before it can be put inside HTML. This will convert quotes, ampersands and angle brackets into entities, causing the string to be shown literally on screen in the browser.

#9

Category:task» bug report
Status:needs work» needs review

I think this patch might get all of them.

And this really should be considered a bug, since it prevents e.g. contrib modules from (safely) allowing less-privileged administrators to edit the role names -- which is something they might reasonably want to do.

AttachmentSizeStatusTest resultOperations
user-roles-check-plain-316136-9.patch4.77 KBIdlePASSED: [[SimpleTest]]: [MySQL] 18,740 pass(es).View details | Re-test

#10

Priority:normal» critical

Is this critical for D7 stable? I guess so...

#11

Priority:critical» normal

This does not seem critical, as it's a 'root user can hack himself' type of vulnerability.

#12

Let's just get this fixed and committed.

+++ modules/block/block.admin.inc 21 Mar 2010 20:14:37 -0000
@@ -313,7 +313,7 @@ function block_admin_configure($form, &$
+  $role_options = array_map('check_plain', db_query('SELECT rid, name FROM {role} ORDER BY name')->fetchAllKeyed());

Couldn't we use user_roles() here instead of querying directly?

#13

Here's a reroll because part of the patch no longer applied. I'm also trying user_roles() instead of the direct query.

AttachmentSizeStatusTest resultOperations
316136-user-roles-check-plain-13.patch3.8 KBIdlePASSED: [[SimpleTest]]: [MySQL] 19,927 pass(es).View details | Re-test

#14

Looks good to me - that direct database query was a preexisting bug (a bug because its failure to use the API means the roles were presented in the wrong order), but might as well fix it here too.

RTBC from my point of view, but I wrote most of it :) I hope in the intervening months there weren't any new instances where role names are displayed (probably not, but I remember them being a pain to search for).

#15

Status:needs review» fixed

I can't think of any new ones either. This looks like a good hardening improvement.

Committed to HEAD. Thanks!

#16

Status:fixed» closed (fixed)

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

#17

Version:7.x-dev» 6.x-dev
Status:closed (fixed)» patch (to be ported)

OMG, this was never backported to D6? How should I explain to new contributors that they need to sanitize role names before printing them if even D6 core doesn't handle it properly?

#18

It probably should be done, yes, but per http://drupal.org/security-advisory-policy it's not a priority.

As for the review process, tell them it's still right to learn the best practices even if core doesn't follow them.

#19

Status:patch (to be ported)» needs review

D6 backport!

AttachmentSizeStatusTest resultOperations
user_roles_check_plain-316136-19-d6.patch4.13 KBIgnored: Check issue status.NoneNone

#20

Status:needs review» needs work

+++ b/modules/filter/filter.module
@@ -323,6 +323,26 @@ function filter_formats($index = NULL) {
+function filter_get_roles_by_format($format) {

please don't introduce new functions, we should only do sanitization in this patch to comply with Drupal security standards. Also I think it is too late in the D6 release cycle to add a function just like that.

#21

Status:needs work» needs review

Gotcha, wasn't sure how to go about that. I've re-implemented the logic directly in the function. Does this look better?

AttachmentSizeStatusTest resultOperations
user_roles_check_plain-316136-21-d6.patch3.4 KBIgnored: Check issue status.NoneNone

#22

Status:needs review» needs work

Do not add any logic, just sanitize the role names and leave everything else as is, I would say. Do not touch the DB queries, do not replace anything with user_roles(), just plain sanitization before outputting role names.

#23

-      // Prepare a roles array with roles that may access the filter.
-      if (strstr($format->roles, ",$rid,")) {
-        $roles[] = $name;
-      }
+    $format_roles = array();
+    $permission = filter_permission_name($format);
+
+    if ($format->format == filter_fallback_format()) {

Those filter functions don't exist in Drupal 6, so they can't be used here.

Isn't it possible to just run check_plain() on the content of $roles, without adding any extra code?

nobody click here