Sanitize OR add Content-Security-Policy for SVGs

Created on 9 April 2017, over 7 years ago
Updated 23 October 2024, 6 days ago

Problem/Motivation

People want to use SVGs on their websites. Unfortunately SVGs are a security disaster as they allow embedded CSS and JS. Which means that allowing users to upload SVGs should be viewed in the same was as allowing them to upload JS or create inline JS. That is to say that permission to do that should be require a permission that has a security warning.

Here is a really good article about SVGs - https://www.owasp.org/images/0/03/Mario_Heiderich_OWASP_Sweden_The_image... - the tldr; is

Allowing SVG for upload == allowing HTML for upload

Proposed resolution

Requiring additional permissions to upload SVGs is probably not going to happen since we don't have that type of granular permissions on fields. Also people want to make image styles play nice with SVGs - see #2652138: Image styles do not play nicely with SVGs β†’ .

Maybe we can leverage the content security policy header to prevent this. This is how github does it - see https://github.com/github/markup/issues/289.

Remaining tasks

Add Content Security Policy headers for SVGs using .htaccess and web.config
Consider adding warning to image fields when people allow SVGs to be uploaded.

User interface changes

None

API changes

None

Data model changes

None

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

image.module

Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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.

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.

  • πŸ‡§πŸ‡ͺBelgium mr.baileys πŸ‡§πŸ‡ͺ (Ghent)

    mr.baileys β†’ made their first commit to this issue’s fork.

  • Merge request !9919Add CSP headers for svg files. β†’ (Open) created by mr.baileys
  • πŸ‡§πŸ‡ͺBelgium mr.baileys πŸ‡§πŸ‡ͺ (Ghent)

    * Converted the patch to a MR, rerolled for 11.x
    * Added a test to confirm the presence of the CSP header for file downloads.

    I'm not sure we should be adding the CSP header (Content-Security-Policy: "default-src 'none') for none image/svg+xml downloads, as that seems out of scope for this issue and might break existing functionality.

  • Pipeline finished with Failed
    6 days ago
    Total: 135s
    #318213
  • πŸ‡©πŸ‡ͺGermany rgpublic DΓΌsseldorf πŸ‡©πŸ‡ͺ πŸ‡ͺπŸ‡Ί

    @mr.baileys: It should probably at least work properly with the "csp" module. An easy solution would be to the set the priority of the respond event such that our event handler is guaranteed to run after their event handler. Then we could check whether the header is already set and do nothing in this case and just hope that the user knows what they're doing.

  • Pipeline finished with Failed
    6 days ago
    Total: 135s
    #318243
  • πŸ‡§πŸ‡ͺBelgium mr.baileys πŸ‡§πŸ‡ͺ (Ghent)

    Re-titling, since the scope of this issue has shifted to adding a content security policy header, rather than sanitizing the file.

    @rgpublic: makes sense, I added a condition so that the default CSP header is only set if none has been set earlier. I think it's up to CSP and other contrib modules to alter/set handler priorities?

  • πŸ‡©πŸ‡ͺGermany rgpublic DΓΌsseldorf πŸ‡©πŸ‡ͺ πŸ‡ͺπŸ‡Ί

    @mr.baileys: Yes, you are probably right. Currently they don't do this (I just checked, I see no explicit priority in their code), but I hope/assume their code gets after ours anyway because this is in core and their code is in contrib. If not, they could easily solve this by adding a priority - so: fine for me.

  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA

    There's also seckit β†’ which also can set the CSP headers.

  • πŸ‡©πŸ‡ͺGermany rgpublic DΓΌsseldorf πŸ‡©πŸ‡ͺ πŸ‡ͺπŸ‡Ί

    The evil thing is: If we set sth. here and anyone keeps overwriting it they can inadvertently create a security hole because people can upload SVGs with a XSS payload again :-(

    But I don't want to spoil the party. Glad this is finally moving forward.

    In a perfect world, core would provide a CSP service that anyone can *add* their stuff to if they want so nothing gets destroyed except intentionally. But this is probably way out-of-scope for this issue...

    It's quite an unfortunate situtation, but OTOH seckit / csp etc. are not modules anyone not savvy with these security issues will probably install lightheartedly. So perhaps we can just assume IF they change sth. they should know that then THEY have full responsibility over what security problems arise. Perhaps these contrib module owners should be informed that this is happening and add an appropriate readme note or sth. to their installation instructions...

    Just my 0,02€

  • πŸ‡¨πŸ‡¦Canada xmacinfo Canada

    There's also seckit which also can set the CSP headers.

    Can we set the CSP heasers on both public and private served files? Is Seckit able to add CSP headers on files?

  • πŸ‡§πŸ‡ͺBelgium mr.baileys πŸ‡§πŸ‡ͺ (Ghent)

    @xmacinfo: for public files, I think the only way to add the header is through via web server directives (MR here includes a change to .htaccess for this reason)

  • πŸ‡¨πŸ‡¦Canada gapple

    πŸ‘‹ CSP module maintainer here.
    CSP module provides APIs for modules to amend a site's policy β†’ . In most cases it's pretty straightforward, but gets more involved for cases where it's not just adding an additional external domain to a directive. The goal of the module is that anyone can install the module for a security improvement, that it's easily configurable for site builders to add exceptions not captured automatically (& ideally harden the default config with basic knowledge), but also flexible for developers to handle any more complex uses (and to do so safely without breaking other site functionality).
    Module subscribers with the same priority will execute after core, so it's the module's responsibility to preserve/amend/replace any value previously set by core.

    CSP has its own issue for applying a different policy to private files ( ✨ Provide different CSP policy for private files Active ), though there hasn't been any work done on it so far. If a special value is added to core for SVGs, then I'll update the module to match. If there's consensus on what should be added to core, I'll probably have a new release of the module before it's committed to core πŸ™‚.
    Alternately, the CSP module would be a good way to evaluate a potential solution (at least for private files, I'm not sure about the module altering htaccess), with users who have opted in to using a policy on their site. It would have to be an optional feature for the current major version, but could be enabled by default for new modules installs (and a site status message could prompt existing sites to try enabling it).

    This issue for core would add a static policy that can be configured in a service parameter: πŸ“Œ Add a default CSP and clickjacking defence and minimal API for CSP to core Active
    Instead of providing a single policy set, maybe it should be keyed policies to allow for additional cases (e.g. default, private-files, svg, etc, each with report-only and enforced values).

Production build 0.71.5 2024