1681809 - Typed arrays generated by TextEncoder belong to a wrong context

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[KOLANICH]

When a TextEncoder is used in a WebExtension in a script in content_scripts, it returns typed arrays and their ArrayBuffers belonging to a wrong context.

1. returnedData isinstance Uint8Array is false
2. new Uint8Array(returnedData.buffer) isinstance Uint8Array is false
3. new returnedData.constructor(b.length) isinstance Uint8Array is false`
4. for another Uint8Array b with a correct context let res = new returnedData.constructor(b.length);res.set(b, 0);

Reproducer: https://gist.github.com/KOLANICH/5c85e09af65d87b24a3763c1316f099e

Related bugs:
https://github.com/greasemonkey/greasemonkey/issues/3097
https://github.com/violentmonkey/violentmonkey/issues/1124

Comment 1

[KOLANICH]

4. for another Uint8Array b with a correct context let res = new returnedData.constructor(b.length);res.set(b, 0); causes an error Permission denied to access object

Workaround: new Uint8Array([...utf8Enc.encode("a")]);

Comment 2

[Rob Wu [:robwu]]

This is confusing, but not unexpected due to the way that the content script sandbox is implemented.
The short version is: Pick a Uint8Array that you'd like to work with (window.Uint8Array if you work with DOM methods; due to the Xray wrapper this is slower than Uint8Array from the content script itself), and stick to it. You can convert between Uint8Arrays from different contexts with Uint8Array.from(returnedData); or window.Uint8Array.from(b);

Longer version to explain your observations:

1. TextEncoder here is actually an API from the web page, with Xray vision.
   The returned values are instances of Uint8Array objects from the web page (with Xray vision) instead of instances of Uint8Array from the content script's sandbox. This is the truthy condition that confirms what I said: new TextEncoder().encode("xxx") instanceof window.Uint8Array // true
2. Uint8Array is just a view for the underlying ArrayBuffer. Since returnedData is from the web page's context instead of the content script's, the ArrayBuffer belongs to the web page and the instantiated view is from the web page too. It would be a security bug to get a different result, because that would mean that one could use Uint8Array to read data from a different context. Again, the expected result is new Uint8Array(returnedData.buffer) instanceof window.Uint8Array
3. Since returnedData.constructor === window.Uint8Array, and window.Uint8Array !== Uint8Array, new returnedData.constructor(b.length) is equivalent to new window.Uint8Array(b.length)
4. b = new Uint8Array([0]); let res = new returnedData.constructor(b.length);res.set(b, 0); is equivalent to
   b = new Uint8Array([0]); let res = new window.Uint8Array(b.length);res.set(b, 0);.
   b and res are Uint8Arrays from different context, so the res.set(b, 0) call fails with Uncaught Error: Permission denied to access object

Comment 3

[Alex Cornestean]

Hello Rob,

Based on your observations from Comment 2, is this a valid issue or can it be set to Invalid ?

Comment 4

[KOLANICH]

TextEncoder here is actually an API from the web page, with Xray vision.

Thanks for the info. Why don't content scripts have own TextEncoders?

Comment 5

[Rob Wu [:robwu]]

(In reply to Alex Cornestean from comment #3)

Hello Rob,

Based on your observations from Comment 2, is this a valid issue or can it be set to Invalid ?

Depends - what does the reporter wanted to achieve with this bug report? I can explain the behavior, but I also recognize that it may be confusing. If there isn't anything actionable I would close out the issue, but I'd first like to ask the reporter if they are expecting any changes.

(In reply to KOLANICH from comment #4)

TextEncoder here is actually an API from the web page, with Xray vision.

Thanks for the info. Why don't content scripts have own TextEncoders?

Because they already have access to TextEncoder inherit from window. There is no clearly good choice either way. For example, fetch is from the content script and returns typed arrays in the context of the content script, but content.fetch is from the web page. Extensions that rely on content.fetch + TextEncoder may break if we replace the TextEncoder with the one from the content script.

Comment 6

[Tomislav Jovanovic :zombie]

Rob has investigated if this is a real bug or we can close it.

Comment 7

[KOLANICH]

I wonder if it is possible to modify the behavior of *Encoders so they return data which context belongs not to the one of the encoder object (if it is not modified or inherited), but matching the one of its inputs.

Comment 8

[Tomislav Jovanovic :zombie]

While most of these can be explained once you understand the underlying Xrays mechanism, this one seems especially confusing:

(In reply to KOLANICH from comment #0)

2. new Uint8Array(returnedData.buffer) isinstance Uint8Array is false

(In reply to Rob Wu [:robwu] from comment #2)

2. Uint8Array is just a view for the underlying ArrayBuffer. Since returnedData is from the web page's context instead of the content script's, the ArrayBuffer belongs to the web page and the instantiated view is from the web page too. It would be a security bug to get a different result, because that would mean that one could use Uint8Array to read data from a different context. Again, the expected result is new Uint8Array(returnedData.buffer) instanceof window.Uint8Array

While I can buy that explanation, I'm not sure it's the only interpretation that could be considered correct.

Matthew or Kris, do you maybe have a better idea what's going on here? Are there perhaps easy wins we could fix here with not too much effort (for any of the original points)?

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Clearing the ni? on myself here -- I don't think I have anything I can add here. This seems to be an API design question around content sandboxing, so pretty far outside my knowledge.

Comment 10

[Tomislav Jovanovic :zombie]

Back to Ted I guess. :)

Comment 11

[Kris Maglione [:kmag]]

The new Uint8Array() case is the only one that's ambiguous. The others are unarguably correct.

Having new TypedArray(crossCompartmentArrayBuffer) return an object in the compartment of the TypedArray constructor rather than the ArrayBuffer isn't a security issue, since the TypedArray compartment's principal would need to subsume the ArrayBuffer's compartment's principal, and only compartments with principal which subsumed the TypedArray compartment's principal would be able to access the result object.

But from an implementation perspective, it's much more difficult, since we'd need the typed array machinery to be able to handle storing references to cross-compartment ArrayBuffers, which is non-trivial, and could have performance implications.

I think that probably makes it not worth the effort. But I'm also not sure whether the spec has an opinion on this. It might, but if so, it's not really my area.

Comment 12

[Rob Wu [:robwu]]

We can't easily change this now because it's a consequence of the implementation. I'm marking it as P3S3 because related bugs have a similar classification.