New role name not filtered into admin/user/permissions

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.

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

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.

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.

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.

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

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.

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.

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

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

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?

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

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

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

Committed to HEAD. Thanks!

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?

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.

D6 backport!

+++ 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.

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

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.

-      // 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?

Really surprised this hasn't been fixed already. While this isn't really a security problem, if nothing says you can't enter special characters then the site should really be able to properly display them. I just ran across this testing a contrib module that wasn't sanitizing role names. I entered an unsafe role name and noticed that the admin/user/roles page wasn't even sanitizing them. Eventually I came across this issue.

This patch is based on the one from #21. No extra logic in this one, just check_plain() calls. Also, the previous patch missed a spot - theme_user_admin_new_role(), ironically affecting the page where I first noticed this bug. Another difference is in user_admin_account() - I made sure to call check_plain() after sorting rather than before, because sorting sanitized versions of role names gives slightly different results if they actually have special characters.

I spent far too much time deciding whether I wanted to make it call check_plain() on the names for anonymous user and authenticated user roles in theme_user_admin_new_role(). I don't think it matters either way. Sanitizing them accounts for the infinitesimally small chance that a module (or an administrator wielding phpMyAdmin) would attempt to rename those roles to something with special characters. Leaving it out saves just two function calls on just one admin page. I chose to go for it, but I don't care if someone else chooses to remove the second check_plain() I added in that function.

Would be nice to finish this off.