595081 - Don't allow changes to be made to AddonManager and AddonManagerInternal

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

By using Object.freeze we can make it impossible to replace/remove/add properties to these core objects. Since we currently have no way to override JSM loads themselves this I think makes it impossible to replace any part of the add-ons manager.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Changing properties doesn't throw exceptions since we aren't in strict JS mode, but the test verifies that nothing changes anyway.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Should be doing the same to AddonManagerInternal - there was some discussion on IRC about getting that object by working around JSM exports via Components.utils.getGlobalForObject().

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Disappointing that it's possible to get at the rest of the JSM like that but I guess this doesn't really cost us anything.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 482257 [details] [diff] [review]
patch rev 2

This shouldn't effect anything except people trying to do bad things to the extension manager.

Comment 5

[Dave Garrett]

Technically speaking, it should help others who would break if some idiots tried to do bad things to the manager by saving them a headache. ;)

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Apparently I didn't correctly test the last change, it fails because we actually modify properties on AddonManagerInternal. We can avoid doing that though it means moving one property to a global variable. An alternative I guess would be to iterate all the properties and lock them individually if they are functions but this seems simpler.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 482650 [details] [diff] [review]
patch rev 3

*sigh* still not right for some reason

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 482675 [details] [diff] [review]
patch rev 4

Landed: https://bugzilla.mozilla.org/show_bug.cgi?id=595081

Comment 10

[Dave Townsend [:mossop]]

Err the landing was http://hg.mozilla.org/mozilla-central/rev/e72ff6aed4ef

Comment 11

[Nochum Sossonko [:Natch]]

Ftr, after some discussion on #jsapi, it seems Object.defineProperty would actually do what was intended here (i.e. restrict the object and all it properties from being deleted, overwritten, configured, etc.).

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> Ftr, after some discussion on #jsapi, it seems Object.defineProperty would
> actually do what was intended here (i.e. restrict the object and all it
> properties from being deleted, overwritten, configured, etc.).

Can you elaborate on that for those that weren't in the channel?

Comment 13

[Nochum Sossonko [:Natch]]

(In reply to comment #12)
With Object.freeze the properties are frozen and can't be changed. If, however, the object is copied to the __proto__ of another object, and then the object is replaced with the new object, we're back to square one. IOW:

var foo = {b:true};var bar = {b:false};Object.freeze(foo);
bar.__proto__ = foo; foo = bar;
print(foo.toSource()); // prints {b:false};

Object.defineProperty however does not allow that.

Object.defineProperty(window, "foo", {value: {b: true}}); var bar = {b: false};
bar.__proto__ = foo; foo = bar;
print(foo.toSource()); // prints {b: true};

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> (In reply to comment #12)
> With Object.freeze the properties are frozen and can't be changed. If, however,
> the object is copied to the __proto__ of another object, and then the object is
> replaced with the new object, we're back to square one. IOW:
> 
> var foo = {b:true};var bar = {b:false};Object.freeze(foo);

The last statement, Object.freeze(foo);, affects the object denoted by 'foo' in the scope chain. It does *not* affect the attributes of the property 'foo' in the global object.

> bar.__proto__ = foo; foo = bar;
> print(foo.toSource()); // prints {b:false};
> 
> Object.defineProperty however does not allow that.
> 
> Object.defineProperty(window, "foo", {value: {b: true}}); var bar = {b: false};
> bar.__proto__ = foo; foo = bar;

The foo = bar; attempt fails because here, Object.defineProperty has made 'foo' non-configurable (no configurable:true in the object initialiser passed as the 3rd parameter to Object.defineProperty).

But the object denoted by 'foo' is still not frozen!

> print(foo.toSource()); // prints {b: true};

So foo.b = false; would succeed, which is not what you want.

Moral is you can't blame Object.freeze or any particular API here. You need both freeze and defineProperty, and probably more, to be tamper-proof. Each level of the object-graph's spanning tree starting from the global object has to be audited to see what property attributes allow mutation of other attrs or of value; and for an object reference value, whether the next ply down (in that object) you can mutate or not.

/be

Comment 15

[Nochum Sossonko [:Natch]]

Correct.

Basically, you'd just want:
Object.defineProperty(this, "AddonManagerInternal", {value: Object.freeze(AddonManagerInternal)});
Object.defineProperty(this, "AddonManagerPrivate", {value: Object.freeze(AddonManagerPrivate)});
Object.defineProperty(this, "AddonManager", {value: Object.freeze(AddonManager)});

I think that should do it. :)

Comment 16

[Nochum Sossonko [:Natch]]

Actually, I'm not sure if you can defineProperty on a property that's already defined (even though it seems from the docs that you should be able to).

Comment 17

[Dave Garrett]

(In reply to comment #15)
> Object.defineProperty(this, "AddonManagerInternal", {value:
> Object.freeze(AddonManagerInternal)});
> Object.defineProperty(this, "AddonManagerPrivate", {value:
> Object.freeze(AddonManagerPrivate)});
> Object.defineProperty(this, "AddonManager", {value:
> Object.freeze(AddonManager)});

There's got to be a less convoluted way to do that.

Comment 18

[Dave Garrett]

(In reply to comment #13)
> (In reply to comment #12)
> With Object.freeze the properties are frozen and can't be changed. If, however,
> the object is copied to the __proto__ of another object, and then the object is
> replaced with the new object, we're back to square one. IOW:
> 
> var foo = {b:true};var bar = {b:false};Object.freeze(foo);
> bar.__proto__ = foo; foo = bar;
> print(foo.toSource()); // prints {b:false};

How does this problem apply here? Objects inside a JavaScript module are being frozen and the scope inside the module is already non-accessible to the outside. Once the frozen object is exported, that should be enough. Wouldn't the above attempt just give you a modifiable object within the scope you have, but NOT actually modifying the original object? In other words, this hack would let you get an AddonManager object you can play with, but the actual real AddonManager object in the JavaScript module would still be left alone. (Brendan, am I correct here?)

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> (In reply to comment #13)
> > (In reply to comment #12)
> > With Object.freeze the properties are frozen and can't be changed. If, however,
> > the object is copied to the __proto__ of another object, and then the object is
> > replaced with the new object, we're back to square one. IOW:
> > 
> > var foo = {b:true};var bar = {b:false};Object.freeze(foo);
> > bar.__proto__ = foo; foo = bar;
> > print(foo.toSource()); // prints {b:false};
> 
> How does this problem apply here? Objects inside a JavaScript module are being
> frozen and the scope inside the module is already non-accessible to the
> outside. Once the frozen object is exported, that should be enough. Wouldn't
> the above attempt just give you a modifiable object within the scope you have,
> but NOT actually modifying the original object? In other words, this hack would
> let you get an AddonManager object you can play with, but the actual real
> AddonManager object in the JavaScript module would still be left alone.
> (Brendan, am I correct here?)

My understanding is that you can get the global object for the JSM using Components.utils.getGlobalForObject() (a mistake IMO)

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #18)
> How does this problem apply here? Objects inside a JavaScript module are being
> frozen and the scope inside the module is already non-accessible to the
> outside

As I said in comment 2, apparently you can get the scope object via Components.utils.getGlobalForObject(). Which would let you do things like:

ie: Components.utils.getGlobalForObject(AddonManager).AddonManagerInternal = myObject;

(Note that I haven't tested that, I'm just going off what I've been told)

Comment 21

[Dave Garrett]

(In reply to comment #19)
> My understanding is that you can get the global object for the JSM using
> Components.utils.getGlobalForObject() (a mistake IMO)

(In reply to comment #20)
> As I said in comment 2, apparently you can get the scope object via
> Components.utils.getGlobalForObject(). Which would let you do things like:
> 
> ie: Components.utils.getGlobalForObject(AddonManager).AddonManagerInternal =
> myObject;

That's past a mistake; that's dangerous. The whole point of JSM is to only export the symbols in EXPORTED_SYMBOLS, so any way around that is against its original design. If JSM is to be used for secure internal code like this then JSM really should be immune to getGlobalForObject, or at least allow defining of an option to do so. There a bug somewhere for this or should we file one?

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Actually, I'm not sure if you can defineProperty on a property that's already
> defined (even though it seems from the docs that you should be able to).

You can -- defineProperty can change an existing property if it is configurable. If not configurable, you can redefine a writable data property to be a different data property. See:

http://wiki.ecmascript.org/doku.php?id=es3.1:attribute_states&s=state+transition

/be

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #14)
> > bar.__proto__ = foo; foo = bar;
> > print(foo.toSource()); // prints {b:false};
> > 
> > Object.defineProperty however does not allow that.
> > 
> > Object.defineProperty(window, "foo", {value: {b: true}}); var bar = {b: false};
> > bar.__proto__ = foo; foo = bar;
> 
> The foo = bar; attempt fails because here, Object.defineProperty has made 'foo'
> non-configurable

I meant to write "non-writable", not "non-configurable", argh.

This stuff is fairly hairy. The complexity of comment 15 is not terrible and it is hard to build up a bigger gun to shoot at the problem. An Object.deepFreeze, for example, would have to walk a spanning tree of some object graph, and could easily become an Ice-9 disaster. We don't want to freeze too much.

/be

Comment 24

[Dave Garrett]

(In reply to comment #20)
> As I said in comment 2, apparently you can get the scope object via
> Components.utils.getGlobalForObject(). Which would let you do things like:
> 
> ie: Components.utils.getGlobalForObject(AddonManager).AddonManagerInternal =
> myObject;
> 
> (Note that I haven't tested that, I'm just going off what I've been told)

I just noticed that it's far simpler than that and getting the global object for JSM is apparently trivial, which we'd know if one of us RTFM:
http://hg.mozilla.org/mozilla-central/file/df7ea854a68c/js/src/xpconnect/idl/xpccomponents.idl#l194

Components.utils.import() _returns_ the entire global object for the module.

Can prove with a lazy test. Evaluate the following in the Error Console:
(function(){
var AMglobalObj = Components.utils.import("resource://gre/modules/AddonManager.jsm");
AMglobalObj.EXPORTED_SYMBOLS = ["monkey"];
})();

Then evaluate this:
(function(){
var AMglobalObj = Components.utils.import("resource://gre/modules/AddonManager.jsm");
alert(AMglobalObj.EXPORTED_SYMBOLS);
})();

Yep, it alerts "monkey". JavaScript module global scopes are in no way protected at all.

Side note: it looks like it might be a good idea to make all EXPORTED_SYMBOLS everywhere const instead of var to resist this particular test.

Comment 25

[Dave Garrett]

(the comment 24 test obviously kills the Addon Manager entirely, by the way)

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hard to test manually. I trust the automated tests we have for this patch.
Marking as verified fixed based on no reported test failures.