751241 - Feedback on background worker code for social api

Status: RESOLVED WORKSFORME

(Firefox Graveyard :: SocialAPI, defect)

Description

[Sheila Mooney]

The innovation team is currently working on some social api features. They are looking for some early feedback on some background worker code particularly around use of MessagePorts and the cross-compartment Sandbox stuff.

https://github.com/mozilla/socialapi-dev/blob/chrome-toolbar-item/modules/frameworker.js

Comment 1

[Johnny Stenback (:jst)]

I think the best person to look over the worker ports emulation code here would be Ben. Ben, can you have a quick look at the emulated worker API here? The long term intent here is to move this code into an actual shared worker once we have support for them, so the API used here should be as close to the API we'll have there as we can realistically make it. And note that this will never be visible to actual webpages, only to social provider code, of which there's likely to be a very small number.

Bobby and/or Blake, could you look over the sandbox usage here and make sure it looks good? And again, this is never exposed to webpages, only to social provider code running with privileges of the domain they're served from.

Comment 2

[Sheila Mooney]

Hey, has anybody had a chance to look at this stuff yet? Any hope of getting that done in the near term since we were trying to maybe land something in Fx15.

Comment 3

[Bobby Holley (:bholley)]

My operating assumption here is that the code in the sandbox is untrusted.

        let sandbox = new Cu.Sandbox(workerWindow);
        // copy the window apis onto the sandbox namespace only functions or
        // objects that are naturally a part of an iframe, I'm assuming they are
        // safe to import this way
        let workerAPI = ['MozWebSocket', 'WebSocket', 'mozIndexedDB', 'localStorage',
                         'XMLHttpRequest',
                         'atob', 'btoa', 'clearInterval', 'clearTimeout', 'dump',
                         'setInterval', 'setTimeout',
                         'MozBlobBuilder', 'FileReader', 'Blob',
                         'navigator'];

I'm entirely unsure whether those constructors will be happy being instantiated in a different scope than the one in which they're defined. You should probably talk to someone who understands those implementations better.

Also, it seems safer to expose objects off of the workerWindow, rather than ones off the chrome window this script is running in.



        for each(let fn in workerAPI) {
          try {
            if (workerWindow[fn]) {
              sandbox.importFunction(workerWindow[fn], fn);
            }
          }

importFunction is no longer necessary. I just filed bug 756173 to update the docs.

        sandbox.importFunction(function importScripts(uris) {
          if (uris instanceof Array) {
            for each(let uri in uris) {
              Services.scriptloader.loadSubScript(uri, sandbox);
            }
          }
          else
            Services.scriptloader.loadSubScript(uris, sandbox);
        }, 'importScripts');
        // and we delegate ononline and onoffline events to the worker.
        // See http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#workerglobalscope
        frame.addEventListener('offline', function(event) {
          Cu.evalInSandbox("onoffline();", sandbox);
        }, false);
        frame.addEventListener('online', function(event) {
          Cu.evalInSandbox("ononline();", sandbox);
        }, false);

        sandbox.importFunction(workerWindow.postMessage, "_postMessage");
        sandbox.importFunction(workerWindow.addEventListener, "_addEventListener");

So, this _might_ work now due to some quirky behavior in XPConnect that we're getting rid of. But it shouldn't, in general. postMessage needs a real |this| binding, and AFAICT you don't have a reference to |workerWindow| within the sandbox. So you probably want to use Function.bind() here.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Bobby Holley (:bholley) from comment #3)
> My operating assumption here is that the code in the sandbox is untrusted.

Correct.

>         let sandbox = new Cu.Sandbox(workerWindow);
>         // copy the window apis onto the sandbox namespace only functions or
>         // objects that are naturally a part of an iframe, I'm assuming they
> are
>         // safe to import this way
>         let workerAPI = ['MozWebSocket', 'WebSocket', 'mozIndexedDB',
> 'localStorage',
>                          'XMLHttpRequest',
>                          'atob', 'btoa', 'clearInterval', 'clearTimeout',
> 'dump',
>                          'setInterval', 'setTimeout',
>                          'MozBlobBuilder', 'FileReader', 'Blob',
>                          'navigator'];
> 
> I'm entirely unsure whether those constructors will be happy being
> instantiated in a different scope than the one in which they're defined. You
> should probably talk to someone who understands those implementations better.


XMLHttpsRequest seems to be the only problem we're running into, but much of this hasn't been stressed yet.


> Also, it seems safer to expose objects off of the workerWindow, rather than
> ones off the chrome window this script is running in.


I'm a little confused, we are importing objects from workerWindow into the sandbox


> 
> 
>         for each(let fn in workerAPI) {
>           try {
>             if (workerWindow[fn]) {
>               sandbox.importFunction(workerWindow[fn], fn);
>             }
>           }
> 
> importFunction is no longer necessary. I just filed bug 756173 to update the
> docs.


That bug is my other bug related to this.  But what you are saying is that we can just do:  sandbox[fn] = workerWindow[fn] and it will be safe?  Will sandbox.importScripts = fucntion importScripts() { ... } also be safe?



>         sandbox.importFunction(workerWindow.postMessage, "_postMessage");
>         sandbox.importFunction(workerWindow.addEventListener,
> "_addEventListener");
> 
> So, this _might_ work now due to some quirky behavior in XPConnect that
> we're getting rid of. But it shouldn't, in general. postMessage needs a real
> |this| binding, and AFAICT you don't have a reference to |workerWindow|
> within the sandbox. So you probably want to use Function.bind() here.


noted, thanks!

Comment 5

[Bobby Holley (:bholley)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

> I'm a little confused, we are importing objects from workerWindow into the
> sandbox

Oh whoops, I misread and missed the workerWindow[fn] part. Ignore.

> That bug is my other bug related to this.  But what you are saying is that
> we can just do:  sandbox[fn] = workerWindow[fn] and it will be safe?  Will
> sandbox.importScripts = fucntion importScripts() { ... } also be safe?

As safe as the function you're calling. I doubt that anyone has ever given any thought to malicious URI parameters to loadSubScript, so you might want to look into that.

In general, it might be worth coordinating with seceng to get some fuzzing done for the sandbox. It's probably very easy to do (just take the existing fuzzers and clamp their vocabulary a bit), and it might yield interesting results. Fuzzing tends to be our best way to find out about security bugs. Me glancing at the code doesn't count. ;-)

Comment 6

[Sheila Mooney]

This bug has served it's purpose and any further review comments will live in bug 762569.