1561502 - (CVE-2019-11761) By using a form with a data URI it's possible to gain access to the privileged JSONView object that has been cloned into content.

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect, P2)

Description

[Cody Crews]

Attachment: ObtainJSONView-testcase-new.html

I'm attaching a testcase that shows this work. When viewing the testcase click the submit button, and then open the console window and look at testWin.JSONView

The issue here is that defense in depth against data URIs in new tabs actually leaves a small window of time in which it is possible to gain a reference to a JSONView window/tab before it is closed.

This tab just leaves the JSONView Object that has been cloned into the content scope accessible from the window reference that is held.

I was planning to do more work on this seeing as this is a great stepping stone, but my situation and time right now aren't going to allow it.

In the past I believe we treated this as sec-high so yeah.

Comment 1

[Cody Crews]

I thought I should point out that since bugzilla and the attachments are separate you do have to confirm that you are leaving bugzilla with the testcase hosted here. Still works though.

Comment 2

[:Gijs (he/him)]

Ugh. Looking. (Sorry for slow, I've been on holiday for the last few days... still jetlagged so ymmv. CC'd a bunch of other folks who might have an interest.)

Comment 3

[:Gijs (he/him)]

OK, I have 3 questions for different domain experts:

1. why do we block the toplevel data URI load "too late" here, and is there any chance of doing it sooner? (Christoph)
2. this custom mimetype stuff is scary. Can we stop allowing websites to trip this? (Boris/Bobby)
3. Can we ensure the document gets a null principal and/or ensure that the content-exposed JSONView thing is only accessible from the null principal associated with that document? (unsure if this alone is sufficient here, if the document is alive and the controlling page that creates it can put/run script in it, it might not be) (Boris/Bobby)

Comment 4

[Boris Zbarsky [:bzbarsky]]

this custom mimetype stuff is scary. Can we stop allowing websites to trip this?

Well, we shouldn't be building the JSON viewer on top of a stream converter to a custom mimetype to start with, and make it be more like the XML viewer, but people ignored me when I told them that... That said, the real issue is the various script being injected into the global scope, which would probably still be an issue, right?

The only reason the random MIME type matters here is that unlike application/json it triggers the data: detection to close the window.

Can we ensure the document gets a null principal and/or ensure that the content-exposed JSONView thing is only accessible from the null principal associated with that document?

Just to be clear, the document in question is "about:blank".

So the sequence of events here is:

1. Page opens a new window. That window contains an "about:blank", which is of course same-origin with the page that did the window.open call. The script on the page has a reference to this window.

2. We start loading the data: URL in there.

3. The data: blocking kicks in, blocks the load, closes the new tab.

The page still has a reference to the window in question, which contains a same-origin "about:blank", so it can touch its properties. We can't retroactively mutate the principal of the "about:blank", obviously.

It looks like JSONView gets injected in onStartRequest in converter-child.js. In the "normal" case (application/json), we get into this code's onStartRequest, call this.listener.onStartRequest(request), that sets up the new window with a nullprincipal for the JSON viewer (because of the resetPrincipalToInheritToNullPrincipal call), and then we inject in it.

In the testcase in this bug, we call this.listener.onStartRequest(request), that does NOT set up a new window, and cancels the channel instead. We ignore that, and go on to inject into the window, but of course now we're injecting into the wrong window.

So a simple fix would be to check that request.status is still success after that onStartRequest call, and if not skip all the rest of the work. In particular, this code as written is violating Necko's callback contract: it's calling onDataAvailable after the request has been canceled! Similar in onStopRequest, actually...

A more robust fix would be to do all that and also avoid injecting if the principal of win doesn't match request.loadInfo.principalToInherit, because if that's the case then we're not injecting into the right thing.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

Needs tests from someone who knows how to test this stuff...

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Can someone suggest a rating here? How dangerous is this, really?

Comment 7

[:Gijs (he/him)]

(In reply to Al Billings [:abillings] from comment #6)

Can someone suggest a rating here? How dangerous is this, really?

Depends how much can be done with a would-be privileged object. The APIs exposed by the JSONView object don't allow you to do much of anything anymore (they used to allow saving a file to disk, but afaict that's not a directly exposed API on the object but done using events (bug 1297361), so I don't see how you'd activate it on the object once the window it belonged to has gone away).

Normally, I think wrappers are supposed to save our bacon here - but this particular object is cloned into content so content can get access, and the exploit gets access to the clone (I think), not the original object. So I'm not sure there's much left. It also doesn't look like we ever re-access the object from chrome so it's not like you could break assumptions about that later.

That said, looks like https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/devtools/client/jsonview/converter-child.js#193-225 landed in bug 1460840 on 62, and I don't know if that means we're in a slightly worse place on 60-esr.

bug 1297361 and bug 1333210 would make me think the current incarnation would be sec-low or sec-moderate, esp. based on the discussion in bug 1333210. But Bobby or Boris should probably confirm I'm not missing something...

Comment 8

[Cody Crews]

So gaining access to chrome privileged objects, that fully clone into content and remain accessible(not my fault) is a sec-low/moderate now? Will wait on bz or bobby, but this is the perfect stepping stone Gijs, nothing else hanging that I could see and I assure you with time on this that low/moderate will jump to a high/critical. There's functions here that allow one to attempt the bypass of xray wrappers. Will wait and see, thinking I should have put more into this instead of cutting trees ;)

Comment 9

[Bobby Holley (:bholley)]

I haven't looked at the details of this bug, but here's my take:

(1) In general, a regular content object that was created as a clone of a privileged object is not sensitive. It's just a content object after the clone.
(2) The one potential to exception to the above would be any function wrappers created with cloneFunctions/exportFunction. However, the intent is for that exposure mechanism to be treated as explicit API exposure to untrusted code, and thus privileged code doing function exports should be treating calls to those functions as untrusted.
(3) An exported function that is not safe to be called by untrusted code would qualify as a specific security issue with that function, along with any mistaken notion that calls to such a function could be reliably regulated after injecting it into the content scope.

So I don't consider content access to a cloned object to be a security issue, unless it has unsafe exported functions. That said, I'm generally in favor of paying partial bounties for concept code that motivates us to implement useful defense-in-depth improvements, even if the concept code was not a complete exploit.

Comment 10

[:Gijs (he/him)]

(In reply to Cody Crews from comment #8)

So gaining access to chrome privileged objects, that fully clone into content and remain accessible(not my fault) is a sec-low/moderate now?

The point is, the object you get access to isn't actually chrome privileged. The implementation was changed since the earlier bugs you reported on the JSON viewer.

Getting access to an object that isn't privileged but that exposes methods/data that would not normally be web-accessible would still be interesting, but AFAICT it doesn't do that either.

(edit: hm, I didn't see Bobby's comment before replying but didn't actually get asked about a mid-air by bugzilla. In any case, I agree with comment #9)

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Cody Crews from comment #8)

I assure you with time on this that low/moderate will jump to a high/critical.

That perfectly sums up the definition of sec-moderate :-) Not so bad on its own, but the potential to be combined with other bugs, features, or maybe just additional knowledge, to make something abusable. Given some of the other discussion (this is a clone, chrome code never does anything with it after) it might even be sec-low, but I'm willing to go with sec-moderate on the premise that you might find more here.

Comment 12

[Cody Crews]

Fair enough Dan, not trying to be difficult just venting a little. More than once I felt like calling my actual day job and just focusing here, which now I feel was right.

I'll be in touch, hopefully with something better. Time to see those lights I think.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Sorry for the slow response here.

(In reply to :Gijs (he/him) from comment #3)

1. why do we block the toplevel data URI load "too late" here, and is there any chance of doing it sooner? (Christoph)

We tried blocking data: URI navigations earlier in the lifecycle, but we figured we have to wait for the response so we can actually decide whether to render or download, because we don't want to block things that are not rendered in the browser anyway.

To sum it up, we can not block data: URI navigations earlier, we have to wait till we hit nsDSURIContentListener::DoContent
https://hg.mozilla.org/mozilla-central/rev/0c4ecb840463#l1.32

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:Honza, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 15

[Cody Crews]

Hey guys I know I'm lacking lately appreciate all the work here. I would almost literally kill for the stability and time to get back up to date with everything. Working toward it, who knows maybe I'll get there. Thanks again, appreciate the hard work from you guys as always. Keep it up, keeps me motivated too.

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1561502, r?bzbarsky!,Honza!

Comment 18

[:Gijs (he/him)]

Attachment: Bug 1561502, r?Honza

Depends on D42120

Comment 19

[:Gijs (he/him)]

Attachment: Bug 1561502, r?Honza

Depends on D42122

Comment 20

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

https://hg.mozilla.org/integration/autoland/rev/99fa9dc45ddc665417995b52708657dbff2448a9
https://hg.mozilla.org/integration/autoland/rev/797a69c27ac58303ef3da41c9b97f75887123bd9
https://hg.mozilla.org/integration/autoland/rev/61623fea51f3eff1876134a5365320c3dc8f8f9b
https://hg.mozilla.org/mozilla-central/rev/99fa9dc45ddc
https://hg.mozilla.org/mozilla-central/rev/797a69c27ac5
https://hg.mozilla.org/mozilla-central/rev/61623fea51f3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Per IRC discussion with Gijs, it's a bit late in the cycle to take this for Fx69 (we're building the RC next week), so we think it'd be best to let this fix ride with Fx70 and uplift to ESR68 during the next cycle for the 68.2esr release shipping alongside 70.

Comment 22

[Brindusa Tot, Desktop QA]

Reproduced on Beta 69: the testWin.jsonView object is accessible. On latest Nightly 70.0a1, build ID 20190829214656 the same object is not accessible anymore. Verified on Windows 10 and Ubuntu 18.04.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR68 approval when you get a chance.

Comment 24

[:Gijs (he/him)]

Comment on attachment 9085739 [details]
Bug 1561502, r?bzbarsky!,Honza!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: defense in depth fixes for jsonview
• User impact if declined: potential for further security issues
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fairly straightforward changes to reduce attack surface on the jsonview code, has had some baking time without any regressions being reported
• String or UUID changes made by this patch: nope

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9085739 [details]
Bug 1561502, r?bzbarsky!,Honza!

Baked on Beta70 without any known issues so far. Approved for 68.2esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/eb4abe45c758
https://hg.mozilla.org/releases/mozilla-esr68/rev/2fba8a40abcc
https://hg.mozilla.org/releases/mozilla-esr68/rev/0695f4d48980

Comment 27

[Brindusa Tot, Desktop QA]

Verified as fixed on Windows 10 and Ubuntu 16.04 using ESR68, Build ID 20190918214957 (build from taskcluster: https://tools.taskcluster.net/index/gecko.v2.mozilla-esr68.pushdate.2019.09.18.20190918214957)

Comment 28

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt