Web Workers not stripped of communication ability

[willscott]

Per UWNetworksLab/uProxy-p2p#365

the code that needs to be added is probably at both:
https://github.com/freedomjs/freedom/blob/v0.6/src/moduleinternal.js#L57
for making sure the global object is overall cleaned
and
https://github.com/freedomjs/freedom/blob/v0.6/src/link/worker.js#L55
for webworker specific cleansing.

[willscott]

More generally, we don't have a standardized set of functions and interfaces available within modules. This bug can track creation of that exported interface.

[willscott]

The globals in Chrome webworkers are:

Infinity
Array
ArrayBuffer
Blob
Boolean
DataView
Date
Error
EvalError
EventSource
File
FileError
FileList
FileReader
FileReaderSync
Float32Array
Float64Array
FormData
Function
IDBCursor
IDBCursorWithValue
IDBDatabase
IDBFactory
IDBIndex
IDBKeyRange
IDBObjectStore
IDBOpenDBRequest
IDBRequest
IDBTransation
IDBVersionChangeEvent
ImageData
Int8Array
Int16Array
Int32Array
Intl
JSON
Map
Math
MessageChannel
MessageEvent
NaN
Number
Object
Promise
RangeError
RegExp
Set
String
Symbol
SyntaxError
TextDecoder
TextEncoder
URIError
URL
Uint8Array
Uint8ClampedArray
Uint16Array
Uint32Array
WeakMap
WeakSet
WebSocket
WorkerLocation
WorkerNavigator
XMLHttpRequest
console
crypto
decodeURI
decodeURIComponent
encodeURI
encodeURIComponent
escape
eval
indexDB
isFinite
isNaN
location
navigator
onerror
onmessage
parseFloat
parseInt
performance
self
undefined
unescape
webkitIDB*
webkitIndexDB
webkitURL

[willscott]

Many of these properties can't be simply removed. The procedure for obscuring properties we don't want should be:

Object.defineProperty(this, 'WebSocket', {value:undefined});
...
Object.freeze(this);

[bemasc]

Could freedom have a runtime whitelist that eliminates any unexpected attributes of this before loading each module?

[willscott]

That's the plan. The question we haven't fully thought through yet is what is on that whitelist.

[agallant]

EDIT - switching to spreadsheet for planning whitelist. https://docs.google.com/a/cs.washington.edu/spreadsheets/d/1kVAtIPgAeFBRtt7JbgTegT7nYg57JPl63Lt3S0Ac794/edit#gid=0

[agallant]

And just to set the stage - I think there are two conceptual questions that should guide our choices.
-How much does the availability of these globals affect the sorts of apps people can write using freedom?
-Relatedly, is the burden to get on the whitelist "necessary", "useful", or just "not harmful"?

My impression of the first question is that this may have at least some long-term ramifications for further freedom usage, so my tentative answer to the second is to set a burden at "potentially useful" (i.e. lower than "necessary", higher than "not harmful"). Thoughts and corrections from others are welcome.

[willscott]

I guess my response is that for any thing in the context the two things we
want to be comfortable with are:

1. that it's something that we can plausible provide in all platforms
   (node, firefox, chrome)
2. it's something that isn't going to weaken the security properties of
   modules. That would be either if there's a way to use it to communicate to
   an external server without using freedom APIs or being able to store data
   /communicate with other modules not using firefox, or extend its lifetime
   beyond that of a normal module.

On Wed Oct 22 2014 at 4:59:57 PM soycode [email protected] wrote:

And just to set the stage - I think there are two conceptual questions
that should guide our choices.
-How much does the availability of these globals affect the sorts of apps
people can write using freedom?
-Relatedly, is the burden to get on the whitelist "necessary", "useful",
or just "not harmful"?

My impression of the first question is that this may have at least some
long-term ramifications for further freedom usage, so my tentative answer
to the second is to set a burden at "potentially useful" (i.e. lower than
"necessary", higher than "not harmful"). Thoughts and corrections from
others are welcome.

—
Reply to this email directly or view it on GitHub
#110 (comment).

[ryscheng]

agreed

On Wed, Oct 22, 2014 at 5:05 PM, Will [email protected] wrote:

I guess my response is that for any thing in the context the two things we
want to be comfortable with are:

1. that it's something that we can plausible provide in all platforms
   (node, firefox, chrome)
2. it's something that isn't going to weaken the security properties of
   modules. That would be either if there's a way to use it to communicate to
   an external server without using freedom APIs or being able to store data
   /communicate with other modules not using firefox, or extend its lifetime
   beyond that of a normal module.

On Wed Oct 22 2014 at 4:59:57 PM soycode [email protected]
wrote:

And just to set the stage - I think there are two conceptual questions
that should guide our choices.
-How much does the availability of these globals affect the sorts of
apps
people can write using freedom?
-Relatedly, is the burden to get on the whitelist "necessary", "useful",
or just "not harmful"?

My impression of the first question is that this may have at least some
long-term ramifications for further freedom usage, so my tentative
answer
to the second is to set a burden at "potentially useful" (i.e. lower
than
"necessary", higher than "not harmful"). Thoughts and corrections from
others are welcome.

—
Reply to this email directly or view it on GitHub
#110 (comment).

—
Reply to this email directly or view it on GitHub
#110 (comment).

[agallant]

Re: platforms, here's some info for Firefox: https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers

Haven't found as good a source for node yet.

EDIT - also I believe the FF documentation implies workers have all of what they consider standard JS globals. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects?redirectlocale=en-US&redirectslug=JavaScript%2FReference%2FGlobal_Objects

[agallant]

Spreadsheet updated with at-least-tentative yes/no for everything, as well as general presence in Chrome/Firefox/Node (if something was clearly a no I didn't always check all three). Feedback welcome.

[ryscheng]

awesome job on the spreadsheet! this pretty much wipes out most things we wanted to wipe out anyway. I left some comments but largely agree with your assessment.

[agallant]

So here's an initial commit w/tentative whitelist so far:
01c48e7

I ended up having to use Object.getOwnPropertyNames, as apparently most all of the properties in the webworker global scope object aren't enumerable (so e.g. "for...in" doesn't work). This code successfully goes through all the globals in the webworker scope, and logs the ones not on the whitelist.

However, using the Object.defineProperty to mask them causes silent failures. On the first value it wants to mask it ends up stopping the loop and breaking execution entirely. Additionally, the Object.freeze, while it doesn't stop unit tests from passing, does break the demos. One possible avenue is that maybe some of this (like the freeze) should go in setupWorker rather than setupListener. But I'm not sure what to do differently about the masking - as a blind guess I replaced {value:undefined} with plain old undefined, but that lead to similar issues.

Any thoughts are welcome, though I'll continue investigating/experimenting too.

[agallant]

Figured it out: 3b29ce8

Brief takeaways:
-These globals are not considered enumerable properties hence the getOwnPropertyNames
-this.obj has the property names, but this is what needs to be masked/frozen
-Masking Intl breaks everything, so I whitelisted it - it's used for internationalization but is not present in base node, we'll need to polyfill: https://www.npmjs.org/package/intl

I've tested this on Chrome and it does indeed mask XMLHttpRequest and other things in the webworker scope while still passing tests and letting e.g. the demo counter application run. I'm less certain about what needs to be added to moduleinternal, what scope/object besides the webworker globals needs cleaning?

[willscott]

We need to wait on any actual enforcement of this on #136

[willscott]

We just noticed in freedomjs/freedom-for-firefox#33 that firefox in an addon has different exposed globals than firefox in a webpage

[agallant]

#136 is resolved so revisiting this.

[agallant]

Went back to my masking branch and merged up to the current state of freedom. Things seem to be mostly working (freedom demo apps run and unit tests pass with masking) with two caveats/todos:

-Debugging is now more painful because when you open Chrome console and try to switch to the freedom.js worker you get "Uncaught TypeError: Can't add property __commandLineAPI, object is not extensible" - in other words, Chrome seems to need to edit global scope at runtime to inspect/interact with webworker threads. If you don't use the console on the webworker thread then the error doesn't occur and the app runs fine, but as I said it makes debugging painful and I can't figure a good way to whitelist just this mutability without letting in the rest.

-A decent slate of integration tests still fail, mostly because they do things like create a temporary global variable to keep track of some test state. I'm hoping they're mostly refactorable in some sensible way and will push a few commits to that end in the near future, but we may have to add some more helper code/framework around integration testing at some point.

[agallant]

Spent some time debugging the loads-scripts-sequentially module test, and discovered that actually I still wasn't freezing the object at the right spot (it was freezing after each loaded script rather than all of them). Fixed that (8641b91) and now all integration tests pass (at least the first time I ran - on further runs one transport.webrtc.json test seems flaky but I don't think it's related).

The Chrome console issue is still a factor, and basically it's a problem because even just adding that property to the whitelist doesn't help (since the property isn't added to the object until after freedom is completely done loading). Ideally we'd say "let the browser mutate this one property but no others", but I don't think JavaScript provides that level of control here - so we may just want to add a "debug/dev" mode that allows for mutation in general. I'll look into a sensible way of doing that.

[agallant]

Status - what I want to do is add an e.g. if (!config.freezeScope)... around https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/moduleinternal.js#L367 with the freezeScope parameter (default false) passed in optionally along with the manifest. Such parameters should get to moduleinternal (the webworker scope) via message.config at https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/moduleinternal.js#L47 - however, after stepping through and logging a bunch it seems like what actually makes it that far is site_cfg which is set up at https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/entry.js#L47 and does not include all the "external" config parameters.

The rub seems to be that the setup function which makes site_cfg does receive the external config, but only when it is making the site_cfg for the external scope. When it's setting up the webworker scope it's already lacking that parameter and so doesn't have the information to stick in site_cfg and pass on.

Anyway, I'll step through it more later but for now I wanted to (a) document my investigation for my own benefit at least, and (b) let you know in case I'm missing some more obvious way to pass config parameters to the webworker. site_cfg does indeed work and pass some parameters in, but it seems like it can't get them from the outermost scope, which is what I want.

[willscott]

At
https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/entry.js#L62
just after the definition you link, site_cfg should get the additional
properties set by the page config added.
On Thu Jan 15 2015 at 5:16:21 PM soycode [email protected] wrote:

Status - what I want to do is add an e.g. if (!config.freezeScope)...
around
https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/moduleinternal.js#L367
with the freezeScope parameter (default false) passed in optionally along
with the manifest. Such parameters should get to moduleinternal (the
webworker scope) via message.config at
https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/moduleinternal.js#L47

• however, after stepping through and logging a bunch it seems like what
  actually makes it that far is site_cfg which is set up at
  https://github.com/freedomjs/freedom/blob/soycode-webworker-masking/src/entry.js#L47
  and doe s not in clude all the "external" config parameters.

The rub seems to be that the setup function which makes site_cfg does
receive the external config, but only when it is making the site_cfg for
the external scope. When it's setting up the webworker scope it's already
lacking that parameter and so doesn't have the information to stick in
site_cfg and pass on.

Anyway, I'll step through it more later but for now I wanted to (a)
document my investigation for my own benefit at least, and (b) let you know
in case I'm missing some more obvious way to pass config parameters to the
webworker. site_cfg does indeed work and pass some parameters in, but it
seems like it can't get them from the outermost scope, which is what I want.

—
Reply to this email directly or view it on GitHub
#110 (comment).

[ryscheng]

Out of curiosity, what properties does the Chrome debugger modify and for what reason?

[agallant]

Re: Will, yes but only when config is actually defined (and config is undefined when this is setting up webworkers, at least from my logging). For webworkers config seems undefined already by this point, so properties don't pass on.

Re: Ray, the error is "Uncaught TypeError: Can't add property __commandLineAPI, object is not extensible" - not sure the details, that exact error message doesn't give much on search but the specific __commandLineAPI gets results with people talking about various Chrome JavaScript dev issues. I'm assuming it needs to inject some property into the scope to give the nice interaction (ability to click through the hierarchy of any object, etc.).

[willscott]

soycode: 2 rounds of config we need to think about:

1st -> on the outer page the outer config should get mixed into site_cfg
and end up in the module's config.
2nd -> in the web worker, the external default site_cfg is around, but the
one in ModuleInternal should inherit from the external context due to:
https://github.com/freedomjs/freedom/blob/master/src/moduleinternal.js#L47

the config in link/worker within the worker doesn't get that now, though.

On Thu Jan 15 2015 at 7:43:24 PM soycode [email protected] wrote:

Re: Will, yes but only when config is actually defined (and config is
undefined when this is setting up webworkers, at least from my logging).
For webworkers config seems undefined already by this point, so properties
don't pass on.

Re: Ray, the error is "Uncaught TypeError: Can't add property
__commandLineAPI, object is not extensible" - not sure the details, that
exact error message doesn't give much on search but the specific
__commandLineAPI gets results with people talking about various Chrome
JavaScript dev issues. I'm assuming it needs to inject some property into
the scope to give the nice interaction (ability to click through the
hierarchy of any object, etc.).

—
Reply to this email directly or view it on GitHub
#110 (comment).

[ryscheng]

Is the only way to do this to freeze the global? Is there really no way to freeze WebSocket as undefined or null?
I would imagine there's a bunch of cases where we don't want to freeze global right?

[agallant]

With the whitelist we have to freeze the scope around it to prevent the potential of somebody deleting whatever masked object and causing it to be replaced with the regular browser primitive. It may be possible to switch to a blacklist approach and then just explicitly freeze those objects, but this would both be a somewhat different philosophy/design and also something with more potential for longterm maintenance (as new browser primitives come out etc.). I'm not necessarily opposed to it though, but would want to think through the costs/benefits some more.

Re: config, I've made a commit illustrating the points I'm trying to log at: 13c3412

The "config" argument received by setup (line 38 entry.js) does have the outer page parameters and does mix things into site_cfg, but only on calls to setup that seem to lead to the setup of the outer scope. When setup is being called for the webworker that same argument is undefined, and while site_cfg still gets populated it seems like it's basically hardcoded - for instance, "debug" is always "log" for the webworker, even when I set it to be something else for the outer config.

[ryscheng]

Makes sense. Thanks for the clarification
On Jan 16, 2015 11:34 AM, "soycode" [email protected] wrote:

With the whitelist we have to freeze the scope around it to prevent the
potential of somebody deleting whatever masked object and causing it to be
replaced with the regular browser primitive. It may be possible to switch
to a blacklist approach and then just explicitly freeze those objects, but
this would both be a somewhat different philosophy/design and also
something with more potential for longterm maintenance (as new browser
primitives come out etc.). I'm not necessarily opposed to it though, but
would want to think through the costs/benefits some more.

Re: config, I've made a commit illustrating the points I'm trying to log
at: 13c3412
13c3412

The "config" argument received by setup (line 38 entry.js) does have the
outer page parameters and does mix things into site_cfg, but only on calls
to setup that seem to lead to the setup of the outer scope. When setup is
being called for the webworker that same argument is undefined, and while
site_cfg still gets populated it seems like it's basically hardcoded - for
instance, "debug" is always "log" for the webworker, even when I set it to
be something else for the outer config.

—
Reply to this email directly or view it on GitHub
#110 (comment).

[agallant]

Just want to put a summary note of this issue (from a long ago comment by Will in a pull request):

---

Sure. I guess the thing we need to insure is that you can't do

delete WebSocket; 
var x = new WebSocket(); 

and have that work. It's the immutability of the outer DedicatedWorkerGlobalScope that needs to be ensured. We probably need to look at the semantics of importScripts as well, and see if it's possible for the imported script to run in a context we create that is mutable, rather than that locked down DedicatedWorkerGlobalScope.

---