Bug 914970 (XrayToJS)

Implement meaningful Xrays to non-reflector JS objects

REOPENED
Unassigned

Status

()

defect
REOPENED
6 years ago
5 months ago

People

(Reporter: bholley, Unassigned)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
This has becoming more and more of a problem, and I think we'll need to fix it at some point.

The basic problem is that our Xray machinery only works for reflectors (either XPCWN or new binding objects). This is problematic from a number of angles:

1 - |new contentWindow.DOMError()| works, but |new contentWindow.Error()| does not. More generally, we don't have any safe way to create instances of ES-defined objects in Xrayed scopes. In bug 629607, for example, the console API wants to create a proxy in the content scope, but can't do so safely without some Cu hackery. We currently have Cu.createObjectIn, and we may need to add Cu.createProxyIn. But these are all just crutches for our inability to do |new contentWindow.Object(...)| and |new contentWindow.Proxy(...)|.

2 - As more and more stuff starts getting implemented in ES, we start to seriously suffer from our inability to safely manipulate objects in Xrayed scopes. Typed Arrays are one such example, but there are others. Anne wants to move Promises into the JS engine, for example.

The traditional reasoning for the lack of Xrays in this situation is "well, what would you Xray _to_? It's just JS!" This is somewhat valid, but not really. Different JS objects (like Date) have plenty of protected state that lives in reserved slots, but that information isn't exposed outside of the engine.

Bill and I talked for about this for a while on monday. Our main observation was that, modulo certain JSClass hooks, most of the interface for different types of JS objects is encapsulated in JSNative-implemented functions that live on the prototype. We can most likely use this to our advantage. Here's what I think we should do:

1 - Add an API to spidermonkey to access the "natural" prototype of a standard object. This should be very straightforward to do, because of the JS_IdentifyClassPrototype machinery that I added last year. This machinery currently just detect whether the object in question is a bonafide prototype. But since the prototypes share a JSClass with instances, all we need to do is invert the filtering machinery in js_IdentifyClassPrototype so that we detect instances instead.

2 - Add an API to look up the bonafide JSNatives that should exist on the prototype, ignoring whatever mucking about the content script may have done with the prototype object itself. I have no idea how hard this is, but it's probably not trivial. Anyone have any thoughts on how to go about this?

3 - Implement Xray traits for identifiable standard objects that utilize the above APIs to determine the natural behavior of the object. We may need to do some special-casing for objects that rely on JSClass hooks (Arrays, what else?), but this should be doable if we solve (2).

Thoughts? I'd especially like to heard from Waldo/jorendorff/luke about (2).
Re (2), suppose each prototype object had a pointer to the JSFunctionSpec/JSPropertySpec arrays. That wouldn't be hard. Or we could embed those pointers in the JSClass instead, which would be even safer.

We could support JS-language methods and accessors too. It would just take a little more work. We could have some kind of (per-compartment?) registry of the methods and accessors of JSClasses, and then you could make a JSClass xray-able just by adding the relevant information about it to the registry (which we would let you do from C++ or from self-hosted, or chrome, JS code).
(Reporter)

Comment 2

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Re (2), suppose each prototype object had a pointer to the
> JSFunctionSpec/JSPropertySpec arrays. That wouldn't be hard. Or we could
> embed those pointers in the JSClass instead, which would be even safer.

That sounds excellent.
 
> We could support JS-language methods and accessors too.

What does that mean, exactly? JSClass hooks in [[ESSpec]] form?

> It would just take a
> little more work. We could have some kind of (per-compartment?) registry of
> the methods and accessors of JSClasses, and then you could make a JSClass
> xray-able just by adding the relevant information about it to the registry
> (which we would let you do from C++ or from self-hosted, or chrome, JS code).

How far would we get if we implemented without this as a first pass? Would things still be pretty usable, on balance? I think we probably want to be able to support [[Call]], but we could maybe special-case it for now.
(In reply to Bobby Holley (:bholley) from comment #2)
> > We could support JS-language methods and accessors too.
> 
> What does that mean, exactly? JSClass hooks in [[ESSpec]] form?

No, I just meant methods and accessors that are implemented in JS. Currently only methods. Like Array.prototype.map (which is implemented in js/src/builtin/Array.js; it's called ArrayMap there).
More stuff is self-hosted every time I look.

Some standard objects have plain old data properties, defined on the object itself, not a prototype. For example, arrays have a .length property, RegExps have .source, .global, .lastIndex; and so on.

I didn't think of JSClass hooks at all. Some properties, like func.length and func.caller and all globals, are defined by resolve hooks. Except for globals, I think we can live without most of these. It might seem a little arbitrary to users.

The global object is special, as always. We want `new contentWindow.Error()` to call the real Error constructor, no matter what the content has done to the Error property, right? All GlobalObjects keep that original constructor around in a reserved slot, but there's no public JSAPI function to get them out yet.

> How far would we get if we implemented without this as a first pass? Would
> things still be pretty usable, on balance?

X-rays would only be able to call builtin functions that are not self-hosted. Any time we switched a JSNative to self-hosted code, we would risk breaking chrome or addons. Sounds bad.
We can support `new contentWindow.Object()` followed by `contentWindow.Object.defineProperty()`, but it might be more convenient (to use and to implement!) for chrome to get an API for creating structured clones of chrome objects into content windows.

BTW, I don't think it's safe to call C.[[Construct]] — that is, JS_New — directly on a content constructor C, even if you know that C is the exact original constructor object you want to call. For most constructors, content can change .prototype, and in ES6 there will also be a C.@@create() method that content can tamper with.
(Reporter)

Comment 5

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> We can support `new contentWindow.Object()` followed by
> `contentWindow.Object.defineProperty()`, but it might be more convenient (to
> use and to implement!) for chrome to get an API for creating structured
> clones of chrome objects into content windows.

Yeah, we have various such APIs. Gabor recently wrote some stuff that allows you to export a function to an untrusted scope which, when invoked, performs a structured clone on everything but reflectors (which get Xrayed). But that stuff's only exposed to sandboxes.

We also have Cu.createObjectIn, which creates an object in content. The usual pattern is:

let obj = Cu.createObjectIn(contentScope);
Object.defineProperties(obj, propspec);
Cu.makeObjectPropsNormal(obj);

This is tricky, but should be safe I think, since defineProperty should never hit the prototype.

> BTW, I don't think it's safe to call C.[[Construct]] — that is, JS_New —
> directly on a content constructor C, even if you know that C is the exact
> original constructor object you want to call. For most constructors, content
> can change .prototype, and in ES6 there will also be a C.@@create() method
> that content can tamper with.

I would think both of those would be things we could Xray around, right?
(In reply to Bobby Holley (:bholley) from comment #5)
> > BTW, I don't think it's safe to call C.[[Construct]]
> > [...] For most constructors, content
> > can change .prototype, and in ES6 there will also be a C.@@create() method
> > that content can tamper with.
> 
> I would think both of those would be things we could Xray around, right?

Yes.
(Reporter)

Updated

6 years ago
Blocks: SH-wrappers
(Reporter)

Updated

6 years ago
Blocks: 929609
Anyone is working on this bug?
(In reply to Kevin Hu [:khu] from comment #7)
> Anyone is working on this bug?

Kevin, NFC only cares about bug 933681, I think (assuming you're here because you're looking for what blocks bug 933497).
(Reporter)

Comment 9

6 years ago
(In reply to Kevin Hu [:khu] from comment #7)
> Anyone is working on this bug?

It is on my list of things to do, but it's a large project.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Kevin, NFC only cares about bug 933681, I think (assuming you're here
> because you're looking for what blocks bug 933497).

Assuming that all you need to do are to create the foreign TypedArray, and access .length and numerically-indexed properties. I _think_ that's safe without Xrays (i.e. even if you don't trust the standard prototype), but luke would know for sure.
Flags: needinfo?(luke)
(Reporter)

Comment 10

6 years ago
Actually, luke says that indexed access is safe, but length is _not_.
Flags: needinfo?(luke)
(Reporter)

Comment 11

6 years ago
Kyle, is that going to meet your needs, or are we going to need Xrays to the TypedArray too? The latter isn't out of the question, it would just take some prioritization.
Flags: needinfo?(khuey)
(Reporter)

Comment 12

6 years ago
(luke also says that negative indices will hit the proto, and are thus unsafe).
This might be obvious from context but, just to be sure: you do know that you definitely have a real typed array (not an object pretending to be one)?
(Reporter)

Comment 14

6 years ago
Because in bug 933681 I'm resolving standard constructors onto Xrayed globals.
No, we don't need xrays to the typed array.  We just need to be able to create it in the content compartment.
Flags: needinfo?(khuey)
(Reporter)

Comment 16

5 years ago
I'm actively working on this. I'm just laying some initial groundwork, but this is a big enough task that I'm going to do periodic try pushes as I go along to catch regressions from anything I've done so far.

https://tbpl.mozilla.org/?tree=Try&rev=803b0d3e6bf8
(Reporter)

Updated

5 years ago
Depends on: 959012
(Reporter)

Updated

5 years ago
Depends on: 959013
(Reporter)

Updated

5 years ago
No longer depends on: 959012
(Reporter)

Updated

5 years ago
Depends on: 972071
(Reporter)

Updated

5 years ago
No longer depends on: 972071
(Reporter)

Updated

5 years ago
Depends on: 973780
(Reporter)

Updated

5 years ago
Depends on: 975042
(Reporter)

Updated

5 years ago
No longer depends on: 973780, 959013
(Reporter)

Updated

5 years ago
Depends on: 972987
(Reporter)

Updated

5 years ago
Depends on: 976148
(Reporter)

Updated

5 years ago
Blocks: 976307
(Reporter)

Updated

5 years ago
Alias: XrayToJS
(Reporter)

Updated

5 years ago
Assignee: nobody → bobbyholley
(Reporter)

Updated

5 years ago
Depends on: 987111
(Reporter)

Updated

5 years ago
Depends on: 987163
(Reporter)

Updated

5 years ago
Depends on: 987669
(Reporter)

Updated

5 years ago
Depends on: 1014991
(Reporter)

Updated

5 years ago
Depends on: 1015798
(Reporter)

Updated

5 years ago
Depends on: 1020609
(Reporter)

Updated

5 years ago
Depends on: 1023984
(Reporter)

Updated

5 years ago
No longer blocks: 929609
(Reporter)

Updated

4 years ago
Depends on: 1155700
(Reporter)

Updated

3 years ago
Assignee: bobbyholley → nobody
Depends on: 1248865
Depends on: 1130988
Depends on: 1315563
Depends on: 1333073

Comment 18

11 months ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Depends on: 1505511
You need to log in before you can comment on or make changes to this bug.