872772 - Xray expando infrastructure loses Xray waivers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This seems to be happening, presumably because of how we rewrap things into the target compartment. I'll attach a testcase.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Here's a testcase.

Comment 2

[Bobby Holley (:bholley)]

So. We store Xray expandos in the compartment of the Xrayed object (though they're entirely inaccessible to script running in that compartment). Moreover, Xray waivers are represented by a same-compartment wrapper in the compartment of the Xrayed object.

So when we have some waived wrapper to |foo| and want to store it on an Xray wrapper to an object in foo's compartment, we run into problems. Wrapping the object into foo's compartment causes the waiver to be stripped off, and we have no way of knowing to apply it again when it's retrieved.

I played with the idea of propagating these waivers into content in a general fashion via same compartment wrapping callback. But I think this has the potentially to get nasty. For example, if you have some waiver to a function function foo(x), invoking foo(foo) would cause content to see |x != foo|. There are probably a million other ways this breaks.

So I think I'm just going to manually apply these waivers when adding things to the Xray expando object. Patch coming up.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Fix up lost Xray waivers when wrapping into the content compartment. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Tests. v2

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=458ce58bb9b4

Comment 6

[Gabor Krizsanits (INACTIVE)]

This whole thing looks a bit hacky, but I really don't see any better approach either. I guess this is just something we get with the current architecture... I had some concerns about that unsafe unwrapping and also if we catch everything with this single getOwnPropertyDescriptor function, but it seems alright. There is only one case that might not be covered, if the expando property have a content function as its setter, then a., we lose the waiver, but that's kind of expected anyway, b., content code might be able to get behind same compartment security wrappers that way. Since the 'this' pointer in that content function will be the expando object itself (so you could do this.waivedComponents in it). But then again this whole scenario is like shooting yourself in the foot intentionally on the first place by using a content function as an expando property setter/getter... What do you think? Should we somehow check if the getters/setters of the expando props are always same compartment with the xray and not with the expando object itself?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> b., content code might be able
> to get behind same compartment security wrappers that way. Since the 'this'
> pointer in that content function will be the expando object itself (so you
> could do this.waivedComponents in it). But then again this whole scenario is
> like shooting yourself in the foot intentionally on the first place by using
> a content function as an expando property setter/getter... What do you
> think? Should we somehow check if the getters/setters of the expando props
> are always same compartment with the xray and not with the expando object
> itself?

(for posterity)

We discussed this on IRC. The key observation here is that |this| is never the expando object, because we set |obj| to |wrapper| whenever we return a property descriptor corresponding to the expando object. Expando objects are private to the XrayWrapper, and should never leak.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c98f7f0305a7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/532f418b9351
https://hg.mozilla.org/mozilla-central/rev/c98f7f0305a7