1320519 - subarray() does not work with Xrayed TypedArrays in WebExtensions

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[u584145]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/600.5.17 (KHTML, like Gecko) Version/8.0.5 Safari/600.5.17
Build ID: 20161114145022

Steps to reproduce:

Given that file is a (large enough) user-provided File object (e.g. from a file input element), and the following code is running in a WebExtension content script:

var r = new FileReader();
r.onload = function (e) {
  var arr = new Uint8Array(e.target.result);
  console.log(arr.subarray(1,2));
}
r.readAsArrayBuffer(file);


Actual results:

The arr.subarray call in the onload handler fails with `Error: Permission denied to access property "constructor"` (though the browser console fails to identify the offending line.)


Expected results:

A single-byte Uint8Array should've been printed to the console.

Comment 1

[Tooru Fujisawa [:arai]]

can you provide a live example?

Comment 2

[u584145]

Attachment: Minimal WebExtension that reproduces the bug

Install the WebExtension and navigate to http://example.com. At the bottom of the page there should be a file input form; use it to pick any file larger than 3 bytes. The page should alert with the message "[object Uint8Array]", but in the console there will be a message "Error: Permission denied to access property "constructor"[Learn More]  (unknown)"

Comment 3

[Tom S. (please needinfo tschuster)]

Thanks for providing an example! You can fix your immediate problem by changing `new Uint8Array(` to `new content.Uint8Array(`.
I am not sure if this is supposed to work, cc'ing some people.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> Thanks for providing an example! You can fix your immediate problem by
> changing `new Uint8Array(` to `new content.Uint8Array(`.

I think you mean `window.Uint8Array`, but this also solves the problem: `new Uint8Array(new ArrayBuffer(e.target.result))`

From what I can tell, this is what's happening:

When you create a typed array view from an ArrayBuffer in a different compartment, we create the actual typed array view in the same compartment as the buffer, but give it a prototype from the same compartment as the caller.

Whenever a built-in method from that prototype is called on a wrapped object, it forwards it to the built-in method for that compartment.

Normally that works fine, but the subarray() method needs to create a new view of the same type as the original view, so it tries to read the `constructor` property.

In this case, though, that property is on a prototype object which lives in the content script compartment, which the content window compartment that the code is running in does not have permission to access.

Comment 5

[u584145]

(In reply to Kris Maglione [:kmag] from comment #4)
> I think you mean `window.Uint8Array`, but this also solves the problem: `new Uint8Array(new
> ArrayBuffer(e.target.result))`

Does the ArrayBuffer constructor perform a memory-to-memory copy of the buffer, or does it just give me a content-script-compartment view into it? It doesn't seem to be documented on MDN.

> From what I can tell, this is what's happening:
> 
> When you create a typed array view from an ArrayBuffer in a different
> compartment, we create the actual typed array view in the same compartment
> as the buffer, but give it a prototype from the same compartment as the
> caller.

Doesn't this mean that `arr instanceof Uint8Array` in the extension should return true, since it's comparing arr's prototype chain against the caller's notion of Uint8Array? But it doesn't - that's another thing I encountered when I hit upon this issue. I thought the typed array I got was Xrayed in body and soul...

Comment 6

[Kris Maglione [:kmag]]

(In reply to Lari Rasku from comment #5)
> (In reply to Kris Maglione [:kmag] from comment #4)
> > I think you mean `window.Uint8Array`, but this also solves the problem: `new Uint8Array(new
> > ArrayBuffer(e.target.result))`
> 
> Does the ArrayBuffer constructor perform a memory-to-memory copy of the
> buffer, or does it just give me a content-script-compartment view into it?
> It doesn't seem to be documented on MDN.

It creates a copy, yes.

> Doesn't this mean that `arr instanceof Uint8Array` in the extension should
> return true, since it's comparing arr's prototype chain against the caller's
> notion of Uint8Array? But it doesn't - that's another thing I encountered
> when I hit upon this issue. I thought the typed array I got was Xrayed in
> body and soul...

Normally it would, yes, but the fact that you wind up with an X-ray wrapper
complicates things, since that returns the default prototype in the target
compartment, regardless of what the actual object prototype property is.
`typedArray.wrappedJSObject instanceof Uint8Array` returns true.

Although, when I waive X-rays and then call .subarray() I get a different
error:

Error: Permission denied to access property "subarray"

Which is interesting. I think that means we're getting bounced into the owner
compartment of the wrapped array when we try to access the property, and then
that compartment fails to access the property on the privileged prototype.


Which also means that I was probably slightly wrong about what was happening
initially. Without X-rays, that's more or less what would happen. But with
them, I think we're just forwarding to the built-in prototype method in the
target compartment, and then that compartment is failing to access the
constructor property on the wrapped prototype from the privileged compartment.

Comment 7

[Kris Maglione [:kmag]]

I think that probably the simplest solution to this is only to create the prototype in the caller compartment if the target compartment is a subsumer of the caller. I'm not sure if that's entirely spec compatible, but I don't think this should ever apply to web content, so it may not matter.

Comment 8

[Kris Maglione [:kmag]]

Tom, were you planning on working on this, or should I?

Comment 9

[u584145]

This should probably go into a separate bug, but - should the fact that instanceof cannot identify cross-compartment prototypes be considered a bug as well? It's a small but annoying hurdle to navigate around if one is trying to write cross-browser extensions or libraries. Or is assuming such an equivalence insecure in some way?

I can appreciate that it's probably much harder trying to determine in exactly which contexts objects from different compartments should or shouldn't be interchangeable.

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Lari Rasku from comment #9)
>  should the fact that
> instanceof cannot identify cross-compartment prototypes be considered a bug
> as well? It's a small but annoying hurdle to navigate around if one is
> trying to write cross-browser extensions or libraries.

instanceof on ECMAScript stuff has always been single-global-scoped, saying "no" for |window.<ctor>() instanceof otherWindow.<ctor>| where |<ctor>| is Array, Object, String, Uint8Array, Date, Promise, &c. &c. &c.  You'd need an ECMAScript spec change for us to do this.  I doubt you could sell the TC39 committee that maintains the standard on this change given twenty years' history.  But if you really, really care, feel free to give it a shot there.  If they changed, we'd change.

Comment 11

[u584145]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> instanceof on ECMAScript stuff has always been single-global-scoped, saying
> "no" for |window.<ctor>() instanceof otherWindow.<ctor>| where |<ctor>| is
> Array, Object, String, Uint8Array, Date, Promise, &c. &c. &c.  You'd need an
> ECMAScript spec change for us to do this.

Aha, I see. Thanks, I didn't understand how exactly it discriminated between compartments. I think I'll work with what I've got.

Comment 12

[Tom S. (please needinfo tschuster)]

(In reply to Kris Maglione [:kmag] from comment #8)
> Tom, were you planning on working on this, or should I?
Not right now. If this isn't pressing I can probably get to this after Hawaii.