Add a default CSP and clickjacking defence and minimal API for CSP to core

Created on 27 June 2015, about 10 years ago
Updated 16 January 2023, over 2 years ago

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

Problem/Motivation

Content security policy is a browser feature available that helps prevent XSS attacks based on headers sent by the site.

For CSP spec see: http://www.w3.org/TR/CSP/
https://www.owasp.org/index.php/Content_Security_Policy
https://www.owasp.org/index.php/Clickjacking_Defense_Cheat_Sheet

Inline JS is not compatible with enabling a reasonable secure content security policy, so this issue is postponed until the Drupal settings are fixed in the related issue

Proposed resolution

Implement a basic and reasonably secure CSP header for Drupal core, such as

Content-Security-Policy: default-src 'self'; frame-ancestors 'self';

Possibly (or as a follow-up or in contrib): Add a callback to receive and log CSP violation reports to watchdog. e.g. with CSP report-uri directive like:

Content-Security-Policy: default-src 'self'; frame-ancestors 'self'; report-uri /system/csp-report-logger;

Likely this reporting should be supported only as a something that can be temporarily enabled for debugging. It has obvious potential for abuse (DoS attacks, bogus data, etc) such as outlined at https://www.virtuesecurity.com/blog/abusing-csp-violation-reporting/

Remaining tasks

Implement

User interface changes

Possibly an admin page to configure some aspects of the CSP (optional for 8.0.x)

API changes

API addition to allow modules to alter or add to the CSP header for each page/or response event.

Beta phase evaluation

<!--Uncomment the relevant rows for the issue. -->
πŸ“Œ Task
Status

Active

Version

10.1 ✨

Component
BaseΒ  β†’

Last updated about 1 hour ago

Created by

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

    I guess the X-Frame-Options header that core adds via FinishResponseSubscriber should also be removed when this is done. The "frame-ancestors" directive in the CSP header is well supported and obsoletes X-Frame-Options.

  • πŸ‡¨πŸ‡¦Canada gapple

    With IE no longer being supported, core could replace its default X-Frame-Options: SAMEORIGIN with Content-Security-Policy: frame-ancestors 'self'. Starting to always add a default CSP header could cause problems for a site that sets X-Frame-Options: DENY though. (It looks like IE was the only browser to support ALLOW-FROM).

    A possible transition is:
    [11.1] Add a late-subscriber that translates x-frame-options to a CSP frame-ancestors if the response does not have a CSP header (assume if a CSP header is set but does not include frame-ancestors, then it was intended to allow any frame parent). Issue a deprecation warning if the response has a value other than X-Frame-Options: SAMEORIGIN to notify users to use CSP instead.
    [12.0] Remove the late subscriber, and replace X-Frame-Options with a static CSP policy that includes frame-ancestors 'self'. A site that uses the Content-Security-Policy module, SecKit, or a custom listener to set the CSP header will override this default value.

    My suggestion for a minimal static policy that core can add by default:

      Content-Security-Policy: script-src * 'unsafe-inline'; object-src 'none'; frame-ancestors 'self'
    

    The simplest way to allow modifying the default CSP (or adding a CSP-Report-Only), would be to have a container parameter. Users wanting a more flexible implementation can use the CSP module.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 692s
    #136140
  • Pipeline finished with Failed
    over 1 year ago
    Total: 195s
    #136173
  • Status changed to Needs review over 1 year ago
  • πŸ‡¨πŸ‡¦Canada gapple

    A slightly rough MR:
    - Introduce a new service parameter for setting static CSP values
    - A late acting response subscriber will translate X-Frame-Options to Content-Secutity-Policy: frame-ancestors
    - If a CSP policy is set (via service parameter, or another module like CSP or seckit), then that value is not changed.
    - The X-Frame-Options header is always removed - either it's being replaced by core with an equivalent CSP policy, or we assume that the CSP policy set by the user has their desired frame-ancestors value (including the option of being omitted).
    - If X-Frame-Options is not set to SAMEORIGIN, then a deprecation warning is issued to use CSP to set the value. (If the value is still the default SAMEORIGIN, then a future version of Drupal changing to frame-ancestors 'self' will not change browser behaviour).
    - ALLOW-FROM is ignored by modern browsers (and equivalent to not sending the X-Frame-Options header), but is translated to a CSP value if used.

    In a future version of Drupal:
    - The service parameter can be set to a default enforced CSP value
    - The late acting event subscriber can be completely removed
    - Core can stop setting X-Frame-Options
    - Modules (such as CSP or seckit) will still override core's default (now static) value.

    Why the CSP policy value:
    - script-src * 'unsafe-inline' will only block eval(). Not including 'unsafe-inline' would have the potential to break existing sites (and hopefully its presence leads people to explore making sure it's not required...)
    - object-src 'none' is recommended to block legacy HTML elements
    - frame-ancestors replaces X-Frame-Options
    - I don't think there's other values that can safely be added as a default enforced policy directive without a reasonable risk of negatively affecting some sites. (maybe base-uri 'self'?)

  • Status changed to Needs work over 1 year ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

  • Assigned to gapple
  • πŸ‡¨πŸ‡¦Canada gapple

    Update issue summary

  • Pipeline finished with Canceled
    about 1 year ago
    Total: 353s
    #176273
  • Pipeline finished with Failed
    about 1 year ago
    Total: 178s
    #176277
  • Issue was unassigned.
  • Status changed to Needs review about 1 year ago
  • πŸ‡¨πŸ‡¦Canada gapple

    Rebased and fixed the bot's previous issue.

    In my previous comment I wasn't sure of the core dev timeline, but this looks like it's something that could be put in 10.4, with the compatibility later removed in 11.0.0.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 871s
    #176279
  • Pipeline finished with Failed
    about 1 year ago
    Total: 870s
    #177064
  • Pipeline finished with Failed
    about 1 year ago
    Total: 909s
    #177090
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Added some comments to MR.

  • Status changed to Needs review about 1 year ago
  • Pipeline finished with Failed
    about 1 year ago
    Total: 906s
    #177874
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can be rebased so tests appear green please.
    Also is it meant to be draft?

    For the CR you can actually go ahead and create it just don't check published yet till the issue is merged.

  • Pipeline finished with Failed
    11 months ago
    Total: 562s
    #275340
  • Pipeline finished with Success
    11 months ago
    Total: 424s
    #275408
  • Status changed to Needs review 11 months ago
  • πŸ‡¨πŸ‡¦Canada gapple

    - Squashed and rebased
    - Draft Change Record created β†’
    - Postponed issue for 12.x created πŸ“Œ [12.x] Set default Content-Security-Policy in services.yml Postponed

    I'm not sure why tests are failing on ImageStylesPathAndUrlTest:L315

    Remaining questions:
    - Service parameter name: http.response.content_security_policy (like http.response.debug_cacheability_headers) or csp.config (like cors.config) or ?
    - Should any X-Frame-Options header present on a response be removed, since browsers will ignore it when CSP is present?
    - Review additional directives to add by default: script-src * 'unsafe-inline'; object-src 'none'

  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    False positive

  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

  • πŸ‡¨πŸ‡¦Canada gapple

    Something that came up in #2868079: Add a default Content-Security-Policy-header for svg files β†’ is that there may be a need for different policies in certain cases - uploaded files should probably have scripts and styles blocked, though SVGs may need inline styles, image data, and fonts. Private files can have the header applied from the event subscriber, but public files would require an addition to .htaccess.

    Instead of a single set of policies, the service parameter could be keyed by use:

    default:
      report_only:
      enforced: "script-src * 'unsafe-inline'; object-src 'none'; frame-ancestors 'self'"
    private_files:
      report_only:
      enforced: "default-src 'none'"
    svg:
      report_only:
      enforced: "default-src 'none'; style-src 'unsafe-inline'; image-src data: font-src: data:"
    
  • Status changed to Needs work about 1 month ago
  • For SVG images,

    I have spotted this error, which says I need `data:` whitelisted.

     Content-Security-Policy: The page’s settings blocked the loading of a resource (img-src) at data:image/svg+xml,%3csvg width='24' hei… because it violates the following directive: β€œdefault-src 'self'” js_J79s-Ta4InFzicdahjWbsmc-zVw1kaXPV9gHaoS8QUg.js:3:71535
    

    Are there plans for making it completely CSP compatible, ie. without using the `data:` source?
    If yes, do you have an issue created for it?
    If no, can you provide a justification on that please?

Production build 0.71.5 2024