1716641 - Crash [@ mozilla::dom::CanvasRenderingContext2D::PutImageData_explicit ] new ImageData crashes tab when used in putImageData from an extension content script

Status: NEW

(Core :: Graphics: Canvas2D, defect)

Description

[Poor Will Hunting]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

here is a test addon: https://github.com/aliko-str/tmpFFAddon2

a) get some image data as Uint8ClampedArray; do some work on them
b) create a new ImageData from that Uint8ClampedArray
c) use this ImageData in putImageData

Actual results:

the tab crashes

Expected results:

At least show an exception and tell me what's wrong, not a complete crash.

Interestingly, if an ImageData object is not created with a constructor, it works just fine -- use line 30 instead of 29 in the test addon

Comment 1

[Florian Quèze [:florian]]

Thanks for reporting this. When the tab crashed, a crash report was likely produced. Is there any chance you could go to the about:crashes page, double check that the crash report generated at the time you encountered the crash has been sent, and give here the crash id, so that we can see which code caused the crash?

Comment 2

[Poor Will Hunting]

bp-7ec98c1f-5964-405e-b22e-b53cd0210617

Comment 3

[Florian Quèze [:florian]]

(In reply to Poor Will Hunting from comment #2)

Thanks!

The crash is at https://searchfox.org/mozilla-central/rev/8d4c947c179ac3fbf88e81b8aa029b2014f5513e/dom/canvas/CanvasRenderingContext2D.cpp#5262 with a crash address of 0x0000000000000000, so it's very likely that aArray is null.

Emilio, I see you made the most recent changes to this file, but feel free to forward the needinfo if you are not the right person to look at this.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, so this assert is failing because GetDataObject is a cross-compartment wrapper:

(rr) p aImageData.mData.ptr->dump()
object 2de54e109358
  compartment 7f55ec280d80
  class 7f56073365d0 Proxy
    handler 7f5606fb4770 (CCW)
    private <Uint8ClampedArray object at 2de54e109258>
  shape 3720d2dab5c0
  flags: maybe_has_interesting_symbol not_native
  proto <dynamic>

And js::UnwrapUint8ClampedArray uses js::CheckedUnwrapStatic which returns null because handler->hasSecurityPolicy returns true here.

So there is at least two things to fix:

• That assert shouldn't exist (unless we call UncheckedUnwrap), and we should deal with the failure somehow.
• Maybe that code should use CheckedUnwrapDynamic or UncheckedUnwrap rather than CheckedUnwrapStatic? It's not clear which of the two is right in this case. We only use the Uint8Array to access its data / dimensions, so it shouldn't matter much to use an unchecked unwrap in this particular case, but it's my understanding that if we were to call UncheckedUnwrap and then expose the object to JS somehow it'd be a security bug. Perhaps we need to use UncheckedUnwrap just in the putImageData implementations?

Jandem / Olli, do you know of any precedent here or if what I wrote above makes sense to you? Thanks.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I guess we should deal with the failure anyways in case the compartment has been already nuked, since then it wouldn't be an array?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

• Maybe that code should use CheckedUnwrapDynamic or UncheckedUnwrap rather than CheckedUnwrapStatic?

I don't think CheckedUnwrapDynamic would make a difference, that only differs from CheckedUnwrapStatic for the Window/Location objects AFAIK. What kind of wrapper is this? Xray?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah, it's x-rays I think.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Actually how do I confirm that?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Actually how do I confirm that?

You can get the address of the proxy handler from the object-dump output ("handler 7f5606fb4770" in comment 4), then info symbol 0x<addr> in gdb should give you the type.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(I think jandem answered here :) )

Comment 11

[Bob Owen (:bobowen)]

I think the needinfo-chain broke here.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yep, it's a CCW:

xpc::FilteringWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::Opaque>::singleton in section .data.rel.ro of /home/emilio/src/moz/gecko/obj-debug/dist/bin/libxul.so

So the question is I guess whether this should bypass the cross-compartment security check in this case... Do you know who's the right person to make that call? If the answer is "it shouldn't", then I guess this is working as intended after bug 1717381.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

So the question is I guess whether this should bypass the cross-compartment security check in this case... Do you know who's the right person to make that call? If the answer is "it shouldn't", then I guess this is working as intended after bug 1717381.

Because it's indeed an opaque wrapper here, I don't think we should try to do anything clever. It would allow bypassing the security checks by copying the image data to a canvas element and then reading it back or something, right?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Yeah, content could indeed read back the image data from the add-on, but arguably it also can read it back if the add-on calls methods on the canvas like clearRect...