1325923 - (CVE-2017-7837) SVG image can be used to set cookie

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

1. Go to https://jsfiddle.net/26nLj6mL/1/
2. Now go to https://shhnjk.com/


Actual results:

https://shhnjk.com/ is not accessible and says "Bad request" due to Cookie bomb. Same attack can be done from https://vuln.shhnjk.com/svg_data.html.


Expected results:

Cookie should not be set by SVG image. Fix of https://insert-script.blogspot.ae/2016/12/firefox-svg-cross-domain-cookie.html only prevented setting cross domain cookie. But if site hosted svg image who has content disposition: attachment specified would be affected too. Same for the site who allows data URI in img tag.

Comment 1

[:Gijs (he/him)]

Dan or Dragana, do you know why we allow setting cookies from <meta> tags from inside SVG images, esp. when they are used inside <img> tags? That seems surprising to me...

The jsfiddle loads: https://vuln.shhnjk.com/test.svg

which does:

<meta http-equiv='Set-Cookie' content='a=aa<snip>; expires=Thu, 31 Dec 2020 12:00:00 UTC; path=/; Domain=.shhnjk.com' />

(repeated for 'b', 'c', etc., with very long values).

Comment 2

[Dragana Damjanovic [:dragana]]

This looks like a principal issue. Also needinfo Christoph, maybe we know a fast answer.
(I have not try it yet).

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to :Gijs (gone until 3 jan) from comment #1)
> Dan or Dragana, do you know why we allow setting cookies from <meta> tags
> from inside SVG images, esp. when they are used inside <img> tags? That
> seems surprising to me...

In bug 1317641 comment 4, bz seemed to say that the spec technically requires this behavior. Though as I suggested in bug 1317641 comment 2, we could add a "no images" check in nsContentSink::ProcessHeaderData to prevent this.

I think that might be a good idea, from the perspective of images being atomic black-boxes & having no external side-effects / no scripting / etc. bz, what do you think?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think we should talk to the spec editor about how to get the spec fixed.  For example, the way the spec is written right now, a set-cookie <meta> in an XHR response document should also set cookies, which seems obviously undesirable.  And yet I expect Gecko does just that right now...

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dragana Damjanovic [:dragana] from comment #2)
> This looks like a principal issue. Also needinfo Christoph, maybe we know a
> fast answer.

Unfortunately I don't know a fast answer :-( Probably the right path forward is to do what bz suggests in comment 4.

Comment 6

[Jun (Request needinfo for comment, I ignore other updates)]

Hi,

Any update on severity or status of this issue? Will it take quite some time to fix it?

Comment 7

[Daniel Holbert [:dholbert]]

A direct fix would be trivial -- just a one-line change.  We could just add a "mDocument->IsBeingUsedAsImage()" check to this nsGkAtoms::setcookie comparison, in nsContentSink::ProcessHeaderData():
https://dxr.mozilla.org/mozilla-central/rev/b3774461acc6bee2216c5f57e167f9e5795fb09d/dom/base/nsContentSink.cpp#307

(An automated testcase would be a little more difficult to write, but not too bad.)

But per comment 4, we should first probably figure out more generally which sorts resources should & shouldn't be allowed to use the <meta> tag to set cookies, and get that settled in the spec... Jason, is there anyone who's worked on networking/cookie code who we could task with that?

Comment 8

[Jason Duell]

cc-ing a bunch of folks who've been working on cookies stuff.

I'll take a stab and guess that Ehsan is a good person to figure out the right behavior here.

Comment 9

[(no longer active)]

Hmm, my reading of the spec is a bit different than Boris'.

<https://html.spec.whatwg.org/multipage/semantics.html#attr-meta-http-equiv-set-cookie> step 2 takes us to <https://tools.ietf.org/html/rfc6265#section-5.3>, where per step 1 we can ignore the cookie.

So technically we could change our behavior here freely, but I agree that it seems more useful to spec something.

Chromium currently only honors the mata element for HTML documents with a TODO comment about enabling it for XML documents in general: <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/HttpEquiv.cpp?sq=package:chromium&rcl=1484828258&l=116>.  I think honoring this only for HTML documents makes sense.  What do you think, Boris?

We also have two extra bugs in the meta processing:

1. We ignore whether the document has access to cookies at all, which is the reason that Gecko currently will honor such meta elements for XHR parsed documents.  (As far as I can tell, this behavior isn't even spec'ed.)
2. We ignore whether the document is in a sandboxed iframe that doesn't have the allow-same-origin bit set.

Comment 10

[(no longer active)]

To answer the question of what I think our behavior should be, I think we should only honor the meta tag if |doc->IsHTMLorXHTML() && !doc->GetDisplayDocument()|.  We should also only honor such meta tags if they appear within the head element, per spec.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> where per step 1 we can ignore the cookie.

Well... sort of, in the sense that not setting cookies ever is technically spec-compliant.

But in practice, the HTML spec should spell out cases in which _setting_ a cookie would be a spec violation.  And this should probably be one of those cases.

> What do you think, Boris?

Then text/html XHR results will set cookies, right?  And HTML imports?  And so forth...

Imo we should restrict this to "non-data" documents.

Comment 12

[(no longer active)]

That sounds good to me.

Comment 13

[Frederik Braun [:freddy]]

Is this a duplicate of bug 1317641?

Comment 14

[Boris Zbarsky [:bzbarsky]]

No.  This is about SVG images just served over http:.

Comment 15

[Jun (Request needinfo for comment, I ignore other updates)]

And I also wrote about img tag having data: URI.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, fair.  That's still not a duplicate of bug 1317641, because <img> tag with data: URI is in fact expected to be treated as same-origin.

Comment 17

[Jun (Request needinfo for comment, I ignore other updates)]

Hi, I know some bounty program which are affected by this bug. Will you fix this bug very soon? I'm worried because Alex's blog is wide spread and I'm sure there are many people who knows about this bug by now.

Comment 18

[(no longer active)]

Andrew, I think we need an owner to take this.

Comment 19

[Andrew Overholt [:overholt]]

baku, please take this.

Comment 20

[Jun (Request needinfo for comment, I ignore other updates)]

Any update?

Comment 21

[Andrea Marchesini [:baku]]

Oh... I didn't know this was assigned to me. Sorry. I don't receive notification for assignment.
I'll take a look in these days.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: cookie.patch

> Actual results:
> 
> https://shhnjk.com/ is not accessible and says "Bad request" due to Cookie
> bomb. Same attack can be done from https://vuln.shhnjk.com/svg_data.html.

I cannot reproduce this issue using jsfiddle.net: when the svg image is loaded from https://jsfiddle.net/26nLj6mL/1/, the cookies are not set and not sent to https://shhnjk.com/ when that URI is loaded.

But I can reproduce it when loading the data: URL approach. Here a fix.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> when the svg image is loaded from https://jsfiddle.net/26nLj6mL/1/, the cookies are not set
> and not sent to https://shhnjk.com/ when that URI is loaded

Why not?  What prevents it?

Disabling all <meta> for data: is clearly wrong.

The patch says it implements comment 10 and 11, but what it does looks nothing like those comments.

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

I am still able to reproduce in Nightly Fx55.0a1. By visiting site #1, opening Preferences and viewing cookie store. I can see the cookies for site #2's domain. 

However, the behavior of error message "Bad request" is something that I cannot reproduce. Site #2 loads just fine.

Comment 25

[Jun (Request needinfo for comment, I ignore other updates)]

Hi, could anyone assign severity of this issue?

Comment 26

[Jun (Request needinfo for comment, I ignore other updates)]

You were too late. Already went public by Gareth https://twitter.com/garethheyes/status/864755217522470912

Comment 27

[Jun (Request needinfo for comment, I ignore other updates)]

BTW, I guess you could do something like this to set cookie too.
<a href="" style="background:url('data:image/svg+xml;base64,asdfasdf')">

So maybe using WebVTT style import too? Better fix it soon.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: svg.patch

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877941 [details] [diff] [review]
svg.patch

The !doc->IsLoadedAsData() block makes no sense.  What it's doing doesn't match its comment, for example.

What actual policy are you trying to implement?  Has an HTML spec issue been raised?

Comment 31

[Andrea Marchesini [:baku]]

> Then text/html XHR results will set cookies, right?  And HTML imports?  And
> so forth...
> 
> Imo we should restrict this to "non-data" documents.

Maybe I misunderstood your comment here.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: svg.patch

Comment 33

[Boris Zbarsky [:bzbarsky]]

> Maybe I misunderstood your comment here.

Maybe?  I suggested that we should only allow "non-data" documents to set cookies via <meta>.  The patch in attachment 8877941 [details] [diff] [review], on the other hand, allows any data document that is HTML/XHTML to do so no matter what, but for non-data documents only allows it if the <meta> is a descendant of <head>.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8879075 [details] [diff] [review]
svg.patch

I would really appreciate answers to my questions from comment 30.  It's hard to review this patch without having some idea of what it's trying to accomplish.

Comment 36

[Andrew McCreight [:mccr8]]

Andrew, can we get this bug unstuck somehow? It is almost 7 months old. Thanks.

Comment 37

[Andrew Overholt [:overholt]]

Maybe Blake has an idea while baku's on PTO?

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

I spoke with bz on IRC and have a plan: we should be using doc->IsLoadedAsData() to filter "data documents" out (with a comment about IsLoadedAsInteractiveData).

I'm also going to send email to Anne and Dominic about fixing the spec to follow reality.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Anne, please see comment 10 and 11. We'd like to ignore <meta> in non-(X)HTML documents and (X)HTML documents that are "data documents" (though that concept doesn't seem to be defined in the spec as far as I can tell). I don't know the best way to track this given that this is a security bug and the spec's github issues are public.

(For my reference: https://html.spec.whatwg.org/multipage/semantics.html#the-meta-element)

Comment 40

[Anne (:annevk)]

I think we should be using https://html.spec.whatwg.org/multipage/dom.html#cookie-averse-document-object instead. That's what document.cookie uses and seems more appropriate.

Can we set some kind of flag that whenever this bug is opened to the public it triggers something that results in specification follow-up? It seems fine to not fix the specification for a bit as long as everyone that is shipping an affected product is informed.

Comment 41

[Anne (:annevk)]

See also https://github.com/whatwg/html/issues/1950.

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

As a note: this testcase actually asserts in debug builds after bug 1331680.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implement the "cookie averse document" concept

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a820f19f9c0ace86e75ce801aef3b98e7b52f5fd

MozReview-Commit-ID: GndxqhU77cS

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the bug

MozReview-Commit-ID: 6tuaEqQA551

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Use this API where we're supposed to

According to [1], the cookie getters and setters are supposed to check IsCookieAverse before doing anything.


[1] https://html.spec.whatwg.org/multipage/dom.html#resource-metadata-management:cookie-averse-document-object

MozReview-Commit-ID: HGU5YtUzv9U

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a mochitest.

MozReview-Commit-ID: GDJnC38RafH

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Use this API where we're supposed to

According to the spec, we should check for cookie averseness before getting or setting cookies. The spec isn't clear about precedence wrt the CSP check but there's a WPT that fails if we check for document averseness before the CSP check (and therefore don't throw).

MozReview-Commit-ID: HGU5YtUzv9U

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c52e16ec4c974ee49028b99fb65f50e9378848&selectedJob=124249910

I'm assuming that the linux-opt-wpt1 failure on websocket-allowed.https.html is unrelated to this change but plan on investigating further on Monday to double-check.

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8898522 [details] [diff] [review]
Implement the "cookie averse document" concept

Ehsan, this doesn't exactly implement comment 9 and 10, but it appears to be the direction the spec is taking (see comment 40 and 41) and it does fix the bug.

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8898525 [details] [diff] [review]
Add a mochitest.

I have to admit that I cribbed a bunch of this from the original jsfiddle. This test would be in the public domain once it lands. Jun, are you OK with that?

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8899026 [details] [diff] [review]
Use this API where we're supposed to

This patch is orthogonal to this bug, but it brings us more in line with the spec. I would also like to file a followup bug on possibly combining this with nsHTMLDocument::mDisableCookieAccess.

Comment 52

[(no longer active)]

Comment on attachment 8898522 [details] [diff] [review]
Implement the "cookie averse document" concept

Review of attachment 8898522 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIDocument.h
@@ +2095,5 @@
> +    nsAutoCString scheme;
> +    codebaseURI->GetScheme(scheme);
> +    return !scheme.EqualsLiteral("http") &&
> +           !scheme.EqualsLiteral("https") &&
> +           !scheme.EqualsLiteral("ftp");

This will disable document.cookie on file:// URIs.  It's worth making sure that what we are doing here is compatible with what WebKit, Blink and EdgeHTML are shipping.  r=me assuming that is the case but please double check that.  If that is not the case, please let me know, depending on what the reality is we may need to consider updating the spec or do something else...

Comment 53

[(no longer active)]

Comment on attachment 8898525 [details] [diff] [review]
Add a mochitest.

Review of attachment 8898525 [details] [diff] [review]:
-----------------------------------------------------------------

Please make this a wpt test so that we ensure cross-browser compatibility on this behavior.  Thanks!

Comment 54

[(no longer active)]

Comment on attachment 8899026 [details] [diff] [review]
Use this API where we're supposed to

Review of attachment 8899026 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, IMO it's a bug in the spec that it doesn't clarify the order of processing here!  Please file a spec issue and note a link to it in the commit message or something?  It'd be nice if you mentioned the wpt test that depends on the processing order there as well!  Thank you.

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #52)
> This will disable document.cookie on file:// URIs.  It's worth making sure
> that what we are doing here is compatible with what WebKit, Blink and
> EdgeHTML are shipping.

Ehsan was spot-on here. I tested and found that Safari, Edge, and Firefox (before this patch) all allow cookies on file URIs. Chromium is the odd browser out that does not. This seems like a spec bug that we should raise against the definition of "cookie-averse documents" -- Anne, that does leave us with a decision about what we should do in the meantime. Firefox has explicit code to allow file: URIs (which don't have hosts/base domains) to set cookies but doesn't allow other types of host-less URIs to set them.

So, I can either explicitly allow file: URIs to the list at [1] (keep the explicit whitelist in the spec and extending it) or, instead, check either that the document's codebase URI either is a file URI or has a base domain. In the "old world" of Gecko, where addons could add protocols and URI implementations, I'd probably prefer the latter solution, but now that we're in the new webextensions world, maybe we can hold a firmer line and have a whitelist?

Anne, what do you think?

[1] https://fetch.spec.whatwg.org/#network-scheme

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a WPT

Here's a WPT that does the same thing as the previously-added mochitest.

Comment 57

[(no longer active)]

Comment on attachment 8900053 [details] [diff] [review]
Add a WPT

Review of attachment 8900053 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed even though there is no way for you to know about those issues by pushing to try or anything.   :-/

::: testing/web-platform/tests/html/dom/documents/resource-metadata-management/document-cookie-image.html
@@ +13,5 @@
> +      addEventListener("message", event => { resolve(event.data.cookie) }, false);
> +    }, { once: true });
> +
> +    let frame = document.createElement("iframe");
> +    frame.src = "http://www2.web-platform.test:8000/html/dom/documents/resource-metadata-management/file-get-cookie.html";

Instead of hardcoding this, you should include /common/get-host-info.sub.js and use the 'OTHER_HOST' key.  See https://searchfox.org/mozilla-central/source/testing/web-platform/tests/common/get-host-info.sub.js.

Also see http://web-platform-tests.org/writing-tests/server-features.html#tests-involving-multiple-origins for how the underlying magic works!

Last but not least, please use the location object to figure out the path to the current test rather than hardcoding that one as well, because these tests should run when served from any arbitrary HTTP server, not just Mozilla's setup.  |location.pathname.replace(/\/[^\/]*$/, '/')| should do the trick.

@@ +20,5 @@
> +    assert_equals(cookie, "", "should not have a cookie");
> +  }, "loads the image and verifies that it doesn't set a cookie");
> +</script>
> +<body>
> +<img src="http://www2.web-platform.test:8000/html/dom/documents/resource-metadata-management/svg-image-setcookie.svg">

Ditto.

Comment 58

[Anne (:annevk)]

I'd just add "file" to the safelist. file: URLs are typically not considered (as they're not cross-platform other than parsing and not really the web) and I'm not sure "file" should be added to the definition of "network scheme" as that would also affect other things, but I suppose that in the definition of cookie-averse we could mention it somehow. Hope that helps.

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Cookie averse + file:

This implements "cookie averse document" and explicitly allows file: URIs.

Comment 60

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixed WPT

I used the templating stuff everywhere since I needed the img src to be substituted too.

Comment 61

[(no longer active)]

Comment on attachment 8900420 [details] [diff] [review]
Cookie averse + file:

Review of attachment 8900420 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIDocument.h
@@ +2097,5 @@
> +    codebaseURI->GetScheme(scheme);
> +    return !scheme.EqualsLiteral("http") &&
> +           !scheme.EqualsLiteral("https") &&
> +           !scheme.EqualsLiteral("ftp") &&
> +           !scheme.EqualsLiteral("file");

Please file the issue to add this to the spec as well, thanks!

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0c42c5bea67
https://hg.mozilla.org/mozilla-central/rev/a16368bbecb2
https://hg.mozilla.org/mozilla-central/rev/6246d30fbec6

Looks like the test never got landed? Also, do we intend to backport this to Beta or let it ride the 56 train. I assume it's not worth backporting to ESR52 either way.

Comment 63

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #62)
> Looks like the test never got landed? Also, do we intend to backport this to

After talking to dveditz, I think we should hold off on landing the test until this ships.

> Beta or let it ride the 56 train. I assume it's not worth backporting to
> ESR52 either way.

I think that given the risk that cookies bring, we should hold off on backporting this to branches. It's been labeled as sec-moderate, so I don't think we have to rush and take on the extra risk that incurs.

Comment 64

[Blake Kaplan (:mrbkap) (inactive)]

I filed https://github.com/whatwg/html/issues/3008 on the file: URI issue in the HTML spec.