Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable inline JavaScript when directly open SVG files #30221

Merged
merged 10 commits into from
Aug 19, 2020

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Jul 29, 2020

Summary of Changes

Disable inline JavaScript when directly open SVG files.
This PR adds an SVG file only CSP rule to protect against JavaScript code embedded in SVG files.

This issue was inital reported to the JSST by Lee Thao

Testing Instructions

upload an SVG file with this content to the images folder:

<?xml version="1.0" encoding="UTF-8"?>
<svg width="30" height="30" xmlns="http://www.w3.org/2000/svg">
  <circle cx="15" cy="15" r="15" onclick="alert('svg inline script executed!')"></circle>
</svg>

try to access that image directly in the browser and click on the black circle.
Please notice the message "svg inline script executed!"
Apply the changes in this PR to the htaccess file.
Reload & click the circle again.
There is no message any more.

Actual result BEFORE applying this Pull Request

When accessing SVGs from outside of image tags JavaScript can be executed that could lead to XSS

Expected result AFTER applying this Pull Request

With the proposed changes here we apply a dedicated CSP rule to SVG files that block all inline JS.

Documentation Changes Required

none

cc @HLeithner @SniperSister

Postinstall message

image

htaccess.txt Outdated
@@ -30,6 +30,11 @@ Header always set X-Content-Type-Options "nosniff"
Options +FollowSymlinks
Options -Indexes

## Disable inline JavaScript when directly open SVG files
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Disable inline JavaScript when directly open SVG files
## Disable inline JavaScript when directly open SVG files or embed them with the object-tag

Copy link
Contributor

Choose a reason for hiding this comment

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

opening and embedding please

@brianteeman
Copy link
Contributor

This should be accompanied with a postinstallation message

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 29, 2020

Agree can you give me an example for the text to be used?

It should say something like additional hardening for SVG files and that they can acomplish that hardening by adding the mention lines to the htaccess file.

@brianteeman
Copy link
Contributor

I would use the previous message .htaccess & web.config Security Update as a template for this one

Joomla is now shipped with an additional security rule in the default htaccess.txt and web.config.txt files. This rule will protect users of svg files from potential Cross-Site-Scripting(XSS) vulnerabilities.

The security team recommends to manually ....

@toivo
Copy link
Contributor

toivo commented Jul 29, 2020

I have tested this item ✅ successfully on cd4fcbb

Tested successfully in Beta3.


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

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on cd4fcbb


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

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Aug 2, 2020
@zero-24
Copy link
Contributor Author

zero-24 commented Aug 2, 2020

I have just implemented an postinstall message and the inline comment:

image

Thanks

zero-24 and others added 2 commits August 2, 2020 13:47
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
@zero-24
Copy link
Contributor Author

zero-24 commented Aug 9, 2020

@viocassel @toivo can you take a look into the postinstall message that has been added here? So we can make this as RTC for 3.9.21 :)

@richard67
Copy link
Member

richard67 commented Aug 17, 2020

@zero-24 Can be be 100% sure that on Apache the mod_headers is enabled?

If not (what I assume), then we have to wrap the htaccess change into an <IfModule mod_headers.c>, if it is possible to have nested IfModule and FilesMatch. Otherwise, if it is not possible, we have at least to add some comment and extend the postinstall message.

@richard67
Copy link
Member

@zero-24 Would the following work on all supported Apache versions?

<FilesMatch "\.svg$">
	<IfModule mod_headers.c>
		Header always set Content-Security-Policy "script-src 'none'"
	</IfModule>
</FilesMatch>

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 17, 2020

@zero-24 Would the following work on all supported Apache versions?

Tbh I don't know, i guess it has to be tried. :D

@richard67
Copy link
Member

Tbh I don't know, i guess it has to be tried. :D

@zero-24 If you get me an Apache 2.0 from the German museum in Munich I can try it ;-)

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 19, 2020

Changes have been pushed.

image

@richard67
Copy link
Member

richard67 commented Aug 19, 2020

@zero-24 Now it will not crash if mod_headers is not there => Fine.

But it also will not apply the rule in this case, and so you can upload dangerous svg.

Is there anything we can do about this?

I mean we have that problem with all csp headers we set in htaccess.

Maybe we really should require mod_headers?

@richard67
Copy link
Member

And the next question of course is: What do we do with IIS?

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 19, 2020

And the next question of course is: What do we do with IIS?

As mention in the postinstall i'm not aware of how to do that with web.config

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 19, 2020

@zero-24 Now it will not crash if mod_headers is not there => Fine.

But it also will not apply the rule in this case, and so you can upload dangerous svg.

Is there anything we can do about this?

I mean we have that problem with all csp headers we set in htaccess.

Maybe we really should require mod_headers?

We just apply server side protection here.

When it is not there but taken care otherwise on the server side this is fine.

@richard67
Copy link
Member

I have tested this item ✅ successfully on aba35f1


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

@HLeithner HLeithner merged commit 66ddc48 into joomla:staging Aug 19, 2020
@HLeithner HLeithner added this to the Joomla! 3.9.21 milestone Aug 19, 2020
@HLeithner
Copy link
Member

Thanks

@zero-24 zero-24 deleted the svg_csp branch August 19, 2020 19:09
Kostelano added a commit to JPathRu/localisation that referenced this pull request Aug 21, 2020
Новые константы, вышла 3.9.21 RC
joomla/joomla-cms#30221
joomla/joomla-cms#30390 (не вносил изменения, у нас "Менеджер" есть и в других местах)
joomla/joomla-cms#30157
joomla/joomla-cms#30110
joomla/joomla-cms#29895
joomla/joomla-cms#30253
Reconix pushed a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
* Disable inline JavaScript when directly open SVG files
* add postinstall message message
* add if module check

Co-authored-by: Brian Teeman <brian@teeman.net>
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
Reconix added a commit to Reconix/joomla-cms that referenced this pull request Aug 31, 2020
@brianteeman brianteeman mentioned this pull request Oct 12, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants