1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement)

Description

[Oriol Brufau [:Oriol]]

The JSON Viewer's converter-child reads data from the JSONView object exported to the page (without Xrays). I guess this could be bad if there was some XSS vulnerability, then JSONView could have been tampered with, and the privileged converter-child could malfunction (e.g. the request may not stop).

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

Review commit: https://reviewboard.mozilla.org/r/243380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243380/

Comment 2

[Oriol Brufau [:Oriol]]

This can be tested with some chunked JSON like in bug 1433655 comment 8, and running `JSONView.json = null` in the console before the page finishes loading.

Comment 3

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

Thanks for the patch Oriol!

I think this should be reviewed by someone with security experience.
@Gijs, could you please take a look at this? Or, is there anyone else we could ask?

Thanks!

Honza

Comment 4

[:Gijs (he/him)]

(In reply to Oriol Brufau [:Oriol] from comment #0)
> The JSON Viewer's converter-child reads data from the JSONView object
> exported to the page (without Xrays).

Can you clarify what you mean here? I'm not that familiar with this bit of the JSON viewer. Having read the code, off-hand it looks to me like the extant code calls Cu.cloneInto(), so the object that's returned is one that lives in the website compartment (not in the privileged one in which the converter-child code lives). Likewise, the text node that gets appended into the website's json content node also gets xray-wrapped, because it's a DOM node from a different compartment -- and nothing in the extant code calls waiveXrays().

The reason you can set `.json` to null is that the JSONView object lives in the content page, and so the content page can access and modify it. I *think* it couldn't replace it with e.g. an object that pretended to be a text node but that replaced some methods with malicious ones, ie those malicious methods wouldn't run with system privileges (I think).

I'm not really sure if what your patch is trying to do is just make it impossible to overwrite `JSONView.json` as far as the converter-child is concerned. Assuming that that is what you are trying to change, I'm confused about 2 things:

1) your patch makes the object exported into the page, and the one used internally, different objects. That seems fine. However, why do you need to change how we create the object in the content scope? Why not just create the object, and then Cu.cloneInto() it into the content scope, and return the original?
2) what we gain by making it impossible to modify the `json` property of the `JSONView` object, given that the text node (ie its value) becomes part of the webpage anyway.

That is, the text node gets appended into the website. Your attack model assumes that you can run JS code in the website's scope. In which case, because the text node that we reference is deliberately part of the website (so that the copy/save code can reference it), the JS could just clear or modify the contents of the text node, right? And that might still break logic in converter-child.js...

So, I'm confused why you explicitly need to waive xrays and need to do the whole Reflect dance to get this to work...

Can you clarify what you're trying to achieve a bit, and maybe point out what bit I'm not "getting" or misunderstanding?

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to :Gijs (he/him) from comment #4)
> Can you clarify what you mean here? I'm not that familiar with this bit of
> the JSON viewer. Having read the code, off-hand it looks to me like the
> extant code calls Cu.cloneInto(), so the object that's returned is one that
> lives in the website compartment (not in the privileged one in which the
> converter-child code lives). Likewise, the text node that gets appended into
> the website's json content node also gets xray-wrapped, because it's a DOM
> node from a different compartment -- and nothing in the extant code calls
> waiveXrays().

The extant code uses Cu.createObjectIn, this returns an object that lives in the website compartment without Xrays. Later, it reads the `json` property of that object, expecting to find the Text node that is set as the initial value. But this Text node could have been tampered with (it does not have Xrays because it's accessed via an object without Xrays), or even replaced with another thing. Just to test:

  let data = Cu.createObjectIn(win, {defineAs: "JSONView"});
  data.json = new win.Text();
  console.log(Cu.waiveXrays(data) === data,            // true
              Cu.waiveXrays(data.json) === data.json); // true

> The reason you can set `.json` to null is that the JSONView object lives in
> the content page, and so the content page can access and modify it. I
> *think* it couldn't replace it with e.g. an object that pretended to be a
> text node but that replaced some methods with malicious ones, ie those
> malicious methods wouldn't run with system privileges (I think).

The methods wouldn't run with system privileges, but they would run. They can for example throw an error and prevent the converter-child from stopping the request. The converter-child doesn't want them to run, it wants the native appendData instead.

> I'm not really sure if what your patch is trying to do is just make it
> impossible to overwrite `JSONView.json` as far as the converter-child is
> concerned. Assuming that that is what you are trying to change, I'm confused
> about 2 things:
> 
> 1) your patch makes the object exported into the page, and the one used
> internally, different objects. That seems fine. However, why do you need to
> change how we create the object in the content scope? Why not just create
> the object, and then Cu.cloneInto() it into the content scope, and return
> the original?

Do you mean something like this?

  win.JSONView = Cu.cloneInto(JSONView, win, {
    cloneFunctions: true,
    wrapReflectors: true,
  });

That won't work, `win` is protected by Xrays, so the website compartment won't be able to access JSONView.

It seems the property needs to be set to the waived window, like

  Cu.waiveXrays(win).JSONView = Cu.cloneInto(JSONView, win, {
    cloneFunctions: true,
    wrapReflectors: true,
  });

But this could run setters, I don't like it.
I could use Object.defineProperty to avoid setters, but it can throw in some cases.
So instead of adding a try...catch statement, I use Reflect.defineProperty, which in case of failure just returns false.
Then the webpage will fail to render the JSON without JSONView, but converter-child will continue working properly.
Sure, since JSONView is created just after starting the request, it probably won't happen that some XSS vulnerability is creates another non-configurable non-writable JSONView property beforehand. But just in case.

> 2) what we gain by making it impossible to modify the `json` property of the
> `JSONView` object, given that the text node (ie its value) becomes part of
> the webpage anyway.
> 
> That is, the text node gets appended into the website. Your attack model
> assumes that you can run JS code in the website's scope. In which case,
> because the text node that we reference is deliberately part of the website
> (so that the copy/save code can reference it), the JS could just clear or
> modify the contents of the text node, right? And that might still break
> logic in converter-child.js...

Yes, the logic in converter-child should not assume that the text contents won't be altered. This holds now, if the page changes them, the rendered json will be affected but converter-child will function properly.
The problem is that right now converter-child cannot even assume that the object is a text node nor that its appendData is the native one. I only want that, when converter-child does this.data.json.appendData(data), this should not call a strange function nor throw.

> So, I'm confused why you explicitly need to waive xrays and need to do the
> whole Reflect dance to get this to work...
> 
> Can you clarify what you're trying to achieve a bit, and maybe point out
> what bit I'm not "getting" or misunderstanding?

Without waiving Xrays the page won't see JSONView.
Without Reflect, the operation could throw, and this seemed simpler than adding a try...catch.

Comment 6

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

Comment on attachment 8974999 [details]
Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

https://reviewboard.mozilla.org/r/243380/#review250342

Just removing myself from the review.
Honza

Comment 7

[:Gijs (he/him)]

(In reply to Oriol Brufau [:Oriol] from comment #5)
> (In reply to :Gijs (he/him) from comment #4)
> > Can you clarify what you mean here? I'm not that familiar with this bit of
> > the JSON viewer. Having read the code, off-hand it looks to me like the
> > extant code calls Cu.cloneInto(), so the object that's returned is one that
> > lives in the website compartment (not in the privileged one in which the
> > converter-child code lives). Likewise, the text node that gets appended into
> > the website's json content node also gets xray-wrapped, because it's a DOM
> > node from a different compartment -- and nothing in the extant code calls
> > waiveXrays().
> 
> The extant code uses Cu.createObjectIn, this returns an object that lives in
> the website compartment without Xrays.

This is suprising, and ought to be documented on MDN (it's not, I checked). The DOM text node is created directly, so does that have xrays? (I realize that doesn't help if the property on `JSONView` gets overwritten, just checking my assumptions here)

> Later, it reads the `json` property
> of that object, expecting to find the Text node that is set as the initial
> value. But this Text node could have been tampered with (it does not have
> Xrays because it's accessed via an object without Xrays), or even replaced
> with another thing. Just to test:
> 
>   let data = Cu.createObjectIn(win, {defineAs: "JSONView"});
>   data.json = new win.Text();
>   console.log(Cu.waiveXrays(data) === data,            // true
>               Cu.waiveXrays(data.json) === data.json); // true
> 
> > The reason you can set `.json` to null is that the JSONView object lives in
> > the content page, and so the content page can access and modify it. I
> > *think* it couldn't replace it with e.g. an object that pretended to be a
> > text node but that replaced some methods with malicious ones, ie those
> > malicious methods wouldn't run with system privileges (I think).
> 
> The methods wouldn't run with system privileges, but they would run. They
> can for example throw an error and prevent the converter-child from stopping
> the request. The converter-child doesn't want them to run, it wants the
> native appendData instead.

Sure, but breaking the page is always going to be possible if you can run script on it... you could just remove the text node from the DOM, or do other nasty things. It's not a privilege escalation issue, which is normally what you care about when dealing with xrays...

> > I'm not really sure if what your patch is trying to do is just make it
> > impossible to overwrite `JSONView.json` as far as the converter-child is
> > concerned. Assuming that that is what you are trying to change, I'm confused
> > about 2 things:
> > 
> > 1) your patch makes the object exported into the page, and the one used
> > internally, different objects. That seems fine. However, why do you need to
> > change how we create the object in the content scope? Why not just create
> > the object, and then Cu.cloneInto() it into the content scope, and return
> > the original?
> 
> Do you mean something like this?
> 
>   win.JSONView = Cu.cloneInto(JSONView, win, {
>     cloneFunctions: true,
>     wrapReflectors: true,
>   });

Yes.

> That won't work, `win` is protected by Xrays, so the website compartment
> won't be able to access JSONView.

The docs say that it should work... Literally every example in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.cloneInto does this. Similar stuff with `exportFunction`.

So if it doesn't work in practice, maybe :mrbkap or :bz can help sort out what is necessary here (and then we can fix the docs?).

> It seems the property needs to be set to the waived window, like
> 
>   Cu.waiveXrays(win).JSONView = Cu.cloneInto(JSONView, win, {
>     cloneFunctions: true,
>     wrapReflectors: true,
>   });
> 
> But this could run setters, I don't like it.
> I could use Object.defineProperty to avoid setters, but it can throw in some
> cases.
> So instead of adding a try...catch statement, I use Reflect.defineProperty,
> which in case of failure just returns false.
> Then the webpage will fail to render the JSON without JSONView, but
> converter-child will continue working properly.
> Sure, since JSONView is created just after starting the request, it probably
> won't happen that some XSS vulnerability is creates another non-configurable
> non-writable JSONView property beforehand. But just in case.

I think the try...catch would be more explicit.

Comment 8

[Oriol Brufau [:Oriol]]

(In reply to :Gijs (he/him) from comment #7)
> This is suprising, and ought to be documented on MDN (it's not, I checked).
> The DOM text node is created directly, so does that have xrays? 

I tested these:

   var text = new win.Text();        // has xrays
   var obj = Cu.createObjectIn(win); // no xrays
   obj.text = text;
   obj.text;                         // no xrays
   Cu.unwaiveXrays(obj).text;        // has xrays

> Sure, but breaking the page is always going to be possible if you can run
> script on it... you could just remove the text node from the DOM, or do
> other nasty things. It's not a privilege escalation issue, which is normally
> what you care about when dealing with xrays...

Yes, the page can break. But converter-child is privileged, so it's potentially dangerous if it malfunctions. Calling functions that could have been altered and throw unexpectedly can cause this malfunction.

> The docs say that it should work... Literally every example in
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/
> Language_Bindings/Components.utils.cloneInto does this. Similar stuff with
> `exportFunction`.

Well, it seems to work with sandboxes but not with window objects:

  function test(global) {
    global.test = Cu.cloneInto({}, global);
    return !!global.eval("this.test");
  }
  test(Cu.Sandbox(null)); // true
  test(Services.appShell.createWindowlessBrowser().document.defaultView); // false

> I think the try...catch would be more explicit.

OK.

Comment 9

[Oriol Brufau [:Oriol]]

Comment on attachment 8974999 [details]
Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243380/diff/1-2/

Comment 10

[:Gijs (he/him)]

I'd like to update the reviewers on mozreview to include both me + mrbkap, but I can't because "Unable to update reviewers as the review request has pending changes (the patch author has a draft)" - can you publish or discard your draft, please? :-)

Comment 11

[Oriol Brufau [:Oriol]]

I tried to correct the review flag via mozreview but it didn't offer me to publish it, so I did it in bugzilla, but it seems the draft remained. Now I have been able to discard it.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8974999 [details]
Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

https://reviewboard.mozilla.org/r/243380/#review250544

Kris, can you doublecheck this looks good to you? Looks like bholley and mrbkap are both out...

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8974999 [details]
Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

https://reviewboard.mozilla.org/r/243380/#review250552

::: devtools/client/jsonview/converter-child.js:199
(Diff revision 2)
>  function exportData(win, headers) {
> -  let data = Cu.createObjectIn(win, {
> -    defineAs: "JSONView"
> -  });
> -
> -  data.debugJsModules = debugJsModules;
> +  let JSONView = {
> +    debugJsModules,
> +    headers,
> +    json: new win.Text(),
> +    readyState: "uninitialized",

It's kind of weird to have properties like this that are updated by content on an object that we save but do not keep updated.

It would probably make more sense to do something like:

    let json = new win.Text();
    let JSONView = Cu.cloneInto({
      json,
      ...
    });
    ...
    return {json};

::: devtools/client/jsonview/converter-child.js:216
(Diff revision 2)
> -  data.Locale = Cu.cloneInto(Locale, win, {cloneFunctions: true});
> -
> -  data.headers = Cu.cloneInto(headers, win);
> -
> -  return data;
> +  let clone = Cu.cloneInto(JSONView, win, {
> +    cloneFunctions: true,
> +    wrapReflectors: true,
> +  });
> +  try {
> +    Object.defineProperty(Cu.waiveXrays(win), "JSONView", {value: clone});

Not that it matters much, but this should probably be writable/enumerable/configurable.

::: devtools/client/jsonview/converter-child.js:217
(Diff revision 2)
> -
> -  data.headers = Cu.cloneInto(headers, win);
> -
> -  return data;
> +    cloneFunctions: true,
> +    wrapReflectors: true,
> +  });
> +  try {
> +    Object.defineProperty(Cu.waiveXrays(win), "JSONView", {value: clone});
> +  } catch (error) {}

You should probably use Cu.reportError or console.error here.

Comment 14

[Oriol Brufau [:Oriol]]

Comment on attachment 8974999 [details]
Bug 1460840 - JSON Viewer's converter-child should not trust JSONView without Xrays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243380/diff/2-3/

Comment 15

[Oriol Brufau [:Oriol]]

OK, done!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc02bb960616
JSON Viewer's converter-child should not trust JSONView without Xrays r=Gijs,kmag

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fc02bb960616