Problem/Motivation

Drupal's vulnerability to clickjacking (embedding a site in an iframe to trick the user into taking an admin action by clicking a button in the frame) is regularly reported to the security team

https://www.owasp.org/index.php/Clickjacking_Defense_Cheat_Sheet

Proposed resolution

Implement a basic and reasonably secure defense Drupal in core, such as setting this header by default

X-Frame-Options: SameOrigin

Remaining tasks

Write a change record

User interface changes

none

API changes

none

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is security hardening.
Issue priority Major because defending against clickjacking is an important security practice.
Prioritized changes The main goal of this issue are security improvements.
Disruption Not disruptive for contributed modules.
CommentFileSizeAuthor
#23 interdiff.txt806 bytesDavid_Rothstein
#23 2514136-D7-23.patch960 bytesDavid_Rothstein
#21 2514136-D7-21.patch738 bytesDavid_Rothstein
#18 2514136-D7-18.patch792 bytespwolanin
#7 increment.txt1.65 KBpwolanin
#7 2514136-7.patch1.52 KBpwolanin
#1 2514136-1.patch1.52 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2514136-1.patch1.52 KB
pwolanin’s picture

pwolanin commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Security improvements Needs change record +Security improvements, +Needs change record

RTBC, we will need a change-record detailing how a contributed module / site builder can use iframes, e.g. how to extend this header.

Adding the information that they can implement their own Response subscriber and pointing to FinishResponseSubscriber should be enough as there are a variety of docs available.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Issue tags: -Needs change record

CR created: https://www.drupal.org/node/2514152

pwolanin’s picture

pwolanin commented

note that this header is rather standard now:

twitter.com
x-frame-options: SAMEORIGIN
facebook.com
X-Frame-Options: DENY
linkedin.com
X-Frame-Options: SAMEORIGIN
nytimes.com
X-Frame-Options: DENY
github.com
X-Frame-Options: deny
google.com
X-Frame-Options: SAMEORIGIN
pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2514136-7.patch1.52 KB
new increment.txt1.65 KB

It seems the fully capitalized value is the more standard one, so let's use that.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Yes, looks great. Back to RTBC.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Add default clickjacking defence to core » Add default clickjacking defense to core
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -113,6 +113,7 @@ public function onRespond(FilterResponseEvent $event) {
     // https://www.owasp.org/index.php/List_of_useful_HTTP_headers

This also documents X-Frame-Options, which is why we don't need to update the docs at all :)

RTBC+1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Hm. Is it possible to write a test that actually attempts to embed the Drupal site in an iframe and verify the iframe is empty? That's what we actually want to test, IMO, not just that a header is output.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

That is unfortunately not possible as we need a real browser for that, that supports that header.

However it is possible to manually test this on simplytest.me:

  • - Start patch on simplytest.me, note down the URL
  • - Go to drupal.org
  • - Open Javascript Console (Chrome: Right-Click > Inspect Element, check "Console" tab)
  • - Issue jQuery('body').append('

    ')

Result:

[<body class=​"html front logged-in no-sidebars page-home drupalorg-site-main project-issue-focused-input-processed">​…​</body>​]
about:blank:1 Refused to display 'https://dlyd.ply.st/' in a frame because it set 'X-Frame-Options' to 'SAMEORIGIN'.

Dance a happy dance!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok, cool, had to ask. :)

Given that this is "merely" outputting a new header, doesn't seem too invasive, so since it's a security improvement I think this is fine to go in. I was a bit worried about what some services like e.g. HootSuite/ow.ly would do but I see that they've changed their behaviour to stop embedding crap in an iframe (thank goodness).

Committed and pushed to 8.0.x. Thanks!

  • webchick committed c8a0c7c on 8.0.x 
    Issue #2514136 by pwolanin, Fabianx: Add default clickjacking defense to...
David_Rothstein’s picture

David_Rothstein commented
Version: 8.0.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Wondering if this could be backported in some form or another? Seems at least worth discussing.

pwolanin’s picture

pwolanin commented

Yes, i think we could default on for new installs? Maybe set a variable for existing installs?

pwolanin’s picture

pwolanin commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 2514136-D7-18.patch792 bytes

Here's a trivial patch for D7. Doesn't address whether we want to disable for existing sites, or simply add release notes about how to disable.

greggles’s picture

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

My suggestion is to enable it by default and provide release notes about how to disable it. It would break a few sites, but being secure by default seems worth it. We could write a specific book page for "Drupal site not loading in iframe" to explain the variable (I'll do that if it's a blocker to committing this as a default).

On the D8 version of the patch, it seems ideal to have a comment for the motivation (like in the D7 patch). As it stands this line is lumped together with something about mime and xss...which is not related. It also seems ideal to explain how to override this - moreso than other options this seems like something that people will want to override.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC per #19

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2514136-D7-21.patch738 bytes

Security wins out here, I think. Let's try it and see what happens :)

However I don't understand why we would only do this if Content-Type isn't set yet... Here's a new version that moves it outside of that if() statement. (Haven't checked if that's relevant for Drupal 8 since the code is so different.) Not sure if we want tests here too, maybe not if we expect some sites to override this in settings.php.

I have a revision to https://www.drupal.org/node/2514152 written up locally to make it apply to Drupal 7 also, which I can add once this is committed.

Will someone be able to update https://www.drupal.org/node/1863034 to reflect the changes from this issue? Based on the comment from @greggles above we should probably also have a followup issue to improve the Drupal 8 documentation...

greggles’s picture

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

I can edit https://www.drupal.org/node/1863034 once this is committed :)

David_Rothstein’s picture

David_Rothstein commented
StatusFileSize
new 2514136-D7-23.patch960 bytes
new interdiff.txt806 bytes

So I realized the patches above all have a problem when combined with the Security Kit module, since they clobber whatever setting it had. This could be especially bad if you're using that module to have a more restrictive setting (i.e., "Deny").

Here's a version that works better, although still breaks the ability to turn off X-Frame-Options entirely via the Security Kit admin UI (since when you set it to "Disabled", Security Kit just doesn't set the header at all, and thus the core code added here still runs). Ideally we would find a way to fix that.

David_Rothstein’s picture

David_Rothstein commented

For reference and so I don't lose it, here's the rewrite I was going to do to https://www.drupal.org/node/2514152. But right now this is written assuming the above issue is fixed (i.e., that Security Kit can always be used to override).

Drupal core now protects against <a href="https://www.owasp.org/index.php/Clickjacking">clickjacking</a> by default by emitting the 'X-Frame-Options: SAMEORIGIN' header.

This should not be disruptive for contributed modules, unless a contributed module or site wants to embed the Drupal site somewhere else (e.g. for example in a Facebook application).  In the case where you do need to override this behavior, methods for doing so are described below.  <em>Do not remove the header lightly, or else your Drupal site could be embedded on other sites and then the user tricked into doing actions they don't want.</em>


<strong>For Drupal 7:</strong>

<ol>
<li>
If you are using a module such as <a href="https://www.drupal.org/project/seckit">Security Kit</a> that already writes the X-Frame-Options header on its own, that setting will be automatically respected and Drupal core will not overwrite it.
</li>
<li>Alternatively, set the 'x_frame_options' variable via any standard method, for example in settings.php:

<?php
  // Turn off the X-Frame-Options header entirely, to restore the previous
  // behavior of allowing the site to be embedded in a frame on another site.
  $conf['x_frame_options'] = '';
?>

or

<?php
  // Set the "DENY" option to prevent the site from ever being embedded in a
  // frame at all, even on this site itself.
  $conf['x_frame_options'] = 'DENY';
?>
</li>
</ol>

<strong>For Drupal 8:</strong>

A new Response Subscriber needs to be added that has a higher priority than the current FinishResponseSubscriber (see core.services.yml) to overwrite or remove the header - depending on the use case.

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!EventSubscriber!FinishResponseSubscriber.php/class/FinishResponseSubscriber/8 is also a good example of how to write such a subscriber.

e.g. as an example for a Facebook application living at the /fb-app path:

<?php
  $path = $request->getPathInfo();
  if (strpos($path, '/fb-app/') === 0) {
    $request->headers->remove('X-Frame-Options');
  }
?>
pwolanin’s picture

pwolanin commented

Sounds like seckit can always override by setting the core variable to an empty value?

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

I agree with #25, security kit can in addition set the core variable when its disabled via $conf directly.

luksak’s picture

luksak
he/him
commented

Could someone point me in the right direction on how to remove this header in D8? The change record doesn't explain where the code in the example should go.

luksak’s picture

luksak
he/him
commented

I solved my issue. I had to set the X-Frame-Options header to allow embedding on a certain URL. Below is an example EventSubscriber that does that in case someone else needs it. Maybe this could be added in the change record?

<?php

/**
 * @file
 * Contains Drupal\EXAMPLE\EventSubscriber\SetXframeOptionsSubscriber.
 */

namespace Drupal\EXAMPLE\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
 * Response subscriber to handle finished responses.
 */
class SetXframeOptionsSubscriber implements EventSubscriberInterface {

  /**
   * Sets extra headers on successful responses.
   *
   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
   *   The event to process.
   */
  public function setXframeOptions(FilterResponseEvent $event) {
    if (!$event->isMasterRequest()) {
      return;
    }
    $response = $event->getResponse();
    $response->headers->set('X-Frame-Options', 'ALLOW-FROM http://www.google.com/', TRUE);
  }

  /**
   * Registers the methods in this class that should be listeners.
   *
   * @return array
   *   An array of event listener definitions.
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::RESPONSE][] = array('setXframeOptions');
    return $events;
  }
}

?>

  • webchick committed c8a0c7c on 8.1.x 
    Issue #2514136 by pwolanin, Fabianx: Add default clickjacking defense to...
pwolanin’s picture

pwolanin commented

Any update here on 7.x?

David_Rothstein’s picture

David_Rothstein commented
Related issues: +#2661644: Integrate with Drupal core clickjacking defense

I went ahead and created an issue for the Security Kit module now (with a patch).

Given that it was just created now, we should wait a bit more to get this into core. Especially because it's a potentially disruptive change, waiting one more release is a good idea anyway if we wind up doing #2598382: [policy] Adopt a 6-month feature release schedule for Drupal 7 (similar to Drupal 8) and use pseudo-semantic versioning since a change like this one is a perfect fit for that type of release (which would be on April 20).

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.50 release notes, +7.50 release announcement

Committed to 7.x - thanks!

  1. I also created a new Drupal 7 change notice based on the text in #24: https://www.drupal.org/node/2735873 (decided it was better to do that than to repurpose the older Drupal 8 one from https://www.drupal.org/node/2514152 - I did cross-link them though)
  2. I edited https://www.drupal.org/node/1863034 to indicate that core now protects against clickjacking. Further edits to that are welcome though.

The release that includes this will probably be labeled Drupal 7.50 to indicate it contains some big changes (including this issue).

  • David_Rothstein committed ebd9325 on 7.x 
    Issue #2514136 by pwolanin, David_Rothstein, Fabianx, greggles: Add...

Status: Fixed » Closed (fixed)

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

jaydee1818’s picture

jaydee1818 commented

So we render our help site as an iframe in our web application. This change has prevented it from working as per usual. Can you please just clarify what I need to add to allow the site to be displayed in an iframe? Is it a configuration in setting.php?

jaydee1818’s picture

jaydee1818 commented

ok sorry, I found the answer to my own question:

$conf['x_frame_options'] = '';

David_Rothstein’s picture

David_Rothstein commented

That will result in allowing the site to be embedded as an iframe in any other site. If you want to limit it to a particular site (which is better for security) you could use the ALLOW-FROM option for the header instead (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options).

Probably the easiest way to do that is using the Security Kit module, since it provides an administrative interface for setting this header.

I'll edit the change record at https://www.drupal.org/node/2735873 now to add a pointer to the above resources.