Bug 1325923 (CVE-2017-7837)

SVG image can be used to set cookie

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: s.h.h.n.j.k, Assigned: mrbkap)

Tracking

({sec-moderate})

Trunk
mozilla57
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(Whiteboard: [adv-main57+][post-critsmash-triage])

Attachments

(4 attachments, 7 obsolete attachments)

1.15 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.85 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.58 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.06 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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

2 years ago
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).
Flags: needinfo?(dholbert)
Flags: needinfo?(dd.mozilla)

Updated

2 years ago
See Also: → CVE-2016-9078
This looks like a principal issue. Also needinfo Christoph, maybe we know a fast answer.
(I have not try it yet).
Flags: needinfo?(ckerschb)
(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?
Flags: needinfo?(dholbert) → needinfo?(bzbarsky)
Group: firefox-core-security → core-security
Component: Untriaged → SVG
Product: Firefox → Core
Version: 1.0 Branch → Trunk
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...
Flags: needinfo?(bzbarsky)
Group: core-security → dom-core-security
(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.
Flags: needinfo?(ckerschb)
Flags: needinfo?(dd.mozilla)
(Reporter)

Comment 6

2 years ago
Hi,

Any update on severity or status of this issue? Will it take quite some time to fix it?
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?
Flags: needinfo?(jduell.mcbugs)

Comment 8

2 years ago
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.
Flags: needinfo?(jduell.mcbugs) → needinfo?(ehsan)

Comment 9

2 years ago
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.
Flags: needinfo?(ehsan)

Comment 10

2 years ago
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.
> 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

2 years ago
That sounds good to me.
Is this a duplicate of bug 1317641?
No.  This is about SVG images just served over http:.
(Reporter)

Comment 15

2 years ago
And I also wrote about img tag having data: URI.
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.
(Reporter)

Comment 17

2 years ago
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

2 years ago
Andrew, I think we need an owner to take this.
Flags: needinfo?(overholt)
baku, please take this.
Assignee: nobody → amarchesini
Flags: needinfo?(overholt)
(Reporter)

Comment 20

2 years ago
Any update?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Posted patch cookie.patch (obsolete) — Splinter Review
> 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.
Attachment #8849043 - Flags: review?(ehsan)
> 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.
Attachment #8849043 - Flags: review?(ehsan) → review-
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.
(Reporter)

Comment 25

2 years ago
Hi, could anyone assign severity of this issue?
Flags: sec-bounty?
(Reporter)

Comment 26

2 years ago
You were too late. Already went public by Gareth https://twitter.com/garethheyes/status/864755217522470912
(Reporter)

Comment 27

2 years ago
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.
Duplicate of this bug: 1368465
Posted patch svg.patch (obsolete) — Splinter Review
Attachment #8849043 - Attachment is obsolete: true
Attachment #8877941 - Flags: review?(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?
Attachment #8877941 - Flags: review?(bzbarsky) → review-
> 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.
Flags: needinfo?(bzbarsky)
Posted patch svg.patch (obsolete) — Splinter Review
Attachment #8877941 - Attachment is obsolete: true
Attachment #8879075 - Flags: review?(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>.
Flags: needinfo?(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.
Attachment #8879075 - Flags: review?(bzbarsky)
Duplicate of this bug: 1379772
Andrew, can we get this bug unstuck somehow? It is almost 7 months old. Thanks.
Flags: needinfo?(overholt)
Maybe Blake has an idea while baku's on PTO?
Flags: needinfo?(overholt) → needinfo?(mrbkap)
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.
Assignee: amarchesini → mrbkap
Flags: needinfo?(mrbkap)
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)
Flags: needinfo?(annevk)

Comment 40

2 years ago
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.
Flags: needinfo?(annevk)
As a note: this testcase actually asserts in debug builds after bug 1331680.
Attachment #8879075 - Attachment is obsolete: true
Posted patch Fix the bugSplinter Review
MozReview-Commit-ID: 6tuaEqQA551
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
Posted patch Add a mochitest. (obsolete) — Splinter Review
MozReview-Commit-ID: GDJnC38RafH
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
Attachment #8898524 - Attachment is obsolete: true
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 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.
Attachment #8898522 - Flags: review?(ehsan)
Attachment #8898523 - Flags: review?(ehsan)
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?
Attachment #8898525 - Flags: review?(ehsan)
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.
Attachment #8899026 - Flags: review?(ehsan)

Comment 52

2 years ago
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...
Attachment #8898522 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8898523 - Flags: review?(ehsan) → review+

Comment 53

2 years ago
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!
Attachment #8898525 - Flags: review?(ehsan) → review-

Comment 54

2 years ago
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.
Attachment #8899026 - Flags: review?(ehsan) → review+
(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
Flags: needinfo?(annevk)
Posted patch Add a WPT (obsolete) — Splinter Review
Here's a WPT that does the same thing as the previously-added mochitest.
Attachment #8900053 - Flags: review?(ehsan)
Attachment #8898525 - Attachment is obsolete: true

Comment 57

2 years ago
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.
Attachment #8900053 - Flags: review?(ehsan) → review+

Comment 58

2 years ago
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.
Flags: needinfo?(annevk)
This implements "cookie averse document" and explicitly allows file: URIs.
Attachment #8900420 - Flags: review?(ehsan)
Attachment #8898522 - Attachment is obsolete: true
Posted patch Fixed WPTSplinter Review
I used the templating stuff everywhere since I needed the img src to be substituted too.
Attachment #8900423 - Flags: review?(ehsan)
Attachment #8900053 - Attachment is obsolete: true

Comment 61

2 years ago
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!
Attachment #8900420 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8900423 - Flags: review?(ehsan) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(mrbkap)
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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.
Flags: needinfo?(mrbkap)
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
I filed https://github.com/whatwg/html/issues/3008 on the file: URI issue in the HTML spec.

Updated

2 years ago
Duplicate of this bug: 1403146
Whiteboard: [adv-main57+]
Alias: CVE-2017-7837
Flags: qe-verify-
Whiteboard: [adv-main57+] → [adv-main57+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.