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

Gitea can't render SVG files. #1095

Closed
lcges opened this issue Mar 1, 2017 · 61 comments · Fixed by #14101
Closed

Gitea can't render SVG files. #1095

lcges opened this issue Mar 1, 2017 · 61 comments · Fixed by #14101
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Comments

@lcges
Copy link

lcges commented Mar 1, 2017

Hello!

Gitea can't render SVG files.
It is a matter of settings?
or
Do not implement functionality?

GITEA:
screenshot_2017-03-01_09-58-01

GITHUB:
screenshot_2017-03-01_10-01-13

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 1, 2017
@lunny lunny added this to the 1.x.x milestone Mar 1, 2017
@pgaskin
Copy link
Contributor

pgaskin commented Mar 1, 2017

The problem is serving with the wrong mime type.

@arkraft
Copy link

arkraft commented Mar 2, 2017

Right now the mime type is read by calling http.DetectContentType. This does not support the "image/svg+xml" mime type. There was a feature request for this (golang/go#15888), but it seems they won't implement it until it is also implemented in the mimesniff spec (whatwg/mimesniff#7). mime.TypeByExtension supports "image/svg+xml" as it has ".svg" in its built-in table. https://github.com/golang/go/blob/release-branch.go1.6/src/mime/type.go#L35

@lunny
Copy link
Member

lunny commented Mar 2, 2017

fileType := mime.TypeByExtension(path.Ext(filePath))
if fileType == "" {
    fileType = http.DetectContentType(buffer)
}

It seems change the code to this will fix the bug.

@pgaskin
Copy link
Contributor

pgaskin commented Mar 2, 2017

My render raw files pull request does exactly this, but tboerger seems to think it is a very big security risk.

@lcges
Copy link
Author

lcges commented Mar 13, 2017

Hi, this is text about SVG security. Don't worry, GitHub could do it, so can we!

First make sure that we set a good headers safety. The basic HTTP response headers with the SVG file will be:

  • Content-Type: image / svg + xml,
  • X-Content-Type-Options: nosniff.

In addition, the magazine SVG files could be transferred to a separate domain or subdomain that does not require authentication even through mechanisms such as cookies (files can be referenced eg. The GUID, you should also add .svg extension to file names). In such a situation, a successful XSS attack will be much less of a threat.

Then, set up a very strict policy Content Security Policy (CSP) - whether it is for a script that returns the content of vector graphics, or the server that serves SVG files from the directory. Setting the CSP value such as Content-Security-Policy: script-src 'none' turns off the ability to execute scripts for the document (ie malicious graphics, SVG).

SVG files can also be processed on the server and convert to safer raster image formats (eg. PNG). You can also attempt to remove potentially dangerous scripts - for this purpose can be used, eg. Utility SVG Purifier.

As you can see ideas to deal with SVG format is quite a lot, but by far the best option is to use SVG files only by developers and the strict prohibition upload such documents.

Say NO SVG files from an untrusted source.

@lcges
Copy link
Author

lcges commented Apr 13, 2017

Is there any plans to add support for SVG in the near future?

@lunny
Copy link
Member

lunny commented Apr 13, 2017

No people are working on this.

@edgar-bonet
Copy link

I have reported the issue to the Gogs bug tracker, before even knowing about Gitea.

@edgar-bonet
Copy link

I tried to investigate the issue. I have no solution, but some info that I hope will be helpful:

SVG embedded in markdown

SVG images do not display in rendered markdown. The following

![alt text](image.svg)

is rendered as

<a href="/USER/REPO/raw/master/image.svg" rel="nofollow"><img
src="/USER/REPO/raw/master/image.svg" alt="alt text"></a>

by both Gogs and Gitea. GitHub's rendering is only slightly different: it links to the blob instead of the raw page, it has target="_blank" instead of rel="nofollow", and it adds style="max-width:100%;" to the img tag. The image, however, is not rendered by the browser (Firefox and Chromium at least) because it is served with the HTTP header

Content-Type: text/plain; charset=utf-8

This is consistent between the three services tested. See my test repo:

SVGs served by GitHub

GitHub has a feature lacking in both Gogs and Gitea: the blob page has buttons for switching between “source blob” and “rendered blob” views, defaulting to the latter. This can be useful since, as stated above, the raw page does not render the image in-browser.

The implementation, however, seems quite complex. The “rendered blob” view uses an iframe pointing to render.githubusercontent.com. This iframe uses JavaScript to generate an img element with a data URL containing the image. Any JavaScript embedded in the SVG is not stripped, but the browser does not interpret it anyway.

When serving SVG from raw URLs, GitHub issues a 302 redirect to raw.githubusercontent.com. Following the redirect, the SVG is served with what looks like a lot of security-related headers:

Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'
Strict-Transport-Security: max-age=31536000
X-Content-Type-Options: nosniff
X-Frame-Options: deny
X-XSS-Protection: 1; mode=block
Content-Type: text/plain; charset=utf-8

Embedding SVGs containing JavaScript

Testing with my local Web server, I found that neither Firefox nor Chromium would interpret JavaScript embedded in an SVG if that SVG is loaded from an HTML's img element. This could be what makes GitHub's “rendered blob” safe. The JavaScript is however interpreted if the HTML embeds the SVG using an iframe, an embed or an object element.

@bkcsoft
Copy link
Member

bkcsoft commented Jun 19, 2017

Gitea (and Gogs) does not handle multiple domains, which would be a requirement for this. This has been discussed some over at gogs/gogs#1314 . And there's already ?render=1 gogs/gogs#2593 that could be used for this if you really want to. As for adding an option to always do this, the answer is currently 'no' since it's a security risk #683

@edgar-bonet
Copy link

Don't the headers

Content-Security-Policy: default-src 'none'
X-XSS-Protection: 1; mode=block

provide a reasonable level of protection against these security risks?

@jonasob
Copy link

jonasob commented Oct 26, 2017

It seems ?render=1 doesn't do much, and I even wonder if what I'm trying to do would be solved by this! Here's what I'm trying to do. I have an svg file, which is served by Gitea under

https://git.fsfe.org/reuse/reuse-ci/raw/master/reuse-compliant.svg

I would like to include this in a README.md as such:

[![reuse compliant](https://git.fsfe.org/reuse/reuse-ci/raw/master/reuse-compliant.svg)](https://git.fsfe.org/fsfe/reuse-web) 

Adding ?render=1 doesn't seem to do much of a difference here. I could, and probably will need to, host the svg on a standard website, but it seems silly to spin up a webserver when it's already (in theory) served through Gitea :)

@lunny
Copy link
Member

lunny commented Oct 26, 2017

maybe a client render via javascript.

@bkcsoft
Copy link
Member

bkcsoft commented Oct 29, 2017

maybe a client render via javascript.

@lunny That is only slightly worse than rendering it directly 🙁

@bluecatchbird
Copy link

Is somebody still working on this?

@lafriks
Copy link
Member

lafriks commented Jun 22, 2018

@bluecatchbird I don't think so

@fduxiao
Copy link

fduxiao commented Jul 30, 2018

You guys can read this, which uses a ?sanitize=true to get rid of javascript.

@kiemrong08
Copy link

Markdown in gitea, i cannot display image. Gitea Version: 280ebcb

@lunny
Copy link
Member

lunny commented Aug 13, 2018

@kiemrong08 please file another issue if you cannot find similar issue.

@ghost
Copy link

ghost commented Aug 13, 2018

I wasn't aware of of exploits though some are listed here: https://security.stackexchange.com/questions/11384/exploits-or-other-security-risks-with-svg-upload. Why not just parse out any inline JS—which IMO shouldn't be allowed in SVG anyway—and serve from the same domain inside a sandboxed iFrame?

@stale
Copy link

stale bot commented Jan 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 14, 2019
@pintman
Copy link

pintman commented Jan 14, 2019

Issue should be closed if the problem is solved, not by time of inactivity. :-/

@stale stale bot removed the issue/stale label Jan 14, 2019
@lesderid
Copy link

Yeah, this new policy is frankly pretty darn stupid.

@lunny lunny removed this from the 1.x.x milestone Jan 16, 2019
@lunny lunny added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jan 16, 2019
@sapk
Copy link
Member

sapk commented Aug 28, 2019

We already have a dom parser/sanitizer (https://github.com/microcosm-cc/bluemonday) that could maybe configure to do this.

@davidsvantesson
Copy link
Contributor

davidsvantesson commented Aug 28, 2019

An other simple way would be to render them (server side) in an other format like png.

Wouldn't that already be possible with the external render functionality? SVG can contain for example links that would be lost if rendering them to png.

Although SVG is what most peope want, are there other files that should be possible to view in raw format? Should be better to find a general solution in that case.

@guillep2k
Copy link
Member

guillep2k commented Aug 28, 2019

Rendering SVG to PNG will have several drawbacks:

  • It's not lightweight (i.e. in golang code and file resources like fonts, etc.).
  • It's easily outdated.
  • As @davidsvantesson said, links and other features like dynamic styles will be lost. As for the latter, I think it's OK for a preview.

@jcw
Copy link

jcw commented Feb 9, 2020

I came here because the lack of support for ![](img/mem-app.svg) in Markdown very much surprised me. Here is my example. Given that both the embedding document and the SVG content are part of the same repository, I don't see how a JavaScript security concern affects any of this.

Would it be an option to support this kind of SVG embedding if the SVG comes from the same repo?

@lafriks
Copy link
Member

lafriks commented Feb 9, 2020

@jcw problem is that svg can contain JavaScript and someone could create repository with malicious svg file that could be used to attack other users on that server

@jcw
Copy link

jcw commented Feb 9, 2020

Thanks for clarifying, @lafriks. I'm not a front-end developer, so what follows may be wrong:

  1. so someone with commit access can cause other users, viewing this nasty SVG to run unexpected JS in their browser, with their privileges, to do bad things, right?
  2. does this comment above indicate the issue is solved in recent browsers?
  3. if I understand it correctly, this is a matter of trusting committers - and in my case, with my own gitea server and limited access to only a few trusted friends, there is no risk

I understand and fully respect the fact that this issue is non-trivial. Nevertheless, it might be worth considering moving towards a limited resolution (or perhaps passing the risk on to gitea admins, by having a site config setting as to whether embedded SVG is allowed).

I'm just trying to help find a gentle way out. SVG's scale independence makes it very useful for visual diagrams, given the wide range of screen sizes used to browse on a gitea site.

@lunny lunny mentioned this issue Mar 30, 2020
4 tasks
@alexanderadam
Copy link

alexanderadam commented Apr 7, 2020

The variant edgar-bonet mentioned sounds very good to me.

JS from inside the image wouldn't have any access to the parent if SVG images would be replaced with an iframe which has the sandbox attribute (at least for, more or less, any modern Browser).

And for the paranoid ones, this feature could be optional, maybe?

Also removing dangerous things were mentioned a few times. I guess the Go library bluemonday is made for exactly this. Furthermore it is already a dependency anyway (it should be updated though because Gitea uses a version from 2016!).

But attackers are sometimes also finding some sanitizer issues (i.e. if projects are using outdated versions 😉 ). So I guess it would make sense to make the sandboxed iframe-solution mandatory to have at least a level of security if the browser works correctly. And I guess it would be a nice bonus if a lib like bluemonday is used additionaly.

@edgar-bonet
Copy link

A while ago I created a repo just for testing the handling of SVG images in a README file:

Visiting this repo again, I notice that now GitHub does render the inline SVGs. The GitHub issue “Fix relative SVG rendering” has accordingly been closed.

The GitHub solution seems to rely on multiple layers of defence:

  • the images are served from a different domain
  • the <script> elements have been removed
  • there are quite a few security-related HTTP headers.

@mpfaff
Copy link

mpfaff commented Apr 18, 2020

Can allowing unsecure svgs just be made a config option? On a semi-private Gitea instance, it really should matter that svgs aren't "technically" secure, because only people you trust have accounts. In my case, I'd like to be able to embed svgs in README.md files, but Gitea is forcing the mime-type to be set to text/plain.

@jcw
Copy link

jcw commented Apr 18, 2020

Exactly - that's what I suggested 2 months ago. For precisely the same reason.

@sapk
Copy link
Member

sapk commented Apr 19, 2020

From testing in #8024 we could use bluemonday to clean-up svg but to be secure (at least in it current state) it need to block some svg functionality like url() linking to some gradient. I suggest we could add a config (SVG_RENDER_METHOD) that would be set to "none" (or reject) by default and can be set to "filtered" (limited set of func) or "insecure" (the raw source).

@alexanderadam
Copy link

alexanderadam commented Apr 19, 2020

On a semi-private Gitea instance, it really should matter that svgs aren't "technically" secure

It's not only about trusting the people on your server (or yourself) but also about not using the mirroring feature of gitea. Because you could obviously also mirror a repository that is safe today, but includes a malicious SVG that hijacks your Gitea credentials tomorrow.

Just to be sure: is there any reason against rendering the SVG in a sandboxed iFrame?
Because if I understand this correctly, this is handled like a different domain if the sandbox attribute is used (it will intentionally always fail same origin policy). Furthermore forms, popups and similar things are blocked as well.
So it probably needs "only" a nearly-static page with an iframe that (with transparent background & defined dimensions and alignment, though).

And to clarify this: I'm not against stripping dangerous tags, as mentioned earlier. I just believe that browsers should be pretty secure if used correctly and I guess some smart people implemented the sandbox feature.

@mpfaff
Copy link

mpfaff commented Apr 19, 2020

To add to what I'd said earlier, I'd like to see Gitea add a "semi-dangerous" option to allow some svg features, and another "more-dangerous" option with more warnings that allows object tags to enable embedding svgs with fonts, something that can't currently be done with img tags.

@guillep2k
Copy link
Member

I'd prefer @alexanderadam 's idea of sandboxing the SVGs, via a config on/off setting. Luckily the suggested solutions are not mutually exclusive.

@lhinderberger
Copy link

lhinderberger commented Sep 7, 2020

This issue is also relevant to Codeberg.org, where there also have been discussion about this: https://codeberg.org/Codeberg/Community/issues/220

One possible solution would be to implement a media proxy as suggested in #916
EDIT: IF that proxy is on another origin, e.g. on a different subdomain.

@jtran
Copy link
Contributor

jtran commented Nov 25, 2020

I see that #8024 is closed now due to inactivity. What's the status of that or some other filtering approach? Is anyone still working on that angle?

@Codeberg-org
Copy link
Contributor

I'd prefer @alexanderadam 's idea of sandboxing the SVGs, via a config on/off setting. Luckily the suggested solutions are not mutually exclusive.

tbh we would like to see sandboxing for all rendering of user-provided content and previews.

@alexanderadam
Copy link

alexanderadam commented Nov 27, 2020

tbh we would like to see sandboxing for all rendering of user-provided content and previews.

I didn't think of it before but of course you're absolutely right. Putting rendered markdown and other content completely within a sandboxed frame also avoids the problem of mapping sizes of the SVG to its containing frame.

PS: Also paths probably have to be still rewritten (for having the SVG within an iFrame) and/or dangerous attributes still removed because users could still be tricked by adding an infographic that you want to see in full screen.
i.e.

Click here to see the full size graphic:
[![Click here to see the full size graphic](/malicious/infographic.svg)](/malicious/infographic.svg)

It doesn't appear evil, since it's still the same domain and many users probably won't know its implications accessing the SVG directly.

@kdumontnu
Copy link
Contributor

tbh we would like to see sandboxing for all rendering of user-provided content and previews.

Can't we just embed in an img tag to disable scripts?

If an SVG file is fetched as image, then certain requirements apply to this document:
- The SVG document is not allowed to fetch any resources. This also applies to scripts, stylesheets or images.
- Fonts shouldn't be loaded as well. The situation in UAs seems to still be unclear though.
- Scripts must not be executed.
- The style attribute and the style element can style the document. Restricting these is unpractical. A lot of content already relies on CSS styling and would be broken by changing any default setting. Even though SVG has presentation attributes, these are still implemented as a stylesheet with slightly higher hierarchy than UA styles. UA styles must be applied in certain cases as well.
- SVG Animations and CSS animations are still allowed. This also allows conditions where animation wait for other animations to be finished.
- Hit testing must be disabled at all times.
- Event listeners must be disabled at all times.

@jtran
Copy link
Contributor

jtran commented Nov 30, 2020

As far as I can tell from @edgar-bonet's test repo, GitHub doesn't strip <script> tags and doesn't serve from a different domain. The <img> tag uses a relative URL, so it's the same domain. I believe what's happening is that the <img> tag is indeed disabling scripts. When you navigate to the URL of the SVG in your browser, the <script> tag is still there, but the CSP forbids the script execution. Firefox gives me a message about it in the console.

Edit: it does serve from a different domain using a 302 redirect.

@jtran
Copy link
Contributor

jtran commented Dec 1, 2020

I propose the following changes to close this issue:

  • Always use an <img> tag to load the SVG file, whether this is navigating to the SVG file in the tree or embedding in markdown. Since browsers disallow scripts or other external resources when using an <img> tag, SVG files with scripts are handled. If people don't want to give up the text rendering when navigating to the file in the tree, there can be a new button that toggles between the SVG with the <img> tag and the text, similar to GitHub's blob page.
  • Serve raw .svg files with the header Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'; sandbox. This is so that when anyone navigates in their browser directly to the raw SVG file, scripts are disallowed.
  • Serve raw .svg files with the header Content-Type: image/svg+xml. This is so that browsers properly display the SVG image when embedded in markdown.

What's wrong with this proposed solution? It handles SVG files with embedded script tags, without a separate domain, and makes no assumptions about the trustworthiness of the files. It handles displaying the SVG when navigating in the source tree, going directly to the raw file, and when embedding the raw image file in markdown. It does it without a whitelist of things to filter and without adding any new configuration options. So what am I missing?

@edgar-bonet
Copy link

@jtran wrote:

Always use an <img> tag to load the SVG file

The user can bypass this by right-clicking on the image and selecting “View Image” (Firefox) or “Open image in new tab” (Chromium). From what I see on GitHub, it seems that the Content-Security-Policy header provides adequate protection in this case.

@jtran
Copy link
Contributor

jtran commented Dec 22, 2020

Check out PR #14101 where I implemented the above.

@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging a pull request may close this issue.