SVG- Persistent Cross site Scripting #4949

Closed
rcubetrac opened this Issue Dec 29, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@rcubetrac

Reported by akhilreni on 29 Dec 2015 11:46 UTC as Trac ticket #1490625

Hello,

Svg images generally contain css, but more importantly javascript.
The fact that you can execute JavaScript from inside an image file presents an unexpected vector for XSS attacks.

After uploading the following svg file as attachment to an email will execute javascript.

<script type="text/javascript"> alert(document.cookie); </script>

Steps to reproduce:
-Compose email
-save the above code as xss.svg
-attach the svg file
-Check the email
-open attachment in new tab
-Xss will be triggered.

Migrated-From: http://trac.roundcube.net/ticket/1490625

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Dec 29, 2015

Comment by @alecpl on 29 Dec 2015 12:37 UTC

Hmm... Firefox and Chrome doesn't give me a possibility to open the svg attachment. If I use the mail/get url I can indeed see the alert (in Firefox it says "undefined"). However, I don't see how an attacker could make the use of such URL.

Comment by @alecpl on 29 Dec 2015 12:37 UTC

Hmm... Firefox and Chrome doesn't give me a possibility to open the svg attachment. If I use the mail/get url I can indeed see the alert (in Firefox it says "undefined"). However, I don't see how an attacker could make the use of such URL.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Dec 29, 2015

Milestone changed by @alecpl on 29 Dec 2015 12:37 UTC

later => 1.1.5

Milestone changed by @alecpl on 29 Dec 2015 12:37 UTC

later => 1.1.5

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Dec 29, 2015

Comment by akhilreni on 29 Dec 2015 17:09 UTC

If a user opens the attachment in a new tab, the xss triggers.
I forgot to add a step to steps to reproduce:

  • compose email
  • add svg attachment
  • send email
  • open email, then the attachment in a new tab to trigger the xss.

Works like a charm on chrome Version 47.0.2526.106 m windows 8.

Comment by akhilreni on 29 Dec 2015 17:09 UTC

If a user opens the attachment in a new tab, the xss triggers.
I forgot to add a step to steps to reproduce:

  • compose email
  • add svg attachment
  • send email
  • open email, then the attachment in a new tab to trigger the xss.

Works like a charm on chrome Version 47.0.2526.106 m windows 8.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 2, 2016

Comment by @thomascube on 2 Jan 2016 14:51 UTC

Indeed, opening an SVG file directly in the browser can be desired, although opening the attachment URL in Roundcube should force a download. As the SVG document is then loaded from the webmail domain, we probably need to sanitize its contents. A similar approach as for filtering HTML bodies (XML DOM traversal) might be implemented.

Comment by @thomascube on 2 Jan 2016 14:51 UTC

Indeed, opening an SVG file directly in the browser can be desired, although opening the attachment URL in Roundcube should force a download. As the SVG document is then loaded from the webmail domain, we probably need to sanitize its contents. A similar approach as for filtering HTML bodies (XML DOM traversal) might be implemented.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 4, 2016

Comment by @alecpl on 4 Jan 2016 09:42 UTC

There's already some related code in get.inc:

// do content filtering to avoid XSS through fake images
else if (!empty($_REQUEST[&& $browser->ie && $browser->ver <= 8) {

I'm not sure we could/should integrate SVG sanitization with it.

I suppose we should use empty($plugin['download']('_embed']))) instead of !empty($_REQUEST['_embed']).

For svg we could indeed be more smart and use DOMDocument to remove script tags or convert to jpeg, but not all installations have ImageMagick.

Comment by @alecpl on 4 Jan 2016 09:42 UTC

There's already some related code in get.inc:

// do content filtering to avoid XSS through fake images
else if (!empty($_REQUEST[&& $browser->ie && $browser->ver <= 8) {

I'm not sure we could/should integrate SVG sanitization with it.

I suppose we should use empty($plugin['download']('_embed']))) instead of !empty($_REQUEST['_embed']).

For svg we could indeed be more smart and use DOMDocument to remove script tags or convert to jpeg, but not all installations have ImageMagick.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 6, 2016

Comment by @alecpl on 6 Jan 2016 13:11 UTC

Fixed in 40d7342. Needed review and backport to 1.1 and 1.0.

Comment by @alecpl on 6 Jan 2016 13:11 UTC

Fixed in 40d7342. Needed review and backport to 1.1 and 1.0.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 7, 2016

Comment by @alecpl on 7 Jan 2016 08:44 UTC

So, the commit fixes only <script>, but there can be also other insecure constructs. As in HTML javascript can be in some attributes (like 'onload'). There can be also insecure code in styles, etc.

We could create something similar to rcube_washtml, but... There can be always some insecure construct that we forgot or there can be always a bug in the code. I propose instead to convert svg images to png using ImageMagick. I think that will give as best security. If ImageMagick is not installed we'll print an error to the log and return error code 500.

Comment by @alecpl on 7 Jan 2016 08:44 UTC

So, the commit fixes only <script>, but there can be also other insecure constructs. As in HTML javascript can be in some attributes (like 'onload'). There can be also insecure code in styles, etc.

We could create something similar to rcube_washtml, but... There can be always some insecure construct that we forgot or there can be always a bug in the code. I propose instead to convert svg images to png using ImageMagick. I think that will give as best security. If ImageMagick is not installed we'll print an error to the log and return error code 500.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 7, 2016

Comment by @thomascube on 7 Jan 2016 08:55 UTC

Replying to alec:

We could create something similar to rcube_washtml, but... There can be always some insecure construct that we forgot or there can be always a bug in the code. I propose instead to convert svg images to png using !ImageMagick. I think that will give as best security. If !ImageMagick is not installed we'll print an error to the log and return error code 500.

Rendering SVG into an image IMO is a regression for user experience and not my favourite solution. I think we reached a fairly good state with the HTML and CSS sanitisation and the threats coming from SVG are pretty much the same. We should therefore use the same code or logic to do clean SVG documents.

Comment by @thomascube on 7 Jan 2016 08:55 UTC

Replying to alec:

We could create something similar to rcube_washtml, but... There can be always some insecure construct that we forgot or there can be always a bug in the code. I propose instead to convert svg images to png using !ImageMagick. I think that will give as best security. If !ImageMagick is not installed we'll print an error to the log and return error code 500.

Rendering SVG into an image IMO is a regression for user experience and not my favourite solution. I think we reached a fairly good state with the HTML and CSS sanitisation and the threats coming from SVG are pretty much the same. We should therefore use the same code or logic to do clean SVG documents.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 9, 2016

Comment by @alecpl on 9 Jan 2016 17:34 UTC

This is tricky, but I started working on integrating SVG support into rcube_washtml. See dev-svg branch. a1fdb20

It's not ready yet. It does not remove all XSS threats, it removes too much (some of my test images look different with and without "washing") and it breaks some existing HTML washing code (failing unit-tests), but maybe it's the way to go.

Comment by @alecpl on 9 Jan 2016 17:34 UTC

This is tricky, but I started working on integrating SVG support into rcube_washtml. See dev-svg branch. a1fdb20

It's not ready yet. It does not remove all XSS threats, it removes too much (some of my test images look different with and without "washing") and it breaks some existing HTML washing code (failing unit-tests), but maybe it's the way to go.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 18, 2016

Comment by @alecpl on 18 Jan 2016 09:31 UTC

Implemented and merge with master in 22a018d and backported to release-1.1 and release-1.0 branches.

Comment by @alecpl on 18 Jan 2016 09:31 UTC

Implemented and merge with master in 22a018d and backported to release-1.1 and release-1.0 branches.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Jan 18, 2016

Status changed by @alecpl on 18 Jan 2016 09:31 UTC

new => closed

Status changed by @alecpl on 18 Jan 2016 09:31 UTC

new => closed

@rcubetrac rcubetrac closed this Jan 18, 2016

@rcubetrac rcubetrac added this to the 1.1.5 milestone Mar 20, 2016

ncopa pushed a commit to alpinelinux/aports that referenced this issue Apr 21, 2016

ncopa pushed a commit to alpinelinux/aports that referenced this issue Apr 21, 2016

ncopa pushed a commit to alpinelinux/aports that referenced this issue Apr 21, 2016

@attritionorg attritionorg referenced this issue in distributedweaknessfiling/DWF-CVE-Database Jul 14, 2017

Open

CVE-2017-1000049 is a duplicate based on available info #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment