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.
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
Comment #1
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #2
pwolanin CreditAttribution: pwolanin commentedCatching it in the user_roles() function also gets most places it's displayed - but might lead to double-encoding.
Comment #3
pwolanin CreditAttribution: pwolanin commentedComment #4
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #5
dwwRight. 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.
Comment #6
gregglesIs 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.
Comment #7
Heine CreditAttribution: Heine commentedBecause rolenames are plaintext (would you expect tags in them? (no) Ampersands ? (yes)), they need to be check_plained to get even valid HTML.
Comment #8
Heine CreditAttribution: Heine commentedAlso from Handle text in a secure fashion:
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #10
meba CreditAttribution: meba commentedIs this critical for D7 stable? I guess so...
Comment #11
Jody LynnThis does not seem critical, as it's a 'root user can hack himself' type of vulnerability.
Comment #12
coltraneLet's just get this fixed and committed.
Couldn't we use user_roles() here instead of querying directly?
Comment #13
coltraneHere's a reroll because part of the patch no longer applied. I'm also trying user_roles() instead of the direct query.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedLooks 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).
Comment #15
webchickI can't think of any new ones either. This looks like a good hardening improvement.
Committed to HEAD. Thanks!
Comment #17
klausiOMG, 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?
Comment #18
gregglesIt 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.
Comment #19
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport!
Comment #20
klausiplease 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.
Comment #21
Albert Volkman CreditAttribution: Albert Volkman commentedGotcha, wasn't sure how to go about that. I've re-implemented the logic directly in the function. Does this look better?
Comment #22
klausiDo 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.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedThose 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?
Comment #24
darktygur-1 CreditAttribution: darktygur-1 commentedReally 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.
Comment #25
gregglesWould be nice to finish this off.