New role name not filtered into admin/user/permissions
ingo86 - October 2, 2008 - 08:31
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Security improvements |
Description
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.

#1
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
Catching it in the user_roles() function also gets most places it's displayed - but might lead to double-encoding.
#3
#4
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->rolesdirectly. 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: