Bobby Holley (:bholley) Reporter
	Description • 11 years ago

This is part (1) of slaughterhouse (bug 929539).

	David Chan [:dchan] Assignee
	Comment 1 • 11 years ago

I cleaned up the document that I was using for keeping track of exposedProps usage. I will write a more in-depth summary in another comment.

Quick Summary
127 matching lines in 51 files

29 files were in non-testing code
22 files were either tests or non-code

A majority of the __exposedProps__ usage was in B2G code, mainly for the small temp objects bholley mentioned in Monday's meeting. There were no similar idioms to the InstallTrigger bug. However I identified a couple files we should look more closely at. These files are denoted by a *** in the text file.

1) RILContentHelper.js
2) WifiWorker.js
3) nsDOMIdentity.js

RILContentHelper.js contains a helper function which copies an object from src to dest. It does this by iterating over the src objects keys. We will have to investigated if there is a way to influence this code through the exposed APIs. If it is possible, then it should be the same attack as InstallTrigger

WifiWorker.js also follows a similar pattern. It takes a scan object and transforms it by iterating over the object then assigning to another object.

nsDOMIdentity.js also has a copy object utility function. It copies an aOption object to be used by a message manager by iterating over the keys. It does ignore keys that start with "_", but that may not be sufficient


Of the above three files, I think we should prioritize reviewing the Identity code first, then RILContentHelper, then WifiWorker.

I also tried to note all the files which make use of DOMRequests and link to the associated webidl/idl

DOMRequest files
1) ContactManager.js
2) NetworkStatsManager.js
3) RILContentHelper.js
4) DOMWifiManager.js
5) Webapps.js
6) PushService.js

The above objects have functions which pass / access COWs through DOMRequests. The COW access was for reading / manipulation from what I saw.

	Bobby Holley (:bholley) Reporter
	Comment 2 • 11 years ago

(In reply to David Chan [:dchan] from comment #1)
> I cleaned up the document that I was using for keeping track of exposedProps
> usage. I will write a more in-depth summary in another comment.

This writeup is already a great start. Thanks for doing it. :-)

> A majority of the __exposedProps__ usage was in B2G code, mainly for the
> small temp objects bholley mentioned in Monday's meeting.

Ok, great! I think those are very easily fixable. We just need to replace:

var obj = { foo: 42, bar: "hi", __exposedProps__: {foo: 'r', bar: 'r'}};

with

let obj = Cu.createObjectIn(contentWindow);
obj.foo = 42;
obj.bar = hi;

These should be very simple patches to write up, and we can probably short-circuit the need to do any coordination with the API authors. Can someone on the security team take a crack at it? It sounds like that would cut our __exposedProps__ usage by at least half, which would be awesome. :-)

> RILContentHelper.js contains a helper function which copies an object from
> src to dest. It does this by iterating over the src objects keys. We will
> have to investigated if there is a way to influence this code through the
> exposed APIs. If it is possible, then it should be the same attack as
> InstallTrigger
> 
> WifiWorker.js also follows a similar pattern. It takes a scan object and
> transforms it by iterating over the object then assigning to another object.
> 
> nsDOMIdentity.js also has a copy object utility function. It copies an
> aOption object to be used by a message manager by iterating over the keys.
> It does ignore keys that start with "_", but that may not be sufficient

Do the original objects come from content in these cases? If so, that definitely sounds like a problem.

> I also tried to note all the files which make use of DOMRequests and link to
> the associated webidl/idl
> 
> DOMRequest files
> 1) ContactManager.js
> 2) NetworkStatsManager.js
> 3) RILContentHelper.js
> 4) DOMWifiManager.js
> 5) Webapps.js
> 6) PushService.js
> 
> The above objects have functions which pass / access COWs through
> DOMRequests. The COW access was for reading / manipulation from what I saw.

Ok. Are these the little microformat objects that we can just fix with Cu.createObjectIn?

	(no longer active)
	Comment 3 • 10 years ago

FWIW I've been removing a bunch of these as part of my work on bug 981845, please see its dependencies if interested.

	Bobby Holley (:bholley) Reporter
	Comment 4 • 10 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> FWIW I've been removing a bunch of these as part of my work on bug 981845,
> please see its dependencies if interested.

Awesome! You rock.

	Masatoshi Kimura [:emk]
	Comment 5 • 10 years ago

> let obj = Cu.createObjectIn(contentWindow);

Is it possible to replace with

let obj = new contentWindow.Object();

now?

	:Gijs (he/him)
	Comment 6 • 10 years ago

So I just checked the current MXR list against the one attached here from last november, and while we've lost a few cases of __exposedProps__, we've gained:

/b2g/components/SystemAppProxy.jsm

    line 77 -- // If the root object already has __exposedProps__,
    line 79 -- if ('__exposedProps__' in details) {

/dom/system/gonk/NfcContentHelper.js

    line 70 -- __exposedProps__ : {canBeMadeReadOnly: 'r',

/dom/wifi/DOMWifiP2pManager.js

    line 63 -- obj.__exposedProps__ = exposedProps;

/dom/wifi/WifiP2pWorkerObserver.jsm

    line 59 -- this.__exposedProps__ = {

/mobile/android/components/PaymentsUI.js

    line 135 -- __exposedProps__: {

/addon-sdk/source/lib/sdk/content/sandbox.js

    line 180 -- // Bug 758203: We have to explicitely define `__exposedProps__` in order
    line 387 -- __exposedProps__: {
    line 396 -- __exposedProps__: {
    line 400 -- __exposedProps__: {



In Dutch we would refer to the situation as "mopping up while the tap is open".

Without tipping our hand, what can we do to avoid new cases from being added to the codebase all the time?

	Bobby Holley (:bholley) Reporter
	Comment 7 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #6)
> In Dutch we would refer to the situation as "mopping up while the tap is
> open".
> 
> Without tipping our hand, what can we do to avoid new cases from being added
> to the codebase all the time?

jst was going to set up a cron job that would notify us when new __exposedProps__ are added.

More generally though, my hope is that the tide is turning and we're mopping faster than we're leaking. We really need traction from b2g folks on some of those APIs though.

	Bobby Holley (:bholley) Reporter
	Comment 8 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #7)
> jst was going to set up a cron job that would notify us when new
> __exposedProps__ are added.

This is now done.

	Bobby Holley (:bholley) Reporter
	Comment 9 • 9 years ago

We didn't remove all of them, but we got most of them, and made __exposedProps__ sufficiently toothless such that this is no longer a major concern.