1274708 - Work out a safe solution for JSON.stringify() in chrome context on data from extension context

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Andrew Swan [:aswan]]

This came up as part of bug 1270357.  In short, extensions pass a message to `postMessage()` on a native messaging Port which is serialized with JSON.stringify() and then written to a pipe to the native application.
:kmag pointed out during review that calling JSON.stringify() from the implementation of postMessage() isn't right if the message has a toJSON() method since that method will be hidden by cross-compartment wrappers.  The first proposed solution was adding a new xpcom utility and we have a working implementation of that idea in bug 1274386.  There was some concern that this was overly specialized and that the same effect could be achieved using some combination of sandboxes and waived xray wrappers.

Creating a new sandbox with the same principal as the extension and doing JSON.stringify() in the sandbox doesn't appear to work unless we also waive xrays on the message object, but :kmag objected to that since it could give the extension access to privileged objects that it should not have access to.

So, I haven't been able to come up with a good solution and I think my lack of depth here is getting in the way, I'm hoping that :kmag and :bholley can have a high-bandwidth discussion here without me in the middle to reach consensus on what we should do.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Andrew Swan [:aswan] from comment #0)
> This came up as part of bug 1270357.  In short, extensions pass a message to
> `postMessage()` on a native messaging Port which is serialized with
> JSON.stringify() and then written to a pipe to the native application.
> :kmag pointed out during review that calling JSON.stringify() from the
> implementation of postMessage() isn't right if the message has a toJSON()
> method since that method will be hidden by cross-compartment wrappers.  The
> first proposed solution was adding a new xpcom utility and we have a working
> implementation of that idea in bug 1274386.  There was some concern that
> this was overly specialized and that the same effect could be achieved using
> some combination of sandboxes and waived xray wrappers.
> 
> Creating a new sandbox with the same principal as the extension and doing
> JSON.stringify() in the sandbox doesn't appear to work unless we also waive
> xrays on the message object, but :kmag objected to that since it could give
> the extension access to privileged objects that it should not have access to.

I don't understand this concern. Can you elaborate kmag?

> 
> So, I haven't been able to come up with a good solution and I think my lack
> of depth here is getting in the way, I'm hoping that :kmag and :bholley can
> have a high-bandwidth discussion here without me in the middle to reach
> consensus on what we should do.

My (much simpler) proposal is just to waive Xrays on the object and call the local JSON.stringify on the result. Does that give us what we want?

Comment 2

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (busy) from comment #1)
> (In reply to Andrew Swan [:aswan] from comment #0)
> > Creating a new sandbox with the same principal as the extension and doing
> > JSON.stringify() in the sandbox doesn't appear to work unless we also waive
> > xrays on the message object, but :kmag objected to that since it could give
> > the extension access to privileged objects that it should not have access to.
> 
> I don't understand this concern. Can you elaborate kmag?

That wasn't exactly my concern. My concern was that if the extension tried to
send an X-ray wrapped object it does have access to, those X-rays would be
waived as well.

I can't think of a way this would happen outside of a content script (which
doesn't support this API at the moment, but does support the storage API,
which has similar problems), but in that case it could wind up stringifying an
object from the content compartment that should have security wrappers, but
suddenly doesn't.

> My (much simpler) proposal is just to waive Xrays on the object and call the
> local JSON.stringify on the result. Does that give us what we want?

It doesn't, because that would stringify the object with full chrome
privileges, and give the add-no read access to things it shouldn't have access
to.

The simplest example I can think of is something like:

  port.postMessage({window: iframe.contentWindow});

which would be safe with an X-ray wrapper, because the `window` property would
be skipped, but wouldn't be safe with them waived. It would still fail because
of the cyclic object references, but I'm sure someone could find a way to
exploit it.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #2)
> (In reply to Bobby Holley (busy) from comment #1)
> > (In reply to Andrew Swan [:aswan] from comment #0)
> > > Creating a new sandbox with the same principal as the extension and doing
> > > JSON.stringify() in the sandbox doesn't appear to work unless we also waive
> > > xrays on the message object, but :kmag objected to that since it could give
> > > the extension access to privileged objects that it should not have access to.
> > 
> > I don't understand this concern. Can you elaborate kmag?
> 
> That wasn't exactly my concern. My concern was that if the extension tried to
> send an X-ray wrapped object it does have access to, those X-rays would be
> waived as well.
> 
> I can't think of a way this would happen outside of a content script (which
> doesn't support this API at the moment, but does support the storage API,
> which has similar problems), but in that case it could wind up stringifying
> an
> object from the content compartment that should have security wrappers, but
> suddenly doesn't.

If you give the Sandbox the same principal as the content, then it sees things exactly as content does. Assuming you make sure only to pull strings out of the sandbox, there doesn't seem to be significant risk there.

The alternative is just to do:

var stringify = sb.eval('function(arg) { return JSON.stringify(arg.wrappedJSObject); }');

Which removes the need to pass wantXrays: false.

I really, really wish we didn't have that stupid double-behavior for wantXrays. I'd take a patch that added a new unencumbered option called wantSameOriginXrays that does what you expect without the surprising waive-for-caller behavior.

> 
> > My (much simpler) proposal is just to waive Xrays on the object and call the
> > local JSON.stringify on the result. Does that give us what we want?
> 
> It doesn't, because that would stringify the object with full chrome
> privileges, and give the add-no read access to things it shouldn't have
> access
> to.

Ah yes, that's a great point - exactly the things we protect against in Xrays and that we can't protect against with waivers.

So yeah, I'm fine with either doing it with sandboxes or adding a bespoke Cu API.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (busy) from comment #3)
> If you give the Sandbox the same principal as the content, then it sees
> things exactly as content does. Assuming you make sure only to pull strings
> out of the sandbox, there doesn't seem to be significant risk there.
> 
> I really, really wish we didn't have that stupid double-behavior for
> wantXrays. I'd take a patch that added a new unencumbered option called
> wantSameOriginXrays that does what you expect without the surprising
> waive-for-caller behavior.

We give the sandboxes an expanded principal the extends the content principal
so that the scripts can do things like cross-origin XHRs, and so content can't
access any objects it leaks to it.

wantSameOriginXrays sounds like a good idea, though. I'll look into
implementing it.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #4)
> (In reply to Bobby Holley (busy) from comment #3)
> > If you give the Sandbox the same principal as the content, then it sees
> > things exactly as content does. Assuming you make sure only to pull strings
> > out of the sandbox, there doesn't seem to be significant risk there.
> > 
> > I really, really wish we didn't have that stupid double-behavior for
> > wantXrays. I'd take a patch that added a new unencumbered option called
> > wantSameOriginXrays that does what you expect without the surprising
> > waive-for-caller behavior.
> 
> We give the sandboxes an expanded principal the extends the content principal
> so that the scripts can do things like cross-origin XHRs, and so content
> can't
> access any objects it leaks to it.

Oh, ok. In that case wantXrays will have no effect, and you'll need to take the scripted approach from comment 3.

> 
> wantSameOriginXrays sounds like a good idea, though. I'll look into
> implementing it.

Great!

Comment 6

[Andrew Swan [:aswan]]

Attachment: Bug 1274708 Add BaseContext.jsonStringify()

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

Comment 7

[Andrew Swan [:aswan]]

Attachment: Bug 1274708 Use Context.jsonStringify() in connectNative

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

Comment 8

[Andrew Swan [:aswan]]

Attachment: Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8762711 [details]
Bug 1274708 Add BaseContext.jsonStringify()

https://reviewboard.mozilla.org/r/59222/#review56318

::: toolkit/components/extensions/ExtensionUtils.jsm:209
(Diff revision 1)
> +   * @param {array<any>} args Arguments for JSON.stringify()
> +   * @returns {string} The stringified representation of obj
> +   */
> +  jsonStringify(...args) {
> +    if (!this.jsonSandbox) {
> +      this.jsonSandbox = new Cu.Sandbox(this.principal, {

No `new`

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:135
(Diff revision 1)
> +
> +add_task(function* test_json_stringify() {
> +  class Context extends BaseContext {
> +    constructor(principal) {
> +      super();
> +      this._principal = principal;

`Object.defineProperty(this, "principal", {value: principal, configurable: true})`

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:136
(Diff revision 1)
> +add_task(function* test_json_stringify() {
> +  class Context extends BaseContext {
> +    constructor(principal) {
> +      super();
> +      this._principal = principal;
> +      this.sandbox = new Cu.Sandbox(principal, {wantXrays: false});

No `new`.

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:148
(Diff revision 1)
> +    get principal() {
> +      return this._principal;
> +    }
> +
> +    get extension() {
> +      return {id: "[e-mail]"};

This doesn't need to be a getter.

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:158
(Diff revision 1)
> +  const PRINCIPAL1 = ssm.createCodebasePrincipalFromOrigin("http://www.example.org");
> +  const PRINCIPAL2 = ssm.createCodebasePrincipalFromOrigin("http://www.somethingelse.org");
> +
> +  // Test that toJSON() works in the json sandbox
> +  let context = new Context(PRINCIPAL1);
> +  let sandbox = new Cu.Sandbox(PRINCIPAL1);

No `new`

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:159
(Diff revision 1)
> +  const PRINCIPAL2 = ssm.createCodebasePrincipalFromOrigin("http://www.somethingelse.org");
> +
> +  // Test that toJSON() works in the json sandbox
> +  let context = new Context(PRINCIPAL1);
> +  let sandbox = new Cu.Sandbox(PRINCIPAL1);
> +  let obj = Cu.evalInSandbox("let obj = {hidden: true, toJSON() { return {visible: true}; } }; obj", context.sandbox);

let obj = Cu.evalInSandbox("({hidden: true, toJSON() { return {visible: true}; }})", context.sandbox);

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:165
(Diff revision 1)
> +
> +  let stringified = context.jsonStringify(obj);
> +  let expected = JSON.stringify({visible: true});
> +  equal(stringified, expected, "Stringified object with toJSON() method is as expected");
> +
> +  // Test that an inaccessible property from another global is not included

Can you make a separate task for each of these?

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:167
(Diff revision 1)
> +  let expected = JSON.stringify({visible: true});
> +  equal(stringified, expected, "Stringified object with toJSON() method is as expected");
> +
> +  // Test that an inaccessible property from another global is not included
> +  let sandbox2 = new Cu.Sandbox(PRINCIPAL2);
> +  sandbox.subobj = Cu.evalInSandbox("let o = { subobject: true }; o", sandbox2);

Cu.waiveXrays(sandbox).subobj = Cu.evalInSandbox("({ subobject: true })", sandbox2);

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:169
(Diff revision 1)
> +
> +  // Test that an inaccessible property from another global is not included
> +  let sandbox2 = new Cu.Sandbox(PRINCIPAL2);
> +  sandbox.subobj = Cu.evalInSandbox("let o = { subobject: true }; o", sandbox2);
> +
> +  obj = Cu.evalInSandbox("let o = { local: true, nested: subobj }; o", sandbox);

obj = Cu.evalInSandbox("({ local: true, nested: subobj })", sandbox);

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:178
(Diff revision 1)
> +  equal(stringified, expected, "Stringified object with inaccessible property is as expected");
> +
> +  // Test that an accessible property from another global is included
> +  let principal = ssm.createExpandedPrincipal([PRINCIPAL1, PRINCIPAL2]);
> +  context = new Context(principal);
> +  sandbox = new Cu.Sandbox(principal);

No `new`.

Also, why not `context.sandbox`?

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:181
(Diff revision 1)
> +  let principal = ssm.createExpandedPrincipal([PRINCIPAL1, PRINCIPAL2]);
> +  context = new Context(principal);
> +  sandbox = new Cu.Sandbox(principal);
> +
> +  sandbox.subobj = Cu.evalInSandbox("o", sandbox2);
> +  obj = Cu.evalInSandbox("let o = { local: true, nested: subobj }; o", sandbox);

obj = Cu.evalInSandbox("({ local: true, nested: subobj })", sandbox);

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8762713 [details]
Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

https://reviewboard.mozilla.org/r/59226/#review56322

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8762712 [details]
Bug 1274708 Use Context.jsonStringify() in connectNative

https://reviewboard.mozilla.org/r/59224/#review56324

Comment 12

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59222/#review56318

> No `new`

Okay, do you want me to also change the code at the top of this file that this was based on?

> let obj = Cu.evalInSandbox("({hidden: true, toJSON() { return {visible: true}; }})", context.sandbox);

Ah, that works.  I had tried that originally without the parentheses and it didn't work.  I still don't really understand why?

> Cu.waiveXrays(sandbox).subobj = Cu.evalInSandbox("({ subobject: true })", sandbox2);

I don't understand the need for the waiveXrays here?

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8762711 [details]
Bug 1274708 Add BaseContext.jsonStringify()

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

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8762712 [details]
Bug 1274708 Use Context.jsonStringify() in connectNative

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

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8762713 [details]
Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

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

Comment 16

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59222/#review56318

> Okay, do you want me to also change the code at the top of this file that this was based on?

Yeah, that would be good.

> Ah, that works.  I had tried that originally without the parentheses and it didn't work.  I still don't really understand why?

Without the parentheses, you get a block statement rather than an object literal expression, so there's a syntax error.

> I don't understand the need for the waiveXrays here?

It's not necessary at the moment, since sandboxes currently automatically waive X-rays. But that may change, so it's good to have it as a defense mechanism.

Comment 17

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59222/#review56490

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:168
(Diff revisions 1 - 2)
> -  sandbox.subobj = Cu.evalInSandbox("let o = { subobject: true }; o", sandbox2);
> +  let context = new Context(PRINCIPAL1);
> +  let sandbox = Cu.Sandbox(PRINCIPAL1);
> +  let sandbox2 = Cu.Sandbox(PRINCIPAL2);
>  
> -  obj = Cu.evalInSandbox("let o = { local: true, nested: subobj }; o", sandbox);
> -  stringified = context.jsonStringify(obj);
> +  sandbox.subobj = Cu.evalInSandbox("({ subobject: true })", sandbox2);
> +  let obj = Cu.evalInSandbox("({ local: true, nested: subobj })", sandbox);

This should be `context.sandbox` rather than a separate sandbox.

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:183
(Diff revisions 1 - 2)
> -  context = new Context(principal);
> -  sandbox = new Cu.Sandbox(principal);
> +  let context = new Context(principal);
> +  let sandbox = Cu.Sandbox(principal);
> +  let sandbox2 = Cu.Sandbox(PRINCIPAL2);
>  
> -  sandbox.subobj = Cu.evalInSandbox("o", sandbox2);
> -  obj = Cu.evalInSandbox("let o = { local: true, nested: subobj }; o", sandbox);
> +  sandbox.subobj = Cu.evalInSandbox("({ subobject: true })", sandbox2);
> +  let obj = Cu.evalInSandbox("({ local: true, nested: subobj })", sandbox);

Same here. Should be `context.sandbox` rather than a separate sandbox.

Comment 18

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59222/#review56318

> Without the parentheses, you get a block statement rather than an object literal expression, so there's a syntax error.

Well yes, I understand that much, I don't understand why it isn't parsed as an object literal.  perhaps a naive comparison but if i type `{visible: true}` at the node repl, it is parsed as an object literal.
In any case, it is fixed in this patch.

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59222/#review56318

> Well yes, I understand that much, I don't understand why it isn't parsed as an object literal.  perhaps a naive comparison but if i type `{visible: true}` at the node repl, it is parsed as an object literal.
> In any case, it is fixed in this patch.

Because `{` only begins an object literal in expression contexts, not in statement contexts[1]. I imagine that it works in the Node REPL because it has a special case for it. It doesn't work in the Spidermonkey REPL.

[1]: https://tc39.github.io/ecma262/#prod-ExpressionStatement

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8762711 [details]
Bug 1274708 Add BaseContext.jsonStringify()

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

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8762712 [details]
Bug 1274708 Use Context.jsonStringify() in connectNative

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

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8762713 [details]
Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

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

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8762711 [details]
Bug 1274708 Add BaseContext.jsonStringify()

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

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8762712 [details]
Bug 1274708 Use Context.jsonStringify() in connectNative

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

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8762713 [details]
Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

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

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8762712 [details]
Bug 1274708 Use Context.jsonStringify() in connectNative

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59224/diff/4-5/

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8762713 [details]
Bug 1274708 Use Context.jsonStringify() in ExtensionStorage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59226/diff/4-5/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a836e13dd4d0
Add BaseContext.jsonStringify() r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee12ef47112
Use Context.jsonStringify() in connectNative r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2062016341
Use Context.jsonStringify() in ExtensionStorage r=kmag

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a836e13dd4d0
https://hg.mozilla.org/mozilla-central/rev/fee12ef47112
https://hg.mozilla.org/mozilla-central/rev/4c2062016341