Follow-up to #2510104: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future

Problem/Motivation

The current selector for the drupalSettings is:

var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');

This means that when someone manages to place some drupalSettings on the page by exploiting XSS they could with CSP on manipulate settings, which could potentially lead to a real XSS exploit. (provided the drupalSettings are in the footer)

Proposed resolution

Use a selector as true child of head or body instead:

var settingsElement = document.querySelector('head > script[type="application/json"][data-drupal-selector="drupal-settings-json"], body > script[type="application/json"][data-drupal-selector="drupal-settings-json"]');

e.g. While before this patch with CSP enabled someone could e.g. put a script element into a hypothetical non-secure page-title element (via some buggy contrib module), which would be at:

   content > div.main-region > span.page-title

After this patch, this exploit vector no longer works as it is way harder to manipulate something that is a direct child of BODY or HEAD.

Remaining tasks

- Create patch
- Commit

User interface changes

- None

API changes

- None

RC phase evaluation

  • - Category: Task, because it is security hardening
  • - Priority: Major, because it is security hardening
  • - Prioritized changes: Security
  • - Disruption: None, as it is just the selector that is changed and other selectors will continue to work.
CommentFileSizeAuthor
#19 harden_drupalsettings-2592925-19.patch748 bytesfabianx
#19 interdiff.txt610 bytesfabianx
#17 harden_drupalsettings-2592925-17.patch740 bytesfabianx
#14 harden_drupalsettings-2592925-14.patch782 bytesfabianx
#2 drupal-harden_drupal_settings_selector_against_XSS-2592925-2.patch671 bytessagar ramgade

Comments

Fabianx created an issue. See original summary.

sagar ramgade’s picture

sagar ramgade commented
Status: Active » Needs review
StatusFileSize
new drupal-harden_drupal_settings_selector_against_XSS-2592925-2.patch671 bytes

Patch attached.

fabianx’s picture

fabianx commented
Assigned: Unassigned » nod_

Thanks for the patch!

+++ b/core/misc/drupalSettingsLoader.js
@@ -7,7 +7,7 @@
-  var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');
+    var settingsElement = document.querySelector('head > script[type="application/json"][data-drupal-selector="drupal-settings-json"], body > script[type="application/json"][data-drupal-selector="drupal-settings-json"]');

There is a slight space change that needs to be fixed.

--

Assigning to nod_ for review.

droplet’s picture

droplet commented

It's like better but useless. anyone able to insert a "script" tag, they could inject settings in many way. I'd say close as won't fix and KISS.

pere orga’s picture

pere orga
Primary language Catalan
commented

It's like better but useless. anyone able to insert a "script" tag, they could inject settings in many way. I'd say close as won't fix and KISS.

Unless CSP is enabled. Then JavaScript code inside script tags would not run, but settings could still be injected that way.

The reason I would close this is because document.querySelector() returns the first element within the document, so the patch would not make much difference.

fabianx’s picture

fabianx commented

#5: Oh, yes it would.

Settings are usually at the bottom of the body by now, so someone could inject settings that way - in case of XSS and CSP enabled and given our Drupal.ajax structure I am pretty sure settings are enough to exploit this somehow.

fabianx’s picture

fabianx commented
Title: Harden drupalSettings selector against XSS » Harden drupalSettings selector against XSS when CSP is enabled

Clarifying title ...

droplet’s picture

droplet commented

Unless CSP is enabled. Then JavaScript code inside script tags would not run, but settings could still be injected that way.

Umm. make sense.

pere orga’s picture

pere orga
Primary language Catalan
commented

Settings are usually at the bottom of the body by now, so someone could inject settings that way - in case of XSS and CSP enabled and given our Drupal.ajax structure I am pretty sure settings are enough to exploit this somehow.

Ah, if Drupal settings are in the bottom then yes.

nod_’s picture

nod_
Primary language French
Location Lille
commented

At a high level we're in the same situation with D7. anything can add a script tag that messes with Drupal.settings value.

If we take the initial scenario:

This means that when someone manages to place some drupalSettings on the page by exploiting XSS they could manipulate settings, which could potentially lead to a real XSS exploit.

If someone manage to place some drupalSettings on the page, it means they managed to put a script tag on the page (whatever the type attribute is) and if that's the case, all bets are off. We can't do anything to salvage the situation.

The only thing we could do is make drupalsettings an inline script again and add the CSP header + attribute needed: http://www.html5rocks.com/en/tutorials/security/content-security-policy/... short of that I'd vote for a won't fix on this issue.

fabianx’s picture

fabianx commented

#10: Huh?

If CSP is enabled and someone manages to put a script tag on the page, it usually is in a deeper down level as only very very few things directly are below BODY or HEAD elements.

So why would this direct-child-of-body-or-head selector not harden the security on that?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Doesn't hurt and it does prevent potential issues when CSP is on. I sort of missed the point in #10.

NW only because of whitespace. RTBC otherwise.

fabianx’s picture

fabianx commented
Issue summary: View changes

Updated IS and added RC evaluation.

fabianx’s picture

fabianx commented
Status: Needs work » Needs review
StatusFileSize
new harden_drupalsettings-2592925-14.patch782 bytes

Fixed whitespace and added some docs so someone is not "simplifying" this in the future.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

all good

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

Tagging as RC target after discussing with the other committers.

However no need to self-reference this issue in the comment, git blame (or an explanation) is better.

fabianx’s picture

fabianx commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new harden_drupalsettings-2592925-17.patch740 bytes

Here we go with the self-reference removed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think there's a typo? Perhaps I'm just tired.

+++ b/core/misc/drupalSettingsLoader.js
@@ -7,7 +7,8 @@
+  // Use direct child elements to harden XSS exploits when CSP is on.

s/harden XSS exploits/harden against XSS exploits/

Can be fixed on commit either way; all committers are native English speakers :P

fabianx’s picture

fabianx commented
StatusFileSize
new interdiff.txt610 bytes
new harden_drupalsettings-2592925-19.patch748 bytes

Uhm, ... Thanks! Nice catch :p.

Fixed in attached patch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed ddda2af on 8.0.x 
    Issue #2592925 by Fabianx, Sagar Ramgade: Harden drupalSettings selector...

Status: Fixed » Closed (fixed)

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