Bug 1442840 (CVE-2018-5176)

Iframe injection & content spoofing & scripts execution via json viewer

VERIFIED FIXED in Firefox 60

Status

defect
VERIFIED FIXED
Last year
9 months ago

People

(Reporter: oc3f.dz, Assigned: Gijs)

Tracking

({csectype-sop, sec-moderate})

58 Branch
Firefox 61
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(Whiteboard: [adv-main60+])

Attachments

(3 attachments)

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......
Posted image PoC.png
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?
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: JSON Viewer
Ever confirmed: true
Flags: needinfo?(odvarko)
Flags: needinfo?(ckerschb)
Summary: Iframe injection & content spoofing & scripts execution via json response → Iframe injection & content spoofing & scripts execution via json viewer
Calling it "moderate" because of the interaction required and it wouldn't apply to all sites, but this is Not Good.
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).
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Flags: needinfo?(ckerschb)
Posted patch Patch v1Splinter Review
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`).
Attachment #8957304 - Flags: review?(bgrinstead)
Posted patch CSP patch v1Splinter Review
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.
Attachment #8957306 - Flags: review?(ckerschb)
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.
Attachment #8957304 - Flags: review?(bgrinstead) → review+
(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.
Flags: needinfo?(nchevobbe)
(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? :-)
(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.
Hello ,
Can I know if this bug is eligible for bounty ? 
Thanks in advance
(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
Flags: needinfo?(nchevobbe)
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?
Gijs, see previous comment please.
Flags: needinfo?(gijskruitbosch+bugs)
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)
(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/addictive_typing_lessons@tomkennedy.net/feature_compatibility/ ) .
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ckerschb)
Flags: needinfo?(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?
Attachment #8957306 - Flags: review?(honzab.moz)
Attachment #8957306 - Flags: review?(ckerschb)
Attachment #8957306 - Flags: review+
(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 on attachment 8957306 [details] [diff] [review]
CSP patch v1

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

wrong honza
Attachment #8957306 - Flags: review?(honzab.moz) → review?(odvarko)
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
Attachment #8957306 - Flags: review?(odvarko)
(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)? :-)
Flags: needinfo?(ckerschb)
Flags: needinfo?(ckerschb)
I pinged Honza (mayhemer) on IRC; he accidentally redirected to the other Honza ;-) He will take another look.
(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.
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 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.
Attachment #8957304 - Flags: approval-mozilla-beta?
Comment on attachment 8957306 [details] [diff] [review]
CSP patch v1

Approval Request is in Comment 28
Attachment #8957306 - Flags: approval-mozilla-beta?
 Can I know if this bug is eligible for bounty ?
Flags: needinfo?(gijskruitbosch+bugs)
(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 security@mozilla.org with the bug number (but not details of the actual security issue) and ask to have it be considered for a bounty.
Flags: needinfo?(gijskruitbosch+bugs)
(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
(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. :-)
(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
Flags: sec-bounty?
Comment on attachment 8957304 [details] [diff] [review]
Patch v1

json viewer sec fix, beta60+
Attachment #8957304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8957306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta uplift, or approval requests on dependent bugs.
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: CVE-2018-5167
(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
Flags: needinfo?(gijskruitbosch+bugs)
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
Flags: needinfo?(gijskruitbosch+bugs)
Flags: sec-bounty? → sec-bounty+
Group: firefox-core-security → core-security-release
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.
(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!
Considering the information from comment 43 and comment 44 I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main60+]
Alias: CVE-2018-5176
Product: Firefox → DevTools
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.