FilterHtmlImageSecure filters out valid local svg images

I ran into this exact same problem when working on #2842780: Add a token for the site logo.

Since svg's are perfectly valid resources for a src attribute of an img element according to the HTML5 specification paragraph 4.7.1, naturally I'd suppose it should be supported by Drupal as well. (Though there are more issues ragarding svg files.)

I'd like to propose a third option; in addition to getimagesize(), also check if the file is a valid svg file. For instance by checking its MIME-type.

If fact, PHP does not even recommend using getimagesize to detect if a file is a valid image:

Caution
This function expects filename to be a valid image file. If a non-image file is supplied, it may be incorrectly detected as an image and the function will return successfully, but the array may contain nonsentical values.
Do not use getimagesize() to check that a given file is a valid image. Use a purpose-built solution such as the Fileinfo extension instead.

So we might want to get rid of that as well?

What about replacing

if (@getimagesize($local_image_path)) {
  // The image has the right path. Erroneous images are dealt with below.
  continue;
}

with

$finfo = finfo_open(FILEINFO_MIME_TYPE);
$type = finfo_file($finfo, $local_image_path);

if (isset($type) && substr($type, 0, 6 ) === "image/") {
  // The image has the right path. Erroneous images are dealt with below.
  continue;
}

Though this might not accept certain extensions that getimagesize() did accept (swf, jpc, jpf, jb2 and swc), I think the acceptance of svg over those could be a valid trade off. (getimagesize() would accept all IMAGETYPE_XXX constants, where this method would only accept a part of them http://php.net/manual/en/function.image-type-to-mime-type.php)

I forgot the @ in case the file does not exist.

Hmm, it appears that some webservers configurations will recognize the files as text/plain or text/html rather than the image/svg+xml I was aiming for. So this does not solve the problem...

We'd probably need some other solution then...

So I have done some additional searching, this actually has been requested as a PHP feature; Request #71517 Implement SVG support for getimagesize() and friends, but it has not received a lot of attention yet so it probably won't happen soon...

Furthermore my suggestion from #3 to rely on MIME-types is not safe because finfo_open only checks for known patterns in files, which can be deliberately added, making the files appear as another MIME-type. (Perhaps that should be added as a comment above getimagesize()?)

And third, SVG files can contain JavaScript (MDN example). I suppose we'd want to sanitize or block that.

So, the best I could come up with would be something like this:

// Perhaps first check for the .svg extension before validating to not waste valuable processing power?

$dom = new \DOMDocument;
$dom->load($local_image_path);
if ($dom->schemaValidate($custom_svg_schema)) {
    continue; // Valid document.
}

Where $custom_svg_schema could be an adapted version of the svg schema, which does not allow script tags and would cause the validation to fail.

Basically everything described in this StackOverflow post.

But I really have no experience with this...

Any progress on this? Our authors can choose SVG files in the WYSIWYG editor, but when they preview or publish the page, they are seeing a red X icon due to this bug.

It is still an issue, with Drupal core 8.7.x.

There are two problems here.

• A filter that intends to limit access to local images is also checking that it is in fact an image (which it should or should not do). Then again, you only want to enable this as some safety measure to prevent users from pulling in unknown (untrusted) content. So it does make sense that the filter also checks if the image is valid.
• As great as SVG images are, they can be terrible dangerous. If you read through https://blobfolio.com/2017/03/when-a-stranger-calls-sanitizing-svgs/ you can see all kinds of exploits people can inject in your website if someone manages to upload a tampered SVG image. So also for SVG images (perhaps specifically for SVG images) it makes sense to check if it is a safe file.

At the bottom of the article is a link to an issue in Wordpress which attempts to solve this by using a huge whitelist: https://core.trac.wordpress.org/attachment/ticket/24251/24251.2.diff

UPDATE, after reading #2868079-14: Add a default Content-Security-Policy-header for svg files I understand that SVG's in HTML img tags should not pose a risk. That is great for this issue because the sanitizing process should not be a blocker then.

I think something like this could work.

And I think this should suffice as a test

Had a typo in classy :(

Switched to using the built in mime type guesser and also asserting that external SVG files are invalid.

Built on latest dev.

For lower version one can use MimeTypeGuesser::guess() Instead.

Patch fails to apply to 9.5

Rerolled

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Will also need an issue summary update before review.

Please review.

Thanks!

The issue summary still needs to be updated.

I think this issue (#2855653) and #2998318: Allow local svg image with "Restrict images to this site" filter may be duplicates of each other.

I'll close this issue in favor of #2998318: Allow local svg image with "Restrict images to this site" filter so we can focus our efforts in the related issue