Sanitize OR add Content-Security-Policy for SVGs

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

* 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.

@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.

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?

@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.

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

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€

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?

@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)

👋 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).

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.

Tried to address the feedback from the bot in #48.

There were some test fails.

The core.services.yml definition needed to be updated after 📌 Add default autoconfigure to all *.services.yml and remove event_subscriber tags Fixed .
Since this issue is about updating the htaccess we also need to update the scaffold htaccess.

The other test fails didn't look related to me, but I'm not sure.

Great there's finally so much progress on this. SVGs are an important part of modern scalable webistes. Just read the issue description again, though, after all that time and noticed:

* web.config
*Consider adding warning to image fields when people allow SVGs to be uploaded.

Both still valid IMHO. We'd need the web.config to support Windows servers. This is in scope for this issue, I guess.

And - the second point - if people finally allow SVGs to be uploaded a warning should be visible to double-check whether these restrictions actually work. If e.g people haven't enabled AllowOverride on their Apache server, all hell breaks lose. This is such a serious security hazard that we shouldn't take this too light-heartedly. Perhaps a reference to a drupal.org-hosted page where people can download an example evil.svg to easily test their installation? This could be spun-off into a separate issue of course. It might be out-of-scope for this issue, but I still think such a warnig is very important before we can actually reap the benefits i.e. people finally being able to actually upload SVGs without worrying too much about security.

We'd need the web.config to support Windows servers. This is in scope for this issue, I guess.

If this is D11 only, then we are no longer shipping web.config → , so no changes required unless we backport to D10.

Backporting to 10.x can be a followup, IMO, so I removed the web.config from the issue summary.

Consider adding warning to image fields when people allow SVGs to be uploaded.

It seems better for this issue to ship the CSP and consider that good enough. I removed the idea bout labels from the issue summary as well.

Very neat!

Think we will need a change record for the new service and maybe a 2nd CR for the change to htaccess? Know we got a few projects where we disabled the scafold of that so we would have to manually add that change.

Although it'd only become relevant IF people actually uploaded SVGs - and that would IMHO still require a lot more than a mere CR (Explicit warning, Test SVG provided etc). I hope people won't allow it right now without that protection. This issue can just be considered a first step. It only tightens security where there wasn't any security whatsoever up until now. This should perhaps at least be clarified in the CR. People can already add these lines in their htaccess if they want to if they have auto-scaffolding disabled for some reason. It doesn't mean (by itself) that they should/could already allow SVG uploads... Just my 0,02€

I added change records as suggested in #54. Thanks for the review!

Fixing link

Thanks, alexpott. I did a bit more of an update to the issue summary to make it more of a focused summary of the conversation and current status.

There's currently a failing test and I'm unsure if it's related or what the cause is.

@greggles I retested - it was a random fail. So all green again.

I did a code review here and everything looks good. No complaints.

I cannot attest to the quality of the fix itself because content security policy stuff is very much not my forte. But if it's good enough for GitHub, and the security team members who are in this issue and have reviewed, then I trust their judgment.

Tests are green, so I say ship it.

Thanks @phenaproxima!

mr.baileys and I are doing another round of testing and will ping others to get more eyes on the security component of this.

I did some more manual testing and realized that the previous rule allowed `malicious.sVg` to bypass the protection. I updated the htaccess rule to be case insensitive. I've now manually confirmed this protection works on public and private svg files.

Thanks for the feedback, @larowlan. I accepted the one suggested change.

The proposed new header for all private files was introduced by gapple (CSP maintainer) so it feels pretty likely to be widely acceptable.

Applying default-src 'none' to other private files should only have an impact if sites were serving HTML files, and would be irrelevant to other responses like image files, so it might be worth leaving for discussion in 📌 Add a default CSP and clickjacking defence and minimal API for CSP to core Active or breaking out to a separate issue. It could then be done in combination to applying the same change for the files directory htaccess so that private and public files would have consistent behaviour, like SVGs would with this issue.

Thanks for your thoughts, gapple, and setting back to RTBC.

I forgot to mention I confirmed this is the header used by github for svgs content they serve.

I agree with gapple's comments that the change to all private binaryresponses could be broken out to another issue and we would still have most of the value in this issue.

I definitely think this issue should be only about setting a CSP for SVG files - the bit where we add a default CSP for all other private files needs discussion and additional thought.

MR is green again and I addressed the feedback in #65.

Some questions:

• Could the .htaccess change interfere with the use of typical theme-provided SVGs?
• Do SVGs pose risks for users outside of an http request, eg when downloading them and viewing them? This risk of course exists with other file types, but less so for jpg/png images.
• Are typical XML-issues such as XXE, XSLT a possible issue, if so, in what contexts?
• Are we sure all possible threats from SVGs in img/object/background contexts are tackled? Which specs apply?

Some other questions to consider - I've added numbers to them to facilitate discussion.

1 Could the .htaccess change interfere with the use of typical
theme-provided SVGs?

Great question. I don't think it's very common for svgs to use these features (I've only seen it once on a non-Drupal mapping site) but it made me realize the change could be inside of the htaccess for the files directory and therefore only apply to a subset of files. That feels like it reduces the risk this change will negatively impact while still retaining most of the value in protecting sites from malicious files. I asked mr.baileys and he agreed on that idea, but we could certainly discuss it more.

2 Do SVGs pose risks for users outside of an http request, eg when downloading them and viewing them? This risk of course exists with other file types, but less so for jpg/png images.

I think they do pose a risk, but solving it feels like a separate problem to me. I filed ✨ Filter SVGs before uploading by default Postponed and linked it to this as related for followup that could address this risk. The situation today is that site admins are enabling SVG uploads on their site often and there is no protection for a variety of risks that creates. This issue attempts to solve one of the risks (XSS), but is not a complete protection against all risks.

3 Are typical XML-issues such as XXE, XSLT a possible issue, if so, in what contexts?

As far as I understand those risks they are not relevant on a typical Drupal site and would only be relevant on sites that parse the XML of the SVG file. These changes don't affect that for better/worse.

* Are we sure all possible threats from SVGs in img/object/background contexts are tackled? Which specs apply?

I'm not sure this mitigates all possible threats (several additional threats are mentioned in this issue already). I'm sure the change in this issue is progress in securing sites against cross site scripting, but not a solution to all risks of SVG files.

@greggles great idea only changing the .htaccess in the files directory. This move made me think 2 things.

1. We need a update function to rewrite existing sites .htaccess files I think
2. What about sites which use nginx or another server... what would be neat is something on the status report that confirms this protection is working. That way we could point to documentation on how to set this up.

I added an update hook (system_update_11103) to update existing .htaccess files.

2. What about sites which use nginx or another server... what would be neat is something on the status report that confirms this protection is working. That way we could point to documentation on how to set this up.

I added a hook_requirements-check to test for the presence of a CSP-header on public SVG-files. I'm assuming that if there is a CSP-header, the site owner knows what they are doing, if it is missing we issue the warning. We could make it more strict and check the contents of the header if necessary.

Currently linking to the change record for more information.

I don't think that will work properly: The URL of the website would have to be specified from the command line. That's not necessarily the case. Neither is it guaranteed that the web server is really routed in such a way that it can call itself. And the check will only check for the existence of any header. Not that the protection really works. So, I have some doubts it's the best approach. IMHO much better would be a test on /admin/reports/status. This test could be an entry that is initially always under the "Warnings" category and says

"SVG security could not be tested properly. Please make sure you have an appropriate Content-Security-Policy header configured before allowing the upload of SVG files".

If there are any JS errors or JS is disabled in the browser this warning will just remain. We won't convey a false impression of safety.

Then there's an object-tag after that entry including an SVG file under public:// with a JS payload. An object tag and not an img tag because in this case scripts are executed. That JS payload would remove that entry and put a different entry under the "OK/Verified section".

Then there's a fallback JS that would move it to the "Error" category and say: "SVG security test failed."

A check along these lines would be way better IMHO. It avoids the problems mentioned above and is really a functional test that will make absolutely sure that scripts are not executed.

@rgpublic: I understand your concerns, consider that code a poc (I think this might also be a candidate to be handled in a follow-up issue)

A check along these lines would be way better IMHO. It avoids the problems mentioned above and is really a functional test that will make absolutely sure that scripts are not executed.

Would there be a risk that the JS-test gives a false positive: embedded scripts blocked due to browser/browser config on the administrator's machine, while not blocked for other contexts/browsers/users? (compared to checking for the header, which guarantees that the change made in this issue works across the board)

@mr.baileys: A very valid concern IMO, thanks for bringing this up! The server-based check, however, could also lead to false positives: If the CSP header is there but configured in such a way that it won't properly apply we have a problem right there. In this case we'd need to parse the whole CSP and see if it really applys. Or if the URL leads to a proxy and the proxy returns a different/404 page which has that header etc. Just two problems I see with a server-based check (apart from the other problems mentioned). We *could* in theory add a different, JS based check and fetch the header with XMLHttpRequest from JS. This would circumvent at least the domain not reachable/domain not configured from the command line problem and give an additional line of security.

If we checked that JS inside the object SVG is not executed AND fetch the headers via JS and check whether they are really there, I think this would be quite safe.

@rgpublic system_requirements is how admin/reports/status is generated - I agree we should only do this when not in the CLI. We already have quite a few requirements that only run when if ($phase === 'runtime' && PHP_SAPI !== 'cli') {

The current implementation in system_requirements is problematic because of adding the file to public... and then removing it. That's not free. Not sure I have a better idea yet. And maybe that means punting the system_requirement part to a follow-up is a good idea.

The concept of the check for CSP header on the status page feels a like a pretty big additional functionality for core. It seems like something security_review could handle, so I filed ✨ Check for CSP on private and public SVG files Active to track it there.

@alexpott: Ah, understood. Thanks for the clarification. I'm not sure if it's necessary to remove the test file again. It could be created if it doesn't exist yet and otherwise just used. Still, I think a JS based approach would be safer. At the very least the CSP header should be checked for more than its mere existence, I guess.

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.