Problem/Motivation

The comment under the "default-src" field at /admin/config/system/seckit is wrong / misleading.
It says:
> Specify security policy for all types of content, which are not specified further (frame-ancestors excepted). Default is 'self'.

But its default is not 'self'. Its default value is empty, meaning the "default-src" definition is not present in the generated Content-Security-Policy header.

Steps to reproduce

1) configure the secktit module

Go to /admin/config/system/seckit
Configure the "Content Security Policy" section under "Cross-site Scripting"
Send HTTP response header - enable
You can see the "default-src" field has empty value (after the fresh module installation)
Verify the "report-uri" field has the default "/report-csp-violation" value.
Save configuration.

2) observe the content security policy header:

for example, using curl and a cache buster: 

$ curl -sI http://my-d8.site/?ab1 | grep Content-Security-Policy
Content-Security-Policy: report-uri /report-csp-violation

conclusion: The default value was empty and the "default-src" directive is not present in the header.

3) reconfigure the seckit module

Set the "default-src" to 'self' and save.

4) Observe the header

$ curl -sI http://my-d8.site/?ab2 | grep Content-Security-Policy
Content-Security-Policy: default-src 'self'; report-uri /report-csp-violation

conclusion: after *explicitly* setting the value of "default-src" to 'self', this made it to the header.

Proposed resolution

Change the comment under the field
from: "Default is 'self'."
to: "Default is empty, meaning the default-src directive won't be present in the generated header."

Remaining tasks

Find the wrong comment in the module code. Create a patch.

User interface changes

Changed user interface text

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#2 3193697-2.patch890 bytesmohit.bansal623

Issue fork seckit-3193697

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marji created an issue. See original summary.

mohit.bansal623’s picture

mohit.bansal623 CreditAttribution: mohit.bansal623 as a volunteer and at OpenSense Labs commented
Status: Active » Needs review
FileSize
3193697-2.patch890 bytes

As per the comment, updated the description and created the patch. Please review.

RichardGaunt’s picture

RichardGaunt CreditAttribution: RichardGaunt at Salsa Digital commented
Priority: Minor » Critical
Status: Needs review » Reviewed & tested by the community

I've updated priority to critical as the critical setting has a misleading description.

If someone was to read this description and assume the default was 'self' then their site would have an ineffective Content Security Policy.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Priority: Critical » Major
Status: Reviewed & tested by the community » Needs work

It would be worth looking at how it's handled in the D7 branch.

Reducing the priority to "major" as nothing is broken, the UI just could be better.

ramonvasconcelos’s picture

ramonvasconcelos CreditAttribution: ramonvasconcelos at CI&T commented
Assigned: Unassigned » ramonvasconcelos

I had a look at the D7 branch and the description is the same.
Specify security policy for all types of content, which are not specified further (frame-ancestors excepted). Default is 'self'.
I think the changes suggested in this issue are applicable.

ramonvasconcelos’s picture

ramonvasconcelos CreditAttribution: ramonvasconcelos at CI&T commented
Assigned: ramonvasconcelos » Unassigned
Status: Needs work » Needs review

Moving it to needs review so we can get a third opinion.

pflora’s picture

pflora CreditAttribution: pflora at CI&T commented
Status: Needs review » Reviewed & tested by the community

I wonder if we should set self as default as the original description intended. In any case, the patch provided by #2 solves the description problem, so I'm moving this to RTBC so a maintainer can give us some input.

Rajeshreeputra’s picture

Rajeshreeputra
Location Pune
CreditAttribution: Rajeshreeputra as a volunteer and at Acquia, Shivswarajya Inc. commented

Agree description needs update.

anoopsingh92’s picture

anoopsingh92
Location Rajasthan, India
CreditAttribution: anoopsingh92 at Valuebound for Valuebound commented

Agree +1 RTBC

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Reviewed & tested by the community » Needs work

re. #4 and #5 yes, the description is the same in the D7 branch but the default is correctly applied to match the description.

https://git.drupalcode.org/project/seckit/-/blob/7.x-1.12/seckit.module#...

Let's fix 2.x rather than changing the description to match the bug.

https://git.drupalcode.org/project/seckit/-/blob/2.0.1/config/install/se...

bharath-kondeti made their first commit to this issue’s fork.

bharath-kondeti opened merge request !16

bharath-kondeti’s picture

bharath-kondeti CreditAttribution: bharath-kondeti as a volunteer and at Specbee commented
Status: Needs work » Needs review

Addressed #10 and raised a PR. Please review

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Status: Needs review » Needs work

Thanks. However, a clean install of the module still seems to have an empty value for default-src:

$ drush cget seckit.settings

_core:
  default_config_hash: **SNIP**
seckit_xss:
  csp:
    checkbox: false
    vendor-prefix:
      x: false
      webkit: false
    report-only: false
    default-src: ''   <====
    script-src: ''
    object-src: ''

..snip..

The same thing shows up in the admin UI, as we'd expect.

I'm not sure why yet, but just adding the default to config/install/seckit.settings.yml doesn't seem to be enough to fix this.