1143810 - Remove some SetPropertyStubs that are not needed anymore.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jason Orendorff [:jorendorff]]

I wrote:
> XPConnect can defines properties with
> * JSPROP_GETTER set, and the setter is a Function object (XPC_WN_GetterSetter)
> * JSPROP_SETTER *not* set, setter=XPC_WN_OnlyIWrite_SetPropertyStub
>
> I very much want to eliminate this case. Can we just make the setter
> null? Or turn the setter into a JSNative? (But the existing setter
> doesn't seem to *do* anything.)

Capt. Holley wrote:
> The point of this setup is that we need to allow property sets by
> XPConnect itself (because the properties on XPC_WN_NoHelper are lazily
> resolved) while at the same time preventing expandos (hence the "only
> I write" name). Both of these properties are very important to
> preserve - do you have any ideas on another way to accomplish them
> short of rewriting XPCWNs as proxies?

I replied:
> What if this property were defined just like any other getter-only
> accessor property? Suppose we just change the JSClass::setProperty
> hook to null.
>
> 1. I claim this is not a security hole: both properties you mentioned
> still hold:
>
>     a) Creating an expando property on this thing fails, because it
>     has an addProperty hook that throws if the caller isn't
>     XPConnect. Even in the existing code, the setter is not called in
>     this case---we don't get that far.
>
>     b) Setting an *existing* property resolved/defined by XPConnect on
>     this object will fail with no effect, because it'll have a getter
>     and no setter. (To be totally precise this is a TypeError in
>     strict mode code and a silent failure in non-strict code. But the
>     security property, that the object is not affected in any way,
>     holds regardless.)
>
> 2. I claim this is not a bug because XPConnect never needs to *set*
> properties of this kind. Only define them. Because what would setting
> them do? It doesn't do anything; these properties have no slot and a
> setter that does nothing on success.

Capt. Holley replied:
> Ok sounds good - make it so, but please add a test to make sure
> expandos aren't allowed on, say, nsITimer, just in case we don't have
> a test for that already.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Remove some SetPropertyStubs that I think are not needed anymore. try: -b do -p linux64,win32 -u all -t none

Comment 2

[Jason Orendorff [:jorendorff]]

Bobby, this patch modifies three JSClasses, but the test only tries tampering with one kind of object (an nsITimer).

Maybe the test should spot-check the other classes too. How do I get objects of those classes to test with?

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Remove some SetPropertyStubs that I think are not needed anymore. try: -b do -p linux64,win32 -u all -t none

Comment 4

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=78f241a05780

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8578385 [details] [diff] [review]
Remove some SetPropertyStubs that I think are not needed anymore. try: -b do -p linux64,win32 -u all -t none

Review of attachment 8578385 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the awesome tests, Jason!

To clarify, I'm not aware of any security issues per se with properties on XPCWNs (we've almost ceased to expose them entirely to non-SystemPrincipaled code). The issue is that we don't have a way to manage these expandos across compartment boundaries (because precreate-less XPCWNs have no canonical compartment), and so we can't allow code to depend on them. This property was crucial to making compartments work, because Blake was able to do the trick in WrapperFactory::PrepareForWrapping where we simply duplicate the XPCWN in the new compartment. Same kind of thing applies when we do e10s tricks to hoist addon code into new compartments.

To make the tests complete, I'd add an nsPrincipal (say, an nsNullPrincipal instance), which has classinfo and should therefore get a proto. I don't think there's much value in testing both the WithCall and NoCall versions (not sure we even need WithCall anymore).

Oh! And do also test a tearoff (see the tearoff unit test in xpconnect/tests/unit on how to create one).

Thanks for doing this!

Comment 6

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92adb459d519

Comment 7

[Phil Ringnalda (:philor)]

Backed out that whole push in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3638d994edd for massive widespread bustage, https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7613fc978d36&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception - something must have changed out from under you.

Comment 8

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c018aa2e76e6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c018aa2e76e6