No Code Attached Yet Release Blocker
avatar rachelwalraven
rachelwalraven
19 Aug 2022

After the update to 4.2 it's no longer possible to upload a SVG file.

Steps to reproduce the issue

Set media options to allow svg files, svg image files and image/svg+xml mimetype
screen shot 2022-08-19 at 10 12 39
Go to media manager
Upload a svg file

Expected result

svg file is uploaded and shows in media manager

Actual result

Error is shown "Unable to upload file"

System information (as much as possible)

Additional comments

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
3.00

avatar rachelwalraven rachelwalraven - open - 19 Aug 2022
avatar rachelwalraven rachelwalraven - change - 19 Aug 2022
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 19 Aug 2022
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 19 Aug 2022
avatar ChristineWk
ChristineWk - comment - 19 Aug 2022

See here please: #38530


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

avatar brianteeman
brianteeman - comment - 19 Aug 2022

@ChristineWk that pull request is just to improve the message. It does NOT resolve the fact that something has changed and all svg uploads are now prevented

avatar brianteeman
brianteeman - comment - 19 Aug 2022

I have found the problem causing this and am working on a pull request

Can someone mark this a release blocker please @roland-d @fancyFranci @richard67

avatar ChristineWk
ChristineWk - comment - 19 Aug 2022

Yes, thanks Brian, that's why I didn't close the issue. Just pointed it out for additional information only.

avatar richard67 richard67 - change - 19 Aug 2022
Labels Added: Release Blocker
avatar richard67 richard67 - labeled - 19 Aug 2022
avatar brianteeman
brianteeman - comment - 19 Aug 2022

ok this doesnt make much sense to me. If I understand the code from @bembelimen correctly in the MediaHelper then we are using the enshrined/sanitizer library to try and make the svg is safe and if the library fails to make it safe then we reject it

bad1, bad3, bad4 - these should be blocked from uploading and they currently are
good, good2, good3, good4, good5 - these should be uploaded and they currently are
example1, example2, example3, example4, example5, example6 - these should be uploaded and they are not

cleaned-example6.svg
As a further test I took example1 and example6 and passed them through the online demo of the sanitizer at https://svg.enshrined.co.uk/ and then tried to upload the sanitized result. They still failed.

I hope this information and the zip of images helps someone better than me to resolve this
svg.zip

avatar brianteeman
brianteeman - comment - 19 Aug 2022

The more I look at this the more confused I am. svgsanitize is designed to clean an upload svg but we appear to be using it as a validator only

avatar obuisard
obuisard - comment - 19 Aug 2022

Yes, the sanitizer is used to check the files, it is not used to clean them, even though it has that capability. The problem with cleaning files and use the cleaned version is that it may not have the intended results. For instance, some xlinks can be removed. Or scripts. Removed, it could break the SVG output.

avatar obuisard
obuisard - comment - 19 Aug 2022

The media manager can be used in the frontend so allowing SVG files that may have concerned issues should not be uploaded. However, if an administrator of the site knowingly puts a SVG on his/her site through FTP, for instance, nothing prevents him/her to. The admin is responsible for making sure the file is safe.

avatar obuisard
obuisard - comment - 19 Aug 2022

Now we need to educate the user so he/she knows why the file is harmful. Hence better wording on the messages we send and proper documentation.

avatar brianteeman
brianteeman - comment - 19 Aug 2022

No not at all. Use svgsanitize as designed. It will clean the files that need cleaning and reject the files that cannot be cleaned.

avatar brianteeman
brianteeman - comment - 19 Aug 2022

Telling a user that an svg file from a reputable source/application is harmful when it is not doesn't help anyone at all.

avatar brianteeman
brianteeman - comment - 19 Aug 2022

further tests with the svg files I supplied above and using the svg-scanner that is part of the package suggest that the scanner is crap. try it yourself and see how it will fail almost any svg that is not minified

avatar N6REJ
N6REJ - comment - 19 Aug 2022

Telling a user that an svg file from a reputable source/application is harmful when it is not doesn't help anyone at all.

100% spot on. Here are 2 images created via inkscape which is the FOSS eqv to Adobe illustrator and both will fail being uploaded.
Bearsampp-header-logo
Bearsampp-logo-128x128
I could supply several more.
The point being is I EXPECT these files to work. They were created with a well known program designed for this kind of work.

avatar brianteeman
brianteeman - comment - 20 Aug 2022

@N6REJ if you minify them then they will probably work

avatar dgrammatiko
dgrammatiko - comment - 20 Aug 2022

SVG security is HIGHLY dependant on the way the SVG is used. As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine. Read the specs: https://svgwg.org/svg2-draft/conform.html#examples

avatar SniperSister
SniperSister - comment - 20 Aug 2022

As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine

The thing is: can we safely assume that an uploaded SVG is always used in the img-src context? I don't think so.

avatar brianteeman
brianteeman - comment - 20 Aug 2022

No we can't but we can at least use the sanitize library as a sanitizer. Its clearly not great as a valaidator

avatar dgrammatiko
dgrammatiko - comment - 20 Aug 2022

we safely assume that an uploaded SVG is always used in the img-src context

No, the CMS doesn't have this flexibility and neither I posted the reference to imply this. I just wanted to point out that if SVGs are used as images (with src attribute pointing to the url of the image) there's no need for sanitisation. It's unrealistic to assume that this would be the only way people will use them as they can have a field and then embed the svg in the html, or in the css or...

No we can't but we can at least use the sanitize library as a sanitizer

Maybe it's easier to do the sanitisation on the client before even it reaches the server. The code from the SVGOM could easily imported and executed when the mime type === svg/... https://github.com/jakearchibald/svgomg/tree/main/src/js

avatar brianteeman
brianteeman - comment - 20 Aug 2022

Maybe it's easier to do the sanitisation on the client before even it reaches the server.

That's still telling someone that the svg from their reputable source is vulnerable just because we are using a library incorrectly

avatar dgrammatiko
dgrammatiko - comment - 20 Aug 2022

My point was that IF you can guarantee that svgs will only be used as image tags then there's no need for any sanitisation, proof is that the images from the svg.zip that @brianteeman posted above were uploaded without any changes on GitHub because the only way GH will ever use them is as img tags. In short this might give you another perspective...

bad1
bad3
bad4
cleaned-example1
cleaned-example6
example1
example2
example3
example4
example-6
example5
good2
good
good3
good4
good5

avatar roland-d roland-d - change - 21 Aug 2022
Labels Removed: Release Blocker
avatar roland-d roland-d - unlabeled - 21 Aug 2022
avatar obuisard
obuisard - comment - 21 Aug 2022

SVG security is HIGHLY dependant on the way the SVG is used. As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine. Read the specs: https://svgwg.org/svg2-draft/conform.html#examples

My understanding is that the img tag disables scripting (SVG files are 'run' in a browser sandbox), but the files are not sanitized. Therefore, anyone downloading the file from your site gets the original file.
Is that right @dgrammatiko?

avatar brianteeman
brianteeman - comment - 21 Aug 2022

The security issue occurs if a malicious svg could be uploaded (which is a low privilege task) it could then be accessed directly www.example.com/images/filename.svg . Image files are not prevented from being loaded directly.

Or an example from the post above https://user-images.githubusercontent.com/3889375/185746788-7c1c39c4-2680-448b-931b-f6011fe320b8.svg

avatar brianteeman
brianteeman - comment - 21 Aug 2022

@roland-d please explain your justification for removing the release blocker label.

avatar N6REJ
N6REJ - comment - 21 Aug 2022

@brianteeman yes that did work.

avatar brianteeman
brianteeman - comment - 21 Aug 2022

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

avatar N6REJ
N6REJ - comment - 21 Aug 2022

looks like if we'd just leave it alone and let it clean it all would be well

avatar brianteeman
brianteeman - comment - 21 Aug 2022

yes. but thats not we're doing

avatar bembelimen
bembelimen - comment - 21 Aug 2022

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

Not sure what you mean with "spaces" but spaces are not marked as invalid. An attribute (xml:space) is marked as invalid due to a bug: darylldoyle/svg-sanitizer#64

avatar brianteeman
brianteeman - comment - 21 Aug 2022
 
PS C:\laragon\www\j4\libraries\vendor\enshrined\svg-sanitize\src> php .\svg-scanner.php .\svg\example2.svg
{
    "totals": {
        "errors": 3
    },
    "files": {
        ".\\svg\\example2.svg": {
            "errors": 3,
            "messages": [
                {
                    "message": "Suspicious attribute 'space'",
                    "line": 3
                },
                {
                    "message": "Suspicious attribute 'space'",
                    "line": 8
                },
                {
                    "message": "Suspicious node 'svg'",
                    "line": -1
                }
            ]
        }
    }
}

ah I see what you mean now about xml:space and that it was just coincidence on the files it was reported. Perhaps until the library is updated you could whitelist it in the same way as you did the comments?

avatar brianteeman
brianteeman - comment - 21 Aug 2022

Found a few more bugs upstream. Please test #38545 as this removes a few more false positives that are preventing upload

avatar N6REJ
N6REJ - comment - 21 Aug 2022

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

Not sure what you mean with "spaces" but spaces are not marked as invalid. An attribute (xml:space) is marked as invalid due to a bug: darylldoyle/svg-sanitizer#64

@brianteeman said that not me.

avatar dgrammatiko
dgrammatiko - comment - 22 Aug 2022

My understanding is that the img tag disables scripting (SVG files are 'run' in a browser sandbox), but the files are not sanitized. Therefore, anyone downloading the file from your site gets the original file. Is that right @dgrammatiko?

@obuisard so, the SVG is never a threat or in anyway problematic for the server side, it's just a static file! For the client side ONLY if the SVG is embedded into the page there are potential problems. The images folder (or any folder that the media manager is managing) is supposed to be only for static assets (jpg, png, webp, avif, mp4, mov, pdf, etc) and the only way that these files are used for the CMS is ALWAYS as image, video, audio or object. There is no way that an SVG will ever be embedded in the page UNLESS a user or a developer does specifically that. In short the whole fear for SVGs is not justified. On top of that if SVGs are problematic because they might introduce XSS vulnerabilities then there are already documented such vulnerabilities in other parts of the software that got a status won't fix or not an issue. It seems to me that there are 2 rules...

avatar roland-d
roland-d - comment - 22 Aug 2022

@roland-d please explain your justification for removing the release blocker label.

There are several reasons I did it at the time:

  • This issue is not blocking the 4.2.1 release
  • There was no PR with a fix at the time
  • SVGs can still be uploaded although I agree not all of them

Now following the discussion here, can't we make it an option that users can decide for themselves to allow SVG uploads? By default we do what we do now but if users want to open it up they can. They can upload SVGs Joomla considers bad anyway via FTP/Filemanager/SSH.

avatar brianteeman
brianteeman - comment - 22 Aug 2022

not having a pr is not a reason to remove a release blocker 🤦

as release lead I respectfully suggest that you do follow the discussion or there is no way yu can actually make a good faith decision.

avatar roland-d roland-d - change - 22 Aug 2022
Labels Added: Release Blocker
avatar roland-d roland-d - labeled - 22 Aug 2022
avatar roland-d
roland-d - comment - 22 Aug 2022

not having a pr is not a reason to remove a release blocker 🤦

Fair enough, I will put the release blocker back

as release lead I respectfully suggest that you do follow the discussion or there is no way yu can actually make a good faith decision.

I am following the discussion, both here and on Glip

avatar crystalenka
crystalenka - comment - 15 Sep 2022

I actually ran into this issue yesterday trying to test the PR for media manager thumbnails.

At a bare minimum, the error text should be expanded. I thought Joomla was broken. Explaining that the SVG could be potentially harmful, and maybe linking to a place to minify it, would be extremely helpful.

Even better would be to offer the user the option to sanitize the SVG with a note it may behave in an unexpected way. That way it's an opt-in action, and if it breaks the user knows why.

avatar brianteeman
brianteeman - comment - 15 Sep 2022

As #38545 has now been merged there should be a big drop in failed svg

avatar N6REJ
N6REJ - comment - 16 Sep 2022

Even better would be to offer the user the option to sanitize the SVG with a note it may behave in an unexpected way. That way it's an opt-in action, and if it breaks the user knows why.

100% agree with that

avatar HDInfautre
HDInfautre - comment - 16 Nov 2022

Hello

I would point out that I can send svg files via FTP, then use them in the media manager (joomla 4.2.5).
Most of the time, it is impossible to download them from the media manager. The error message does not give much information.
Is this fixed for you?
Is this something that can be improved in a future version of Joomla?

Regards


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

Add a Comment

Login with GitHub to post a comment