PR-staging

Success

User tests: Successful: Unsuccessful:

avatar bembelimen
bembelimen
15 Oct 2014

At the moment it's not possible (without hacking the path with Firebug/Inspector) to use SVG files for e.g. menu images or with the "image button" in the article edit form.

Apply path => SVG files can be chosen.

Votes

# of Users Experiencing Issue
2/4
Average Importance Score
4.00

avatar bembelimen bembelimen - open - 15 Oct 2014
avatar jissues-bot jissues-bot - change - 15 Oct 2014
Labels Added: PR-staging
avatar infograf768
infograf768 - comment - 15 Oct 2014

@test
Although I can now insert an SVG image indeed, I still can't upload any through media manager
(added image/svg mime type, and legal extension)
I still get "Not a valid image."

avatar brianteeman brianteeman - test_item - 15 Oct 2014 - Tested unsuccessfully
avatar brianteeman brianteeman - alter_testresult - 15 Oct 2014 - inforgraf: Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 15 Oct 2014

@test
like jean marie this is not enough as it still doesnt allow you to upload an svg.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar bembelimen
bembelimen - comment - 15 Oct 2014

Hello,

that was on purpose, because I don't like patches with several new stuff to test. So the plan was:

  • first add the possibility to choose SVG files in the image manager
  • if accepted, I wanted to create a new patch which extends the upload for uploading SVG files (which is a bigger code change than this patch)

    This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.
avatar brianteeman
brianteeman - comment - 15 Oct 2014

Well we cant have one patch without the other and it is harder for people to text a patch if it relies on another one so it would be better if you create just one patch

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar bembelimen
bembelimen - comment - 15 Oct 2014

Hello Brian,

I see your point and in general I would definitely agree with you. But in this case (I think) it's not a good idea.
The main reason is, that Joomla! has a XSS filter for uploaded files, which blocks inline code (like <!DOCTYPE, <!--, and (perhaps) other relevant stuff for SVG). So we need a bigger rewrite of the upload handling and we have to be careful, when we change stuff there. So in my opinion, I still think, that we should separated the two patches (they depend only indirect) and you have to upload the SVG files via FTP client for testing.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar brianteeman
brianteeman - comment - 15 Oct 2014

Yes but the problem is that their is zero point in committing this pull
request on its own as it will just lead to confusion

On 15 October 2014 12:59, bembelimen notifications@github.com wrote:

Hello Brian,

I see your point and in general I would definitely agree with you. But in
this case (I think) it's not a good idea.
The main reason is, that Joomla! has a XSS filter for uploaded files,
which blocks inline code (like <!DOCTYPE, <!--, and (perhaps) other
relevant stuff for SVG). So we need a bigger rewrite of the upload handling
and we have to be careful, when we change stuff there. So in my opinion, I
still think, that we should separated the two patches (they depend only
indirect) and you have to upload the SVG files via FTP client for testing.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/4674
http://issues.joomla.org/tracker/joomla-cms/4674.


Reply to this email directly or view it on GitHub
#4674 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar bembelimen
bembelimen - comment - 15 Oct 2014

I still see a point in commiting this patch, because I can offer my authors a folder of SVG files which they can use (no upload required).

To infograf768: SVG files are no images, they are "normal" files. So they cannot be validated as images (you have to add the extension to the "Legal Extensions (File Types)" parameter).

Depending on the SVG structur you'll trigger the XSS check in Joomla! or not. So in general you can upload SVG files without a new patch, if they have a structur, Joomla! likes ;) (no <!DOCTYPE in the first 265 chars e.g.)

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar wilsonge
wilsonge - comment - 16 Oct 2014

:-1: for this PR. Reason is that in this case we call getimagesize on the image. This is a really bad idea as it returns false for any SVG's

This is also the reason why you can't upload them in media manager - this line always returns false for an svg https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/helper/media.php#L139 so you get the error brian quoted.

If we want to support SVG's in media manager (and with my worker for Virya Group hat on I really REALLY do). We're gonna have to rewrite some parts of media manager to reduce our dependency on getimagesize because all whilst we use that support for SVG's is gonna be bad-none

avatar bembelimen
bembelimen - comment - 16 Oct 2014

I just can repeat myself:
It is possible to upload SVG files, just allow them in the options under "Legal Extensions (File Types)" and (if you have activated the mime-check) add it to the mime list. Due the fact, that SVG are no image files but vector graphics (XML based) it's not necessary to add "svg" to the "Legal Image Extensions (File Types)" option. So the only thing which is missing is the possibility to choose them after uploading in the media manager when choosing a image. => so when we have applied this patch we can now not only upload SVG files (which is already implemented) but also choose them.

Some stuff you could insert into a SVG file will trigger the XSS check (1). So if someone needs better support for SVG files while uploading them, then he/she/it could improve the handling with a new patch...but this does not associated with this patch, because it just adds basic support for choosing.

(1) Just make sure, that the first 256 characters does not contain invalid strings


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar HARDIKHIRAPARA HARDIKHIRAPARA - test_item - 17 Oct 2014 - Tested successfully
avatar jespersoelundhansen jespersoelundhansen - test_item - 17 Oct 2014 - Tested unsuccessfully
avatar Symatic
Symatic - comment - 17 Oct 2014

cant upload *.svg file

avatar brianteeman
brianteeman - comment - 17 Oct 2014

@symatic thanks for testing. this PR only adds the ability to see them in the list and select them - it does not allow you to upload them. Weird I know but thats what @bembelimen wanted to do

avatar lausianne
lausianne - comment - 17 Oct 2014

@test
When trying to upload in media manager after applying the patch and adding svg to legal extensions I get this error:
"Notice: Possible IE XSS Attack found."

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar lausianne
lausianne - comment - 17 Oct 2014

Without patch, it's the same error. Tried several svg images.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar brianteeman
brianteeman - comment - 17 Oct 2014

As said multiple times above this is not to enable image uploads but just
to enable the ability to see SVG files in the image list. Not my idea to
do it this way ;(

On 17 October 2014 13:44, Ralf Longwitz notifications@github.com wrote:

Without patch, it's the same error. Tried several svg images.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/4674
http://issues.joomla.org/tracker/joomla-cms/4674.


Reply to this email directly or view it on GitHub
#4674 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar bembelimen
bembelimen - comment - 17 Oct 2014

Yep, that's a "bug" in the XSS check.

Try this file (unzip first): http://stuff.benjamintrenkle.de/address-book.zip

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar marcodings
marcodings - comment - 17 Oct 2014

hmm apparently forget to commit my answer yesterday

@bembelimen thanks for this PR, svgs are and will become more important imho

1) you are adding svg, but you probaly should add svgz as they both are typically listed as part of the mime type

2) as for the last post, uploading svg theoretically should work. In practice i have not been able to succeed with a wide range of files, most w3c validated.
Correctly configured i get "Possible IE XSS Attack found." ( instead of "This file type is not supported." ).

I'll look into surpresssing the XSS. Also in relation to the new media manager due for 3.5*+? #3839

avatar lausianne
lausianne - comment - 17 Oct 2014

@brianteeman Alright, sorry. Still, this is not the kind of error message I'd expect.
@bembelimen Could you please add clearer testing instructions?

avatar brianteeman
brianteeman - comment - 17 Oct 2014

@marcodings you cant supress the error message - there is a potential
security issue (not for this public list)

On 17 October 2014 14:30, Ralf Longwitz notifications@github.com wrote:

@brianteeman https://github.com/brianteeman Alright, sorry. Still, this
is not the kind of error message I'd expect.
@bembelimen https://github.com/bembelimen Could you please add clearer
testing instructions?


Reply to this email directly or view it on GitHub
#4674 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar marcodings
marcodings - comment - 17 Oct 2014

@brianteeman i would not even consider removing it bluntly, but looking into how to selectavely surpress / replace it with an alternative test specifically for svg.

avatar brianteeman
brianteeman - comment - 17 Oct 2014

(contact me off list please)

On 17 October 2014 14:35, marcodings notifications@github.com wrote:

@brianteeman https://github.com/brianteeman i would not even consider
removing it bluntly, but looking into how to selectavely surpress / replace
it with an alternative test specifically for svg.


Reply to this email directly or view it on GitHub
#4674 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar brianteeman brianteeman - alter_testresult - 18 Oct 2014 - inforgraf, : Not tested
avatar brianteeman brianteeman - alter_testresult - 18 Oct 2014 - inforgraf: Not tested
avatar brianteeman brianteeman - test_item - 18 Oct 2014 - Tested successfully
avatar brianteeman
brianteeman - comment - 18 Oct 2014

OK on the basis that yes the PR does what it says "adds svg to the list" and nothing more than that I am going to change my test to a success. I still have "issues" if this is worth committing as it is

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar brianteeman brianteeman - alter_testresult - 18 Oct 2014 - infograf768: Tested unsuccessfully
avatar jacxx jacxx - test_item - 18 Oct 2014 - Tested successfully
avatar brianteeman
brianteeman - comment - 28 Oct 2014

I am setting this to Needs Review so that the maintainers can make a decision. The PR does what it says but it might lead to confusion as it does not provide full support for SVG

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar brianteeman brianteeman - change - 28 Oct 2014
Status Pending Needs Review
avatar proweb
proweb - comment - 17 Mar 2015

so what?
2015 on the yard and still can't upload svg file with media-manager...

avatar wilsonge
wilsonge - comment - 17 Mar 2015

The problem with SVG's is that by definition of how they function you can upload malicious code as part of them - see http://blog.guya.net/2014/02/17/svg-for-fun-and-phishing/ for example. This means anyone who can upload regular images can theoretically upload malicious code. This makes it really hard to find a valid solution that works. The company I work for do a huge amount of work with SVG's so believe me I want them to be supported in core more than anyone else in this thread!

avatar JurgenG
JurgenG - comment - 3 Apr 2015

Okay... so I understand the big risk of embedded malware inside svg-files.
Would it be an option to use ACL as some kind of layer of security? As in: "only a given group of users can upload svg files"


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.
avatar essiele
essiele - comment - 1 Jul 2015

That's a good suggestion, as SVGs seem to be used mostly for design/styling purposes.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar roland-d roland-d - close - 2 Sep 2015
avatar roland-d
roland-d - comment - 2 Sep 2015

@bembelimen The PLT has decided that this PR is not going to be included as-is. The support for SVG's should be complete including uploading and security check on it's contents.

I am going to close this PR and a new one can be made with complete SVG support. Thank you for your contribution.

avatar roland-d roland-d - change - 2 Sep 2015
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2015-09-02 05:20:06
Closed_By roland-d
avatar roland-d roland-d - close - 2 Sep 2015
avatar uglyeoin
uglyeoin - comment - 6 Jan 2016

That's annoying, some support is better than no support. Is there a workaround to use SVGs? Or do I have to hardcode them or create my own module?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar uglyeoin
uglyeoin - comment - 8 Jan 2016

For others reading this, and perhaps for others who develop Joomla! and feel this is a huge security hole, you can use SVGs in Joomla! in two ways.

(a) you can type in the SVG code, which is long winded and messy for clients. It also wouldn't work for an intro image.
(b) you can save your SVG and manually type in the link location in the image window.

Hurruh.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar alandarr
alandarr - comment - 24 May 2016

@uglyeoin I agree with your options to insert the svg, but getimagesize is still an issue. The preview will not show as a result in the blog view of a category. Maybe additional fields to add the size would work when entering the URL to the image?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4674.

avatar uglyeoin
uglyeoin - comment - 24 May 2016

I guess the point is, you can still use SVGs, so although seen as a security hole in Joomla, it's already possible to use them. I'd also suggest that if there is a fear of uploading malicious code, it's the users responsibility. Surely Joomla! do not want to take responsibility for this? If taking ownership, then perhaps installation of components should be regulated. Perhaps automatic ban of any items on the unsafe list.

avatar alandarr
alandarr - comment - 24 May 2016

@uglyeoin I ran across this thread and was hoping someone had found a workaround. Maybe someone will figure something out and post it. If we were given the option to manually add the size, that would help.

avatar mbabker
mbabker - comment - 2 Jul 2016

I'm just going to leave this here regarding why it's such a PITA to properly (and securely) support SVG. https://hackerone.com/reports/148853

avatar uglyeoin
uglyeoin - comment - 5 Jul 2016

Excellent. So are you suggesting we should stop users from being able to use them in Joomla? Because at the moment I can just link to an SVG as I would any external image (even if stored on my site). So Joomla! doesn't prevent me using SVGs, it just makes it annoying.

In any case, it looks like they solved the problem, so perhaps we could ask Abdullah to help us resolve any issues at our end.

avatar mbabker
mbabker - comment - 5 Jul 2016

In that specific platform's case their fix was to force SVG files to be sent with a plain text MIME instead of whatever it would be normally which allows the XSS bug reported via that issue to be executed.

I'm not trying to say Joomla shouldn't support SVG, but implementing support means that the platform should NOT be opening security holes and if those can't be mitigated then it shouldn't be supported. And from a security perspective the SVG format comes with several issues which is why it's basically been on a blacklist in Joomla's code for years.

avatar uglyeoin
uglyeoin - comment - 5 Jul 2016

I understand, it's just that we neither support it nor ban it. So we haven't black listed it very well at all.

avatar mbabker
mbabker - comment - 5 Jul 2016

Without adding additional configuration values to the htaccess.txt and web.config.txt files Joomla doesn't go beyond blocks in the Media Manager to implement a full ban, and IMO by default I don't think Joomla has any reason to be messing with how stuff in your filesystem outside the application gets loaded. And since Joomla doesn't try to serve stuff in the filesystem unless you have a component doing so, no PHP code could do anything about it. It's one of those things where if you know your way around you can make it work but it's not user friendly by any means.

avatar marcodings
marcodings - comment - 5 Jul 2016

we are actively looking into supporting svg to the level that is reasonable and possibly sanitise or at least warn in the new media manager.

Upload support is however very likely to restricted to backend and accesslevels. As the risks are to high to open this to the world and thus open the floodgates. It will come with a lot warnings.

This is prelimenary and work in progress..

avatar uglyeoin
uglyeoin - comment - 8 Jul 2016

Thanks for the update @marcodings , thanks for your insights too @mbabker it helps us little guys to see where bottle necks are and what needs to be changed and potentially opens our eyes to ways we could try to help, even if that is merely spreading this information so others are less frustrated.

Add a Comment

Login with GitHub to post a comment