Correctly label all site-owning super-admin permissions

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.

I totally agree with greggles...

We have permission descriptions now. Just use those.

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:

• use PHP for settings
• administer unit tests
• administer filters
• administer nodes
• bypass node access
• administer permissions
• select account cancellation method

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.

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.

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

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.

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.

Not sure whether this qualifies as critical.

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.

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:

• administer site configuration (-)
• administer users (1)
• administer permissions(1)
• administer content (1)
• administer content types (-)
• administer and use any text formats and filters (2)
• use the (x) text format (2)
• administer tests (1)
• use PHP for settings (1)
• bypass content acces access (1)
• select method for cancelling own account (1)

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?

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

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

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.

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.

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

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.

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

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.

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.

Back to "needs work" for the remaining permissions.

@David So what are those remaining permissions?

The one I know for sure that doesn't currently have the warning but needs to have it added is:

• Run software updates: Since on some server configurations this permission allows you to execute arbitrary PHP code. (And it really needs to be renamed back to "Administer software updates" also.)

Two that come to mind that currently have the warning but potentially do not need it are:

• Administer tests: I can't really imagine why this is a fundamentally dangerous permission, although I do see now that on sites with HTTP auth creds entered, there is an administrative screen where this permission allows you to see the HTTP auth password in clear text. We really should just stop showing that password in clear text though (especially if it's the only thing that makes this permission a dangerous one).
• Administer nodes: In theory since "bypass node access" was split off into a different permission, it seems like this one could be downgraded? (assuming that split was complete)

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

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.

Reviewed and confirmed that the patch works as expected.

I don't think we can remove the warning from "Administer tests" unless we also stop using it to show passwords in clear text...

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.

Looks good to me (assuming all test pass).

It is not clear why we are renaming this permission?

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

Committed to CVS HEAD. I'm marking this 'fixed' but we might want to follow-up on the other things, I'm guessing.

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.

IMO, those links are not dangerous enough to merit this class of notice. The description should be enough. Lets not overuse the danger warning

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.

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?

@David_Rothstein this is what we have for Administer nodes & Administer unit tests which are the two items in #25 there were open and needing to be degraded:

    'administer nodes' => array(
      'title' => t('Administer content'),
      'restrict access' => TRUE,
    ),

    'administer unit tests' => array(
      'title' => t('Administer tests'),
      'restrict access' => TRUE,
    ),

I'm not sure what needs to change or what else is outstanding from the initial summary.

I see this was committed.
Is something still needed here or can we close this?