Add the security warning to more core module permissions

Hi. In this patch I added restrict access: true too all permissions mentioning Administer inside core.

I left out the following, inside field_ui.permissions.yml, since Administer is not mentioned in the title (just in the name). Should it be included?

administer display modes:
  title: 'Add, edit, and delete custom display modes.'

Philosophically, we should only add this attribute to permissions where a user who has that permission can take over a site.

For example: a user with "administer permissions" is going to be able to take over the site by the nature of that permission.

In contrast, "Administer book outlines" is a permission where nothing in the nature of the permission means that it makes the whole site vulnerable.

I think we should use the `restrict access: true` attribute minimally and only where appropriate regardless of the word in the permission.

I do agree with @greggles personally.

+++ b/core/modules/field/tests/modules/field_test/field_test.permissions.yml
@@ -4,3 +4,4 @@ view test_view_field content:
 administer field_test content:
   title: 'Administer field_test content'
   description: 'Manage field_test content'
+  restrict access: true

And is it necessary to restrict test modules? For example, the field_test module like the above. Modules for testing are hidden by default. No one will see the warning in general.

I'd vote to no.

It's clear, I just followed the proposed resolution literally. What about things like administer blocks or administer languages, usually you will not be able to take over the site but it can be harmful.

I will create another patch with the right options.

As per my knowledge in Drupal. I have identified some major things or modules where it is mandatory to put the warnings for giving the permission for other users. Additionally i am posting the updated patch and PDF where i am identifying the modules. Please check for the same.

I have updated the patch with the required module administer warnings as per my knowledge. Along with the edited module list below

1Aggregator - Permissions enabled
2 Block - Permissions enabled
3 Book Outlines -Not Required
4 Comment and Comment Settings -Permissions enabled
5 Contact forms -Permissions enabled
6 Content Translation -Permissions enabled
7 Field Test - Not Required
8 Forum -Permissions enabled
9 Image -Permissions enabled
10 Language -Permissions enabled
11 Responsive Image -Permissions enabled
12 Search -Permissions enabled
13 Shortcut -Permissions enabled
14 Statistics -Not Required
15 System -Permissions enabled
16 System_test- Not Required
17 Entity_test- Not Required
18 Taxonomy -Permissions enabled
19 Workspaces -Permissions enabled

We will picking this in vb_global_contribution_hour1

1. +++ b/core/modules/block/block.permissions.yml
   @@ -1,2 +1,3 @@
    administer blocks:
      title: 'Administer blocks'
   +  restrict access: true
   \ No newline at end of file
   

   No newline at end of file

2. +++ b/core/modules/forum/forum.permissions.yml
   @@ -1,2 +1,3 @@
    administer forums:
      title: 'Administer forums'
   +  restrict access: true
   \ No newline at end of file
   

   No newline at end of file

3. +++ b/core/modules/image/image.permissions.yml
   @@ -1,2 +1,3 @@
    administer image styles:
      title: 'Administer image styles'
   +  restrict access: true
   \ No newline at end of file
   

   No newline at end of file

4. +++ b/core/modules/language/language.permissions.yml
   @@ -1,2 +1,3 @@
    administer languages:
      title: 'Administer languages'
   +  restrict access: true
   \ No newline at end of file
   

   No newline at end of file

5. +++ b/core/modules/system/system.permissions.yml
   @@ -28,3 +30,4 @@ link to any page:
    administer menu:
      title: 'Administer menus and menu items'
   +  restrict access: true
   \ No newline at end of file
   

   No newline at end of file

6. +++ b/core/modules/taxonomy/taxonomy.permissions.yml
   @@ -1,6 +1,7 @@
      title: 'Administer vocabularies and terms'
   -
   ...
   +   ¶
   

   redundant leading spaces below restrict access: true

@jungle Please review the patch now after corrections

Looks good for me. Thanks @shimpy!

Thanks, all, for your continued work on this!

With each of these, I think we should ask: "Does this permission inherently allow someone to "take over" the site?" If not, then we should not add the `restrict access: true` property to it. Adding that property to too many permissions makes it seem less serious. If an admin has to say "Well, I trust this person enough to administer forums which has that risky property on it, maybe I should also trust that role for administering users."

Here are a few that are in patch #14 and giving me doubts about adding the restrict access property:

title: 'Administer news feeds'
title: 'Administer blocks'
title: 'Administer comments and comment settings'
title: 'Administer contact forms and contact form settings'
title: 'Add, edit, and delete custom display modes.'
title: 'Administer forums'
title: 'Administer image styles'
title: 'Administer languages'
title: 'Administer URL aliases'
title: 'Administer responsive images'
title: 'Administer search'
title: 'Administer shortcuts'
title: 'Administer themes'
title: 'Administer menus and menu items'
title: 'Administer vocabularies and terms'
title: Administer workspaces

All of those above permissions seem like they allow for seriously altering the way a site looks and behaves. At least they would allow someone to dramatically alter a section of the site (news feeds, display modes, forums, vocabularies and terms). Some would let an admin dramatically alter the whole site (blocks, themes, menus). Some would be very useful in a social engineering attack (URL aliases, contact forms). I'm not sure that the ability to administer forums should really be considered as a permission with the same risk as administer users.

Here's one that seems like it's nature would allow for causing so much damage to a site that it could probably let someone take over a site (e.g. disabling security modules, enabling modules that create weaknesses in a site like a php-execution module).

title: 'Administer modules'

I do think that administer modules should have restrict access set to true.

Before making a new patch we might want to have some discussion about these individual permissions to decide which are inherently risky.

Hi @greggles

Agreed with your point. Because we shouldn't give me permission restrictions for blocks. In my project we have given administer blocks to editor. So we don't want restriction warning for the administer blocks

I agree with the general idea that overuse of "restrict access" risks "debasing the currency" to the point where it becomes somewhat meaningless.

A freshly installed D8 site currently has 98 different permissions as far as I can see, of which 40 begin with "administer". There are 23 permissions currently marked as "restricted", and not all of these include "administer" in their name.

$ drush php

>>> $permissions = \Drupal::getContainer()->get("user.permissions")->getPermissions();

>>> $i = 0; foreach ($permissions as $k => $perm) { if (isset($perm['restrict access'])) { $i++ ; print $i . ': '. $k . PHP_EOL; } }
1: administer comment types
2: export configuration
3: import configuration
4: synchronize configuration
5: administer comment fields
6: administer contact_message fields
7: administer node fields
8: administer block_content fields
9: administer taxonomy_term fields
10: administer user fields
11: administer filters
12: administer nodes
13: administer content types
14: bypass node access
15: administer site configuration
16: administer software updates
17: link to any page
18: access site reports
19: administer account settings
20: administer permissions
21: administer users
22: select account cancellation method
23: administer views

If we add all the other "administer" permissions:

>>> $i = 0; foreach ($permissions as $k => $perm) { if (isset($perm['restrict access']) || strpos($k, 'administer') !== FALSE) { $i++ ; print $i . ': '. $k . PHP_EOL; } }
1: administer blocks
2: administer comment types
3: administer comments
4: export configuration
5: import configuration
6: synchronize configuration
7: administer contact forms
8: administer comment display
9: administer contact_message display
10: administer node display
11: administer block_content display
12: administer taxonomy_term display
13: administer user display
14: administer comment fields
15: administer contact_message fields
16: administer node fields
17: administer block_content fields
18: administer taxonomy_term fields
19: administer user fields
20: administer comment form display
21: administer contact_message form display
22: administer node form display
23: administer block_content form display
24: administer taxonomy_term form display
25: administer user form display
26: administer display modes
27: administer filters
28: administer image styles
29: administer nodes
30: administer content types
31: bypass node access
32: administer url aliases
33: administer search
34: administer shortcuts
35: administer menu
36: administer modules
37: administer site configuration
38: administer software updates
39: administer themes
40: link to any page
41: access site reports
42: administer taxonomy
43: administer account settings
44: administer permissions
45: administer users
46: select account cancellation method
47: administer views

So that's now close to half of all the permissions in the system. I think it'd be good to avoid ending up with half of core's permissions marked as restricted.

On the other hand, @samuel.mortenson makes a good point about contrib code perhaps making assumptions about "administer" permissions, and I think it's generally implied that these should only be assigned to "trusted" roles.

We've certainly dealt with a few security issues recently where an "administer"-type permission allows unfiltered HTML but is not marked as restricted, and we've had some debate about whether marking the permission as restricted is the correct "fix" in this situation (there are a few recent contrib SAs we could reference here).

To think the implications through, I suppose we need to ask:

• Is it a problem if a permission allows e.g. unfiltered HTML but is not marked as restricted?
• If a permission is called administer $something is that sufficient to convey that it should only be given to trusted roles?
• Is there any value in keeping the number of permissions marked as "restricted" to a minimum?

@greggles mentioned considering:

Does this permission inherently allow someone to "take over" the site?

...which I agree is a useful yardstick. What does "take over" mean though? Does that include, for example, being able to perform XSS because you can enter unfiltered HTML?

Looking at the list of "administer" permissions which are not (yet) marked as restricted:

>>> $i = 0; foreach ($permissions as $k => $perm) { if (strpos($k, 'administer') !== FALSE && !isset($perm["restrict access"])) { $i++ ; print $i . ': '. $k . PHP_EOL; } }
1: administer blocks
2: administer comments
3: administer contact forms
4: administer comment display
5: administer contact_message display
6: administer node display
7: administer block_content display
8: administer taxonomy_term display
9: administer user display
10: administer comment form display
11: administer contact_message form display
12: administer node form display
13: administer block_content form display
14: administer taxonomy_term form display
15: administer user form display
16: administer display modes
17: administer image styles
18: administer url aliases
19: administer search
20: administer shortcuts
21: administer menu
22: administer modules
23: administer themes
24: administer taxonomy

... we can probably come up with malicious things that most of them would allow a bad actor to do.

Are there any which we could consider "mostly harmless"? Perhaps "administer image styles" and most of the "administer $something display"?

(This list's not exhaustive as it doesn't include permissions from modules which are not enabled by default; e.g. "Administer forums".)

There are definitely a few on the list which are powerful enough that they should be marked as restricted if the criteria for that is allowing a site to be "taken over" maliciously; e.g. administer blocks, modules, menu.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Research

I've taken a fresh look at existing 11.x restricted access permissions to get a sense of the existing categories where a permission has been considered worth this warning level.

Site building and maintenance

As somebody who builds sites for clients, this is all in the list of things I generally would not want them to be able to do for fear of breaking things, even unintentionally.

• administer [entity_type] types
• administer [entity_type] fields
• administer [major subsystem] (actions|site configuration|themes|views|workflows)
• translate interface
• administer text formats and filters
• export configuration
• import configuration
• synchronize configuration
• translate configuration
• administer software updates

However, I'm not sure that I could back up all of these as having "security implications" without being so broad as to then need to flag all of the administer [entity_type] display and administer [entity_type] form display permissions as well.

User management

It's important for site owners to be able to manage their own users, and having the warning messages here helps to reinforce how important it is that user roles and permissions are handled thoughtfully.

• administer account settings
• administer permissions
• administer users
• select account cancellation method

Access overrides

A webmaster or designated content administrator should be able to bypass access controls that are in place for users with lower levels of access.

• administer [entity_type] entities (usually "content" but implied in some cases)
• delete any file
• configure any layout
• bypass content access control
• link to any page
• bypass entity access own workspace

Information disclosure

Pages that provided detailed system information should be protected in order to prevent malicious actors from gaining information about the specific software version(s) used by the site, or to information disclosed in error logs.

• access site reports

Other notes

The permissions for individual filter formats now have a specific warning: "This permission may have security implications depending on how the text format is configured."

Proposal

Using the above as guidelines, here are the permissions I suggest adding to the restricted list.

Access

• administer workspaces meets the "administer [entity_type] entities" criteria
• administer blocks meets the "administer [entity_type] entities" criteria
• administer comments meets the "administer [entity_type] entities" criteria
• administer contact forms meets the "administer [entity_type] entities" criteria
• view any unpublished content meets the "bypass access control" criteria
• ban IP addresses could arguably fall into this category as it could do the opposite -- prevent site owners from being able access their site completely

Information disclosure

• administer modules allows access to the module list page, which shows enabled versions
• view update notifications displays module versions and specifically highlights out-of-date modules