Skip to content

Commit

Permalink
Remove SVG from image types
Browse files Browse the repository at this point in the history 
SVG files should not be treated as images - especially when coming to uploads. An SVG file can contain arbitrary HTML data as well as event handlers in native elements
Refs: https://html5sec.org/#svg
Original report by: Ishaq Mohammed
  • Loading branch information
@daftspunk
daftspunk committed Oct 9, 2017
1 parent 7900807 commit 3bbbbf3
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions src/Filesystem/Definitions.php
View file
Expand Up @@ -182,8 +182,7 @@ protected function imageExtensions()
'bmp', 'bmp',
'png', 'png',
'webp', 'webp',
'gif', 'gif'
'svg'
]; ];
} }


Expand Down

9 comments on commit 3bbbbf3

@datune
Copy link
Contributor

@datune datune commented on 3bbbbf3 Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only half the story.

SVG files displayed with < image > are treated as an image: external resources aren't loaded, :visited styles aren't applied, and they cannot be interactive.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daftspunk how about if we sanitized SVG files on upload to make them safe? https://github.com/darylldoyle/svg-sanitizer

@daftspunk
Copy link
Member Author

@daftspunk daftspunk commented on 3bbbbf3 Oct 19, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this was added in the first place, so can't really comment. If SVG is truly needed, one can override the acceptable types in the form widget itself. This list defines what is considered safe and SVG is not considered safe.

@datune
Copy link
Contributor

@datune datune commented on 3bbbbf3 Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daftspunk

Yes, we can all agree on that. However, the fact of the matter is, even though SVG is a document format, and not an image format, it still makes sense for a MEDIA-Manager to support those SVG's that do represent an image. So, while we can enable uploading SVG's, there are no previews in the media manager grid views.

I wanted to add support for this (just previews), but seeing how they will now be disabled by default, the PR I made a while back will need some changes. It's nothing complicated and will not break anything as far as I can tell.

Still, the question is, is this something you will consider? Cause if not, there is no point in me doing more work on this subject.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@datune I'll merge your changes in if you can fix them to work with the latest code

@osmanzeki
Copy link

@osmanzeki osmanzeki commented on 3bbbbf3 Nov 23, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you are going to enable uploads of svg, they should be sanitized for sure. Its too risky to be able to allow for arbitrary JS to be executed if a client uploads a random SVG found online or worse if there's a public upload form in the frontend that somehow ends up visible in the cms views.

@datune
Copy link
Contributor

@datune datune commented on 3bbbbf3 Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osmanzeki It‘s always been possible to upload svg‘s, this is not what this issue is about. My initial commit was simply enabling previews of svg‘s in the media manager.

@osmanzeki
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Am I correct to assume then that if an svg is previewed in the media manager, that its code is executed and could expose the CMS to xss attacks?

In any case, I think previews for svgs are beneficial for sure, but only if we add the sanitization on uploads with it.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osmanzeki he's just putting the URL of the SVG in the img element. That forces the browser to render it purely as an image which eliminates the issues with XSS. It's not going to be rendered straight to HTML at all.

Please sign in to comment.