Filter SVGs before uploading by default

The SVG Image module does sanitization in the Formatter. https://git.drupalcode.org/project/svg_image/-/blob/3.x/src/Plugin/Field...

One option, but not a very practical one, is to save SVG files as private files, not public. Then (somehow) Drupal could sanitize them on output.

I think a better option is to provide a file validator using a suitable PHP library. Then site owners and contrib module maintainers would have the option of adding SVG validation to any field that allows uploading SVG files. We could recommend using it and add it to the Standard and/or Umami profiles. But site owners would have the freedom to use it or not.

This option requires finding a suitable library that implements something like isSvgSafe(), returning bool. I have not researched such libraries.

One concern I have heard is that new vulnerabilities might be discovered in SVG files. The library we use might be updated, preventing new uploads of malicious files, but that would not help if such files have already been uploaded. I think a good way to deal with that would be to provide a field formatter that returns a boolean (Safe or Unsafe). We could add that formatter to /admin/content/files. After updates to the SVG library, site owners could filter on that value to search for existing, unsafe SVG files.

This option requires finding a suitable library that implements something like isSvgSafe(), returning bool. I have not researched such libraries.

I think this library is by far the most used in the PHP world and it seems actively maintained: https://github.com/darylldoyle/svg-sanitizer
It can be used like this:

$sanitizer = new enshrined\svgSanitize\Sanitizer();
$sanitizer->sanitize(file_get_contents(__DIR__.'/../xss.svg'));
$sanitizer->getXmlIssues();

And will return an array of problems found in the SVG.
If the array is empty we can assume the SVG is safe.

Hi, I'm co-maintainer of SVG Image Field. Like SVG Image in comment #4 above, we also sanitize as part of the display formatter.

It looks like the consensus on https://www.drupal.org/project/drupal/issues/2868079 has landed on using CSP headers, which just moves this issue further down the infrastructure stack, and as a result does not protect Drupal sites that don't run on Apache.

What if core introduced a kind of special hybrid storage scheme called unsafe:// or perhaps quarantine:// or similar for handling potentially unsafe file types?

It would partially function like private://, whereby files aren’t directly web-accessible, but with an additional mechanism: a sanitized cache layer in the public filesystem.

Unsafe files could be processed into a new public directory such as sites/default/files/sanitized/, which would act similarly to sites/default/files/styles/.

This “sanitized” directory would serve cached, sanitized versions of unsafe files and be flushable/regenerable (just like styles) when sanitization logic changes or new vulnerabilities are discovered, while leaving the uploaded files intact in their original state, safely stored outside webroot.

Since this scheme may need to sit behind either private and/or public files, it might require an architecture that either stacks schemes and dynamic scheme swapping based on filetype being uploaded, or (worse) doing something like private-unsafe:// and public-unsafe://. But perhaps the best option is to take it one step further and quarantine all files uploaded via the web UI, and force them to pass through a hookable or Subscribable sanitization step on its way to the public or private folders, allowing contrib modules to create additional sanitization plugins for any file type.

A generalized subsystem would avoid the complexity of configuring CSP headers at the server level for this one-off SVG-sanitization problem space while still providing a scalable way to control and refresh all kinds of sanitized file outputs.

To clarify: this issue and #2868079: Add a default Content-Security-Policy-header for svg files are not mutually exclusive and I definitely think we should do both.

IMHO adding a new validator that checks if the uploaded SVG is dangerous and refuses the upload if it finds problems is the easy solution.
While I see the value of sanitizing files when they are served, I think implementing this for public files would too much complexity and new concepts (but it could be implemented in a contrib module as a POC).

These two issues aren’t mutually exclusive, and sanitizing on upload is clearly the straightforward path.

However, since Benji raised the idea in #5 of staging files in private first, I wanted to extend that concept a bit further. There was also discussion in the postponed internal security issue about potentially violating user expectations. Specifically, around the expectation that Drupal always allows files to be uploaded in their original form but then altered at render time. That concern is what led me to think about introducing a separate file schema.

If this feels out of scope, I’m fine splitting it into a dedicated core issue. That said, I’m skeptical that an entirely internal architecture change around managing file uploads would gain much traction in contrib without a clearer external impact.

Yes I think we should keep this issue about sanitizing/filtering on upload.
But your idea is worth exploring so I think you could open a separate issue about it.

There was also discussion in the postponed internal security issue about potentially violating user expectations. Specifically, around the expectation that Drupal always allows files to be uploaded in their original form but then altered at render time.

Indeed, that's why I was suggesting a validator that refuses dangerous files instead of a sanitizer that alters the uploaded files.
This feels more in line with how Drupal usually works.

The last point I’d reiterate is that blocking files outright on upload has a structural downside: it doesn’t give us a clean answer for files that slip through and are only later determined to be malicious.

If the underlying isSvgSafe() upstream library logic evolves, e.g., in response to newly discovered vulns., then site admins or the security team are left with a disruptive choice:

- delete the file (in line with the "no soup for you" approach of blocking files outright, but risks breaking file references)
- attempt in-place sanitization without a preserved source of truth

That’s a muddy remediation path, which amounts to a non-issue with the quarantine approach.

If we allow uploads and quarantine the originals, serve cached sanitized versions, then addressing new vulns is just a cache-rebuild away. We can reprocess the canonical input at any time and regenerate the public derivatives. No need to mull over a retroactive content decision.