Issue Summary
The security team now has a FAQ that mentions the following:
Another case where no security announcement is required is when an exploit requires one of the following permissions:
- Administer filters
- Administer users
- Administer permissions
- Administer content types
- Administer site configuration
In general, every permission that in itself already enables site-takeover.
Why do we have 5 different "super-admin" permissions? Clearly not every Drupal site admin on Earth understands that all 5 of those are effectively "own the site" perms. If having access to any of them gives you powers to accomplish all of them, why pretend there's any priv separation among them at all? Why not a single "super-user" permission? Something like:
"Administer as superuser" ?
Wouldn't that be a lot clearer for people trying to configure permissions on their sites? Wouldn't that remove the illusion that any of these permissions are actually distinct that could safely be granted to a non-super-admin on their own?
Comments
#1
I understand the underlying goal here - why have 5 if they allow you to own the site?
But I think there's also good reason to keep them separate: the person who administers users is often (usually?) different than the person who administers filters. Permissions are not only about security, but also about presenting a logical UI to the end user.
I suggest an alternate solution is to help these "super perms" to stick out more in the UI as permissions that can be used by a malicious end user to take over the site.
#2
I totally agree with greggles...
#3
Permissions are not only about security, but also about presenting a logical UI to the end user.
That's a very good point I hadn't considered. Hiding functionality people don't normally need is a valid use of a permission, even if they could maliciously overcome that and access the functionality, anyway. Preventing accidental human error from causing damage is another worthy goal, even if it's no protection against outright malice.
Okay, probably the original proposal here should be "won't fixed", but perhaps we should alter the scope to address the UX problem of clearly marking these perms as "super-admin" in some way in the UI?
#4
We have permission descriptions now. Just use those.
#5
We actually already use the permission descriptions to warn about security risks -- there are a bunch in Drupal 7 that have this text:
Warning: Give to trusted roles only; this permission has security implications.These messages are themed as placeholders (which makes them practically invisible in the UI) and there are other wonky things about it, but they are there.
However, I figured I'd take a look and see which permissions we actually label that way in D7 core. There appear to be seven of them:
The incomplete overlap between this list and the one in the security team FAQ is a bit troubling... I think this should be considered a bug to be fixed.
#6
I'm pretty sure the lack of overlap is because the security team list is for Drupal 6, whereas this is from HEAD.
fwiw I agree with leaving these as separate permissions, and also trying to make it obvious that their in a trusted users only' group - this would also fit well with the work going on around filter permissions.
#7
That might be true for some of them, but the fact that, say, "administer users" does not currently get a warning is a pretty serious bug. With that permission, you can kind of, like, hijack any user account on the site, including user 1....
#8
This isn't whole issue is not attacking a bug - can we scale the scope of this issue to fix the one bug that is mentioned in #7 or make a new one? The other functionality doesn't seem very tuned, do we really need to warn users about everything? It sets a very unfriendly tone.
#9
The issue mentioned in #7 was actually already fixed in #616616: Warn about "Administer Users".
I'm not a fan of the current "warning" UI either (too verbose), but fixing that may be for a separate issue (unless the answer is don't warn about any permissions at all). Whatever UI we have, though, we need to go through and make sure that it is being applied consistently and for the correct permissions.
#10
Not sure whether this qualifies as critical.
#11
I think it does. If we're going to continue labeling these permissions at all (as we do now), we better label the right ones - otherwise we give people wrong information that could encourage them to configure their sites insecurely.
#12
To summarize the current state. Here is a list of the various permissions with security permissions (as mentioned in this thread), detailing which ones have a warning, and what that warning is:
Key
- => no warning text
1 => "Warning: Give to trusted roles only; this permission has security implications."
2 => "Warning: This permission may have security implications depending on how the text format is configured."
So it looks like there are just a couple left that need warning text 1 added?
#13
@mrfelton that's a great summary and I think your proposal makes a lot of sense. I also agree this is critical given how many people misconfigure these values.
#14
Here is a patch that does what I outlined in #12 (adds the missing warning messages to 'administer site configuration' and 'administer content types' permissions).
#15
Thanks! - looks like a good start to me. Note that "administer and use any text formats and filters" should be a (1) rather than a (2), although I already have an open bug report about that somewhere so we don't really need to fix it here.
I worry a bit about ones that aren't on this list at all, though (for example, 'administer software updates'). We need to look carefully at all the new Drupal 7 permissions.
BTW, does 'administer tests' really need this warning label? Off the top of my head, I'm not clear on what makes it dangerous.
#16
Honestly, I don't see this patch being any good - its a bad trend we set. To warn people for all bad possible configuration, by simply inserting a warning text. It probally won't fix the problem the user will experience, all it will set is a "I told you so" and warning text makes users feel like we think they are dumb.
This should really be more intelligent, par example only giving a warning when giving it to registerd users or to annonymous, not giving a warning for newly created roles. Given that, will be Drupal 8.x stuff... Is this really critical? I whish we can find a better solution for this.
#17
Well, right now we have an "I told you so" buried in the handbook and nobody knows about it so they make their sites unsafe. Is that better?
It is really hard to make this intelligent because many sites will automatically grant a role when a user creates an account so we have to check "anonymous, authenticated, any-role-created-by-the-7-modules-that-automatically-grant-roles-or-sell-roles."
#18
Agreed with #17. An "I told you so" in the admin interface is better than one linked in a reply from the security team when you either report a security issue with a module or your site was hacked.
#19
I also think it should really be more intelligent, far example only giving a warning when giving it to registerd users or to anonymous, not giving a warning for custom created roles.
also the-7-modules-that-automatically-grant-roles-or-sell-roles should be able to hook into this verification process somehow.
Maybe even blocking certain actions for anonymous. (Seriously, WTF?)
#20
@alexanderpas: yes, while it's tempting to say "anon should never have access to X", there are drupal sites installed inside private networks where our assumptions about X will be wrong. while we can and should go out of our way to make sure sites know they're doing something potentially stupid and wrong, we shouldn't completely prevent it in case they know what they're doing...
#21
Also, this is just a description added to permissions. It's not warning you when you actually grant it to someone, flashing drupal_set_message() etc. everywhere. Given there's only two string changes in this patch, I'm marking this RTBC. Cleverer stuff might be nice, but we can do that in new, non-critical issues.
#22
Committed to HEAD.
I agree with Bojhan that a larger discussion about how these dangerous permissions should be identified and what should happen if they're enabled for specific roles does indeed feel like D8 material to me. But it's very difficult to justify not adding this same disclaimer that's already been in D7 for over a year on all of the dangerous permissions identified by the security team in that handbook page.
#23
Back to "needs work" for the remaining permissions.
#24
@David So what are those remaining permissions?
#25
The one I know for sure that doesn't currently have the warning but needs to have it added is:
Two that come to mind that currently have the warning but potentially do not need it are:
I don't know if there are others - it would take a careful look at how permissions are being used in order to be sure. This should definitely happen before release, but I don't think we're in a huge rush, since some functionality might still change (and given that #248598: Label permissions which are warned about in the user interface is now in, these warnings can now be removed and added at any time without breaking the string freeze also).
#26
So here is a patch that renames "Run software updates" to "Administer software updates", adds the restriction to it and removes the restriction from Testing permissions. Showing the HTTP auth password or not should be a different issue. Administer nodes permission was not changed.
#27
Reviewed and confirmed that the patch works as expected.
#28
I don't think we can remove the warning from "Administer tests" unless we also stop using it to show passwords in clear text...
#29
OK, thinking about this a bit more, the HTTP auth credentials issue probably doesn't actually make "Administer tests" a dangerous permission, since if you can view the page where those credentials are displayed, you probably already had to know them in the first place to be able to access the website? Plus, looking at CVS history, it appears the permission warning predates those, so it certainly wasn't the reason it was added. However, what to do about this permission is probably best discussed separately in the SimpleTest queue, where people who understand all the innards of SimpleTest might be able to weigh in. So I've created separate issues here:
#799932: Simpletest HTTP authentication credentials should use the 'password' form element
#799936: "Administer tests" permission shouldn't be labeled a security risk
Meanwhile, here is a reroll of #26 to remove that part of the patch so it only focuses on 'administer software updates'.
There is more reviewing of permissions to be done before this issue can be closed out, but this one would make a good interim commit to get in sooner rather than later, especially since it changes a translatable string. Note that everywhere else in core besides the permission description itself was already still referring to this as "Administer software updates" (e.g., user-facing text in update.php, code comments in settings.php, etc) so all we are doing here is reverting back the permissions page to the text that was already decided on and is already in use everywhere else.
#30
Looks good to me (assuming all test pass).
#31
#29 is definitely a good move. I'm not sure why it was changed, but yes, it should be reverted back to how it was. Thanks.
#32
It is not clear why we are renaming this permission?
#33
We're restoring it to its original name, which is still the name that's used to refer to this permission everywhere else in Drupal except on this one page.
And "Run software updates" doesn't accurately convey the extent what the permission allows you to do (upload new code in addition to running it).
#34
Committed to CVS HEAD. I'm marking this 'fixed' but we might want to follow-up on the other things, I'm guessing.
#35
Automatically closed -- issue fixed for 2 weeks with no activity.
#36
Let's reopen so we can potentially follow up on other things. The 'administer nodes' discussion above is not resolved (although that could maybe get its own issue), but more generally it would be a lot more comforting if we reviewed the permission list at least one more time before release to make sure we're not missing anything.
For example, I've heard people discuss that permissions like 'administer menu' and 'administer blocks' might be considered inherently dangerous since by design they let a malicious user mess up a site pretty badly (although not really take it over completely), so we should at least talk about where the 'dangerous' line should be drawn, before the final version of D7 is released.
#37
IMO, those links are not dangerous enough to merit this class of notice. The description should be enough. Lets not overuse the danger warning
#38
Although David_Rothstein his concerns are valid this by no means is a critical issue any more. Also the more you add, the less effective it becomes - keep it in mind.
#39
What are thoughts on having two 'super admins' established? In the case of an organization where there may be a 'backup' required to change regular permissions (in case someone is out). Is this even possible?
#40