Xray expando infrastructure loses Xray waivers

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla24
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

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

Comment 1

6 years ago
Posted patch Tests. v1 (obsolete) — Splinter Review
Here's a testcase.
Assignee

Comment 2

6 years ago
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.
Assignee

Updated

6 years ago
No longer blocks: 871887
Assignee

Comment 4

6 years ago
Posted patch Tests. v2Splinter Review
Attachment #750204 - Flags: review?(gkrizsanits)
Assignee

Updated

6 years ago
Attachment #750121 - Attachment is obsolete: true
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?
Attachment #750203 - Flags: review?(gkrizsanits) → review+
Attachment #750204 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 7

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/532f418b9351
https://hg.mozilla.org/mozilla-central/rev/c98f7f0305a7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.