Problem/Motivation

The "access security review list" permission allows seeing security check results, which can contain sensitive information about the website (path of unprotected folders, etc.).

The README correctly says:

NOTICE: This module provides information on the state of your site's security so
it is imperative you grant these permissions to trusted roles and users only.
For instance, if you have an admin role, be sure that all the users who have
been granted this role are indeed users you trust if you grant them these
permissions.

But in security_review.permissions.yml, these permissions don't have restrict access: true so Drupal doesn't know they are dangerous.

Proposed resolution

Add the "restrict access" flag to the "access security review list" permission.

(This has been discussed privately with the security team and it was decided it could be handled publicly.)

Issue fork security_review-3491589

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

smustgrave’s picture

smustgrave commented
Assigned: Unassigned » smustgrave

prudloff opened merge request !81

prudloff’s picture

prudloff commented
Status: Active » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Reviewed & tested by the community

LGTM. Thanks.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I will add my take: this doesn't feel exactly right to me from the perspective of Drupal's philosophy of restrict access. An attacker with access to a site that has a vulnerable configuration can already exploit it, regardless of the security_review report access. In fact, an attacker could look at the code of security_review on drupal.org for ideas of things to check even if the module is not installed. Restricting this access does make it less likely an attacker will find vulnerabilities and there are enough people who think this makes sense that I'm OK with the idea.

prudloff’s picture

prudloff commented

In fact, an attacker could look at the code of security_review on drupal.org for ideas of things to check even if the module is not installed.

I think it is true for most of the checks, but some checks could reveal information that would not be available (or hard to acquire) without this module.
For example TemporaryFiles provides a list of sensitive files that could be downloaded. Without this information it could be possible to brute force the file name but if the file name is unusual it would be much harder.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Yes, that's a great point in favor of continuing with the commit.

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.