714891 - We should only pass strings to and from sandboxes

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Eddy Bruel [:ejpbruel]]

Due to a recent change in Firefox (introduced by bug 690825), it is no longer possible to detect the type of an object that was created by a script with chrome privileges from a sandbox running a script with content privileges. Among other things, this breaks JSON serialization (see bug 714110 for an example).

The solution for this problem, as suggested by tLt, is to 'just pass them as strings goddamnit'. Passing objects from and to sandboxes is inherently insecure, and despite our best efforts with wrappers so far, it's not inconceivable that we'll end up with a scheme in which objects are not allowed to be passed to and from sandboxes at all. Since we can cover all our use cases for the add-on SDK by serializing objects to strings, this seems like a safe bet.

Comment 1

[Dave Townsend [:mossop]]

Alex, this sounds likely to be on your shoulders, let us know if that is a problem

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Use document's JSON.parse

Here is the simpliest patch we can do in order to fix initial issue.
I'm currently working in parallel on a better patch that split api-utils/lib/content/worker.js in two: 
- one part that lives in chrome sandboxes, and,
- a new second part that lives in content sandbox 
So that all exposed functions/objects (self, self.port, self.postMessage, ...) will live in content sandbox and won't be wrapped anymore!
This is the only way to fix this issue in a non-hacky way and to ensure that nothing else but strings are shared between sandboxes.
This work will address this issue too:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/worker.js#L309-319

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 585781 [details] [diff] [review]
Use document's JSON.parse

myk: What do you think about such workaround for 1.4? 
I tried to find something simple that modify only faulty code.
I'm quite scared to land the big worker.js split in such timeframe.
Having said that, I should have such patch ready for tomorrow.

Comment 4

[David Mason [:dcm :dmason]]

That sounds like a good way to approach this Alex - we can make sure we support Fx 10 and not have to land a scary change with 1 week left on stabilization branch.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 585781 [details] [diff] [review]
Use document's JSON.parse

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

Landing the lowest-risk fix for 1.4 seems like the right strategy to me.

As part of the more complete fix for 1.5, can you fix the typo on the first line of this file?  We should not do this for 1.4, even though the change seems trivial and innocent, to reduce the risk of the 1.4 fix to the absolute minimum.

Overall, this looks good and demonstrably fixes the problem, except for the case that is not being tested, which is the only reason for the r-.  Everything else is just a nit.

::: packages/api-utils/lib/content/worker.js
@@ +501,5 @@
>      
>      let scope = this._contentWorker._port;
>      // Ensure that we pass only JSON values
> +    let array = Array.prototype.slice.call(args);
> +    scope._asyncEmit.apply(scope, ensureArgumentsAreJSON(array, this._window));

This change fixes the problem for values passed via emit(), but the change is not being tested.  It should be tested in *test:emit-json-values-only*.

::: packages/api-utils/tests/test-content-worker.js
@@ +184,5 @@
> +      test.assertEqual(message[0], true, "function becomes undefined");
> +      test.assertEqual(message[1], "object", "object stays object");
> +      test.assertEqual(message[2], true, "object's attributes are enumerable");
> +      test.assertEqual(message[3], "about:blank", "jsonable attributes are accessible");
> +      // See bug 714891, Arrays may be broken over compartements:

Nit: compartements -> compartments

@@ +185,5 @@
> +      test.assertEqual(message[1], "object", "object stays object");
> +      test.assertEqual(message[2], true, "object's attributes are enumerable");
> +      test.assertEqual(message[3], "about:blank", "jsonable attributes are accessible");
> +      // See bug 714891, Arrays may be broken over compartements:
> +      test.assertEqual(message[4], true, "Array keeps being an array");

Nit: `test.assertEqual(something, true` would be better as `test.assert(something`.

@@ +186,5 @@
> +      test.assertEqual(message[2], true, "object's attributes are enumerable");
> +      test.assertEqual(message[3], "about:blank", "jsonable attributes are accessible");
> +      // See bug 714891, Arrays may be broken over compartements:
> +      test.assertEqual(message[4], true, "Array keeps being an array");
> +      test.assertEqual(message[5], "[1,2,3]", "Array is correctly serialized");

Nit: this is brittle, as changes to the way JSON is serialized will break it (f.e. if the JSON serializer started serializing it to "[1, 2, 3]"); it'd be better to serialize it, reparse it, and check that it matches an equivalent array on the other side.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Address comments

So I modified "test:emit-json-values-only" in order to check compartments issues too. (We need to load a content document, that's why I'm loading "data:text/html,". Otherwise `window` refers to the toplevel xul document)


> +      test.assertEqual(message[5], "[1,2,3]", "Array is correctly serialized");

As it is not really easy to compare arrays, I compare JSON.stringify results from both sandboxes:

> +       test.assertEqual(message[5], JSON.stringify(array),
                       "Array is correctly serialized");

Comment 7

[Alex Keybl [:akeybl]]

[Triage Comment]
Tracking for Firefox 10/11 instead of bug 714110

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 585827 [details] [diff] [review]
Address comments

Looks good, r=myk!

I will land this now and then cherry-pick it to the stabilization branch so I can spin 1.4rc1.

Alex: do you want to leave this bug open for the more robust fix, or would you like to close it and track that work elsewhere?

Comment 9

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/166174ff64fd3b3e52a3c0b6cf98b7b9e5f9cbcb
bug 714891: We should only pass strings to and from sandboxes; r=@mykmelez

Comment 10

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5f28e7fdb77f00d919a47e1aabd9952007c5062d
bug 714891: We should only pass strings to and from sandboxes; r=@mykmelez

Comment 11

[David Mason [:dcm :dmason]]

Removed block on release bug (714820) but left open for Alex to continue working on

Comment 12

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5f28e7fdb77f00d919a47e1aabd9952007c5062d
bug 714891: We should only pass strings to and from sandboxes; r=@mykmelez

Comment 13

[Alexandre Poirot [:ochameau]]

I opened bug 718666, for the proper fix that will ensure sharing only strings ... and 2 functions between chrome and content compartments.

Comment 14

[Alex Keybl [:akeybl]]

We shipped Firefox 10 with this regression and have yet to see any fallout. Untracking for FF11.