Posted by ingo86 on October 2, 2008 at 8:31am
| 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
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:
#9
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.
#10
Is this critical for D7 stable? I guess so...
#11
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.
#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
I can't think of any new ones either. This looks like a good hardening improvement.
Committed to HEAD. Thanks!
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
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
D6 backport!
#20
+++ 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
Gotcha, wasn't sure how to go about that. I've re-implemented the logic directly in the function. Does this look better?
#22
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?