joomla-cms
joomla-cms copied to clipboard
joomla
Not able to upload svg file
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
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
See here please: #38530
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38532.
@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
~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
Yes, thanks Brian, that's why I didn't close the issue. Just pointed it out for additional information only.
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
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
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.
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.
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.
No not at all. Use svgsanitize as designed. It will clean the files that need cleaning and reject the files that cannot be cleaned.
Telling a user that an svg file from a reputable source/application is harmful when it is not doesn't help anyone at all.
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
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.
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.
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
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.
No we can't but we can at least use the sanitize library as a sanitizer. Its clearly not great as a valaidator
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
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
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...
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?
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
@roland-d please explain your justification for removing the release blocker label.
@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
@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: https://github.com/darylldoyle/svg-sanitizer/issues/64
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?
Found a few more bugs upstream. Please test #38545 as this removes a few more false positives that are preventing upload