Closed Bug 929542 (SH-APIs) Opened 6 years ago Closed 5 years ago

[Meta] Fix potentially-insecure JS-implemented APIs and remove every instance of __exposedProps__ from the platform

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Assigned: dchanm+bugzilla)

References

Details

(Keywords: meta, sec-other)

Attachments

(1 file)

This is part (1) of slaughterhouse (bug 929539).
No longer blocks: slaughterhouse
Depends on: 926712
Depends on: 885982, 886110, 899322, 903873
Alias: slaughterhouse-APIs
Alias: slaughterhouse-APIs → SH-APIs
Attached file __exposedProps__.txt
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.
(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?
FWIW I've been removing a bunch of these as part of my work on bug 981845, please see its dependencies if interested.
(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.
Depends on: 986837
> let obj = Cu.createObjectIn(contentWindow);

Is it possible to replace with

let obj = new contentWindow.Object();

now?
Depends on: 1011084
Summary: [Meta] Remove every instance of __exposedProps__ from the platform → [Meta] Fix potentially-insecure JS-implemented APIs and remove every instance of __exposedProps__ from the platform
Depends on: 1021244
Depends on: 1022002
Depends on: 1023120
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?
(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.
Flags: needinfo?(jst)
(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.
Flags: needinfo?(jst)
Depends on: 1064437
Depends on: 1065185
Depends on: 1065186
Depends on: 981845
No longer depends on: 981845
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.