1333210 - JSON Viewer: Make even top level windows protected from content level window access.

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Jerri Rice (rehash pending)]

+++ This bug was initially created as a clone of Bug #1297361 +++

Follow up for:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c26

JSON viewer uses `Cu.exportFunction` to export `postChromeMessage` method into content scope to create a communication channel (used for, copying & saving JSON data and HTTP headers from the content).

This should be replaced by window listeners.

Honza

Follow up, this is fine and all, but with content able to open the json viewer in a top level window, and that window remaining accessible from the content window, we still have some work to do.  The problem is that the patch is checking that the event is dispatched from the proper document, but there is no check to see if the event is trusted(which I don't think it can be here.)

So with a content level window accessing this window, it would still be possible for that content window to dispatch events using eval or whatever from the json viewer window so that it would in fact be from the same document.  The problem is that the content window could put whatever object in as the data to be sent with the event.

I hope people are following me here.

The answer in my mind is obvious.  This is intended to be a part of the developer tools, so just make any window that has the content type of application/json or the internal Mozilla equivalent(I cant remember right off) inaccessible from content level windows.

I'll await thoughts on this, while pursuing other bugs.  Trying to spread my attention to as many places as possible while I can.

Comment 1

[:Gijs (out for now; he/him)]

This is starting to feel like whack-a-mole.

Why is the document even same-origin with the opener?

Comment 2

[:Gijs (out for now; he/him)]

Ah, no, we fixed this in bug 1305012. Or rather, we added infra to fix this. I bet we're not using it yet.

Note that this feature is still turned off on release.

Comment 3

[:Gijs (out for now; he/him)]

Attachment: Call resetPrincipalsToNullPrincipal for the json viewer

MozReview-Commit-ID: 2Gu8sv36Yyy

Comment 4

[:Gijs (out for now; he/him)]

Note that 51 is already shipping, the feature is disabled there, and we only landed the API this uses in 52, so 51 seems pretty conclusively wontfix to me.

Comment 5

[Jerri Rice (rehash pending)]

(In reply to :Gijs from comment #1)
> This is starting to feel like whack-a-mole.
> 
> Why is the document even same-origin with the opener?

Sorry this was my slip up.  I remember stating that this was in fact how it worked, but I didn't continue to stress the point and forgot once we starting patching things up.

At least I caught it, which makes me feel a little better.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Jerri Rice (rehash pending) from comment #5)
> (In reply to :Gijs from comment #1)
> > This is starting to feel like whack-a-mole.
> > 
> > Why is the document even same-origin with the opener?
> 
> Sorry this was my slip up.  I remember stating that this was in fact how it
> worked, but I didn't continue to stress the point and forgot once we
> starting patching things up.
> 
> At least I caught it, which makes me feel a little better.

Yeah, sorry, not complaining at you, just frustrated with myself, I think. Should have followed up more. And yes, at least we're fixing it now.

Comment 7

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

Comment on attachment 8829652 [details] [diff] [review]
Call resetPrincipalsToNullPrincipal for the json viewer

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

Thanks for the patch Gijs!

R+, assuming try is green.

Honza

::: devtools/client/jsonview/converter-child.js
@@ +109,5 @@
>  
>      this.channel = request;
>      this.channel.contentType = "text/html";
>      this.channel.contentCharset = "UTF-8";
> +    this.channel.loadInfo.resetPrincipalsToNullPrincipal();

Could we have a short comment explaining this API call. Something like: 

Reset channel principal to null. After the channel (contents) has been manipulated, it should no longer be same-origin with the original content.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8829652 [details] [diff] [review]
Call resetPrincipalsToNullPrincipal for the json viewer

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

r=me and thanks - finally we have a user of resetPrincipalsToNullPrincipal in the wild.

::: devtools/client/jsonview/converter-child.js
@@ +114,1 @@
>  

+1 for the adding the comment.

Comment 9

[:Gijs (out for now; he/him)]

This needs a security severity put on it before I push to try or land. The other bug was marked sec-high.

Comment 10

[:Gijs (out for now; he/him)]

Attachment: Patch with comment

Comment 11

[:Gijs (out for now; he/him)]

Per discussion on IRC with dveditz, OK to land this, so trypushed:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b090f557a269937359f76d5ff5bf427b168ac6e

Comment 12

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf09c87b198b9b59ed84bc1bc76236fa1d9e972

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bf09c87b198

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8829857 [details] [diff] [review]
Patch with comment

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: content code can potentially invoke the privileged JSON viewer APIs
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not manually, but automated tests pass
[Needs manual test from QE? If yes, steps to reproduce]:  I don't think so.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: very small JS-only patch, automated test coverage, early in the cycle, in a feature that's disabled by default in beta anyway (but given that that's going to be an ESR and users might want to pref it on, and this is a security bug, I think we should take it in beta to be safe).
[String changes made/needed]: no.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8829857 [details] [diff] [review]
Patch with comment

fix json viewer issue in aurora53 and beta52

Should be in 52.0b3.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d73c486834d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5aa0f7f0237

Comment 18

[Andrei Vaida [:avaida]]

Per Comment 14 and since this has automated coverage, setting qe-verify-.

Comment 19

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

(In reply to :Gijs from comment #11)
> Per discussion on IRC with dveditz, OK to land this, so trypushed:
> 
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0b090f557a269937359f76d5ff5bf427b168ac6e

I don't know what Dan told you in channel but this bug needed:

1. A security rating
2. Sec-approval template filled out and then approved
3. Branch nominations made and then approved (usually by Release Management)

before it landed in various branches.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Can you please fill out the sec-approval? template so we have the answers to the security questions?

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8829857 [details] [diff] [review]
Patch with comment

(In reply to Al Billings [:abillings] from comment #19)
> (In reply to :Gijs from comment #11)
> > Per discussion on IRC with dveditz, OK to land this, so trypushed:
> > 
> > remote:  
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=0b090f557a269937359f76d5ff5bf427b168ac6e
> 
> I don't know what Dan told you in channel but this bug needed:
> 
> 1. A security rating

OK, so for next time: how do I expedite this process? As it was, it took 2 weeks from when this bug was filed, even though you and Dan were both CC'd from the beginning. Once there's an r+'d patch, if I can't even trypush to figure out if I'm going to need to make further changes (e.g. to tests), just letting the bug sit without landing the patch also represents risk.

FWIW, I don't think this is sec-high, and that played a role in my decisions re: landing this, in addition to the conversation with Dan.

> 2. Sec-approval template filled out and then approved
> 3. Branch nominations made and then approved (usually by Release Management)

It wouldn't have needed those if it got rated sec-moderate or lower, as far as I understand https://wiki.mozilla.org/Security/Bug_Approval_Process

> Can you please fill out the sec-approval? template so we have the answers to
> the security questions?

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch tells you that we use the origin principal for JSON viewed in the JSON viewer. A same-origin opener that has a popup blocker bypass (or convinces the user to click a link or similar) can then open a separate tab and 'make' the JSON viewer do one of the following things:
- copy arbitrary strings to the clipboard
- pop up a 'save file' dialog that saves arbitrary data.

My reasoning that this isn't sec-high is that content-privileged JS *can already do* both of these things without access to the JSON viewer, using the clipboard API and an <a href="data:..." download> link, respectively. I don't think access to the JSON viewer gets you anything else, as long as the JSON viewer doesn't get any other privileges - without this patch, it gets the same principal as the content - it can only do what the content could already do, besides the 'contentMessage' eventing support for copying and saving data. That's speaking without further/other exploits, of course... So it's a good defense in depth fix to make sure the JSON viewer never shares a compartment with a content caller, but I don't think the problem as-stated is sec-high.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's reasonably obvious, yes.

Which older supported branches are affected by this flaw?
Everything, but the JSON viewer is disabled by default on beta and release branches.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They've already landed on beta and aurora. It isn't possible to fix this on esr without also taking the fixes from bug 1305012 and potentially other bugs.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, I think the automated test coverage is sufficient here.

Comment 21

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

(In reply to :Gijs from comment #20)
> OK, so for next time: how do I expedite this process? 

Which process? Sec-approval? You mark sec-approval? on a patch and fill out the template. Those are examined every dya.

> As it was, it took 2
> weeks from when this bug was filed, even though you and Dan were both CC'd
> from the beginning.

Neither Dan or I are going to fix this bug. What was it that you expected us to do that we didn't do?

> Once there's an r+'d patch, if I can't even trypush to
> figure out if I'm going to need to make further changes (e.g. to tests),
> just letting the bug sit without landing the patch also represents risk.

 Then mark sec-approval? to get sec-approval. We have a process for this that goes back years and, generally, works without too much complaint.
 
> FWIW, I don't think this is sec-high, and that played a role in my decisions
> re: landing this, in addition to the conversation with Dan.

 Well, if it isn't a sec-high, what is the rating and why hasn't it been changed? :-)
 
> > 2. Sec-approval template filled out and then approved
> > 3. Branch nominations made and then approved (usually by Release Management)
> 
> It wouldn't have needed those if it got rated sec-moderate or lower, as far
> as I understand https://wiki.mozilla.org/Security/Bug_Approval_Process

 Yes, but it wasn't rated sec-moderate and no one asked us to re-rate it.

 I can't speak to IRC conversations because no one copied a transcript of them here. Bugzilla is the last word on all things and if the bug doesn't contain concerns or details, you can't expect people to know them.

Comment 22

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

If you're wanting to expedite security ratings (as I see on a re-read), critsmash meetings do the ratings. If it isn't happening quickly enough, needinfo? people (like me or Dan or Andrew McCreight) from Critsmash and ask for a rating.

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Al Billings [:abillings] from comment #21)
> (In reply to :Gijs from comment #20)
> > FWIW, I don't think this is sec-high, and that played a role in my decisions
> > re: landing this, in addition to the conversation with Dan.
> 
>  Well, if it isn't a sec-high, what is the rating and why hasn't it been
> changed? :-)

To be clear, when I talked to Dan there was no sec-rating on the bug at all yet, and yes the process of expediting getting a sec-rating was what I was asking about. I'll keep your advice on that in mind.

Now it's marked sec-high. I guess for disclosure reasons (this stuff ends up in the 'what bugs did we fix in what release, and how bad were they') it still makes sense to reconsider this. Per these arguments:


(In reply to :Gijs from comment #20)
> A same-origin opener that has a popup blocker bypass (or
> convinces the user to click a link or similar) can then open a separate tab
> and 'make' the JSON viewer do one of the following things:
> - copy arbitrary strings to the clipboard
> - pop up a 'save file' dialog that saves arbitrary data.
> 
> My reasoning that this isn't sec-high is that content-privileged JS *can
> already do* both of these things without access to the JSON viewer, using
> the clipboard API and an <a href="data:..." download> link, respectively. I
> don't think access to the JSON viewer gets you anything else, as long as the
> JSON viewer doesn't get any other privileges - without this patch, it gets
> the same principal as the content - it can only do what the content could
> already do, besides the 'contentMessage' eventing support for copying and
> saving data. That's speaking without further/other exploits, of course... So
> it's a good defense in depth fix to make sure the JSON viewer never shares a
> compartment with a content caller, but I don't think the problem as-stated
> is sec-high.

and the fact that the affected component is disabled by default on release, I would suggest it should be rated anywhere between sec-audit and sec-moderate. Up to you (but ni for this).


For posterity, my logs of my conversation with Dan are as follows:

[2017-01-27 18:22:30] <Gijs> hi! What would you like me to do with bug 1333210?
[2017-01-27 18:22:56] * Gijs is hoping he's OK to just trypush and land given we don't enable the json viewer on release etc., but no sec rating yet
[2017-01-27 19:05:08] <dveditz> Should be on to land
[2017-01-27 19:05:19] <dveditz> Ok
[2017-01-27 19:05:29] <Gijs> OK!

Comment 24

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

Dan and I are discussing re-rating this. I'll let him make the final determination.

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to :Gijs from comment #23)
> Now it's marked sec-high.

I over-interpreted your meaning in comment 9 plus "privileged JSON viewer APIs" in comment 14. comment 20 came later.

> I guess for disclosure reasons (this stuff ends up
> in the 'what bugs did we fix in what release, and how bad were they')

Yes for disclosure and also the bug bounty program, though since this feature is disabled in Release we won't include it in the formal advisories.

> it still makes sense to reconsider this.

It does indeed.

> > [... can] 'make' the JSON viewer do one of the following things:
> > - copy arbitrary strings to the clipboard
> > - pop up a 'save file' dialog that saves arbitrary data.

That's it for "privileged APIs"? You could even call that sec-low, or sec-moderate if we want to add in some "worst case" worries about what else we might add to that context in the future.

> and the fact that the affected component is disabled by default on release,

We prefer rating things based on how bad they will be if we didn't fix it by time we turned on the feature, and let the release status fields tell the "disabled" story for release prioritization purposes. That's for features we intend to turn on, as in this case, not "what if we shipped with random unsafe pref flips". 

> I would suggest it should be rated anywhere between sec-audit and sec-moderate.

"audit" is for things where the bug is about inspecting code to see if we have a problem (based on known bad patterns, perhaps). Not appropriate for things we know are in fact a problem.

> For posterity, my logs of my conversation with Dan are as follows:

That was based on the fact that the feature was disabled in shipping releases, but we want it fixed for the release where we're going to turn it on and I wanted to leave plenty of time to get uplifts approved.

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #25)
> (In reply to :Gijs from comment #23)
> > Now it's marked sec-high.
> 
> I over-interpreted your meaning in comment 9 plus "privileged JSON viewer
> APIs" in comment 14. comment 20 came later.

I'm sorry for not being more precise.

> > > [... can] 'make' the JSON viewer do one of the following things:
> > > - copy arbitrary strings to the clipboard
> > > - pop up a 'save file' dialog that saves arbitrary data.
> 
> That's it for "privileged APIs"? You could even call that sec-low, or
> sec-moderate if we want to add in some "worst case" worries about what else
> we might add to that context in the future.

I believe so, yes. https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/devtools/client/jsonview/converter-child.js#285 is the actually-privileged code running in response to the unprivileged messaging from the content. I think those are the only two things it accomplishes ("copy" and "copy-headers" having more or less the same effect when you're trying to abuse it from the content side in that they copy stuff to the clipboard). While content can, as comment #0 suggests, send any kind of plain object (needs to be serializable + deserializable because of compartments) it likes, I don't see any way (again, without further, somewhat unrelated exploits) to abuse that into anything other than what those 3 specific messages allow.

Comment 28

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

Comment on attachment 8829857 [details] [diff] [review]
Patch with comment

Clearing sec-approval request since it is a sec-moderate now.