Rooting hazard in FromImageData
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox84 | --- | wontfix |
firefox85 | --- | fixed |
firefox86 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
FromImageData
takes in a dom::Uint8ClampedArray*
and then does stuff that can GC before using the data. If the typed array object is storing that data inline, then the GC could move the data and invalidate the pointer, which is a UAF.
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Please CC me so that I can access that revision for review!
Assignee | ||
Comment 3•4 years ago
|
||
Sorry. I threw a couple of patches up just before going on holiday just so people could look at them -- but I didn't realize they would be inaccessible due to security restrictions!
Assignee | ||
Comment 4•4 years ago
|
||
Note that I wanted to get the support code reviewed before deciding whether this was ready for review. But any feedback is certainly welcome.
Assignee | ||
Comment 5•4 years ago
|
||
As jgilbert pointed out, my patch above was totally bogus: it's returning a pointer to the stack.
It looks like I can tweak things a little to fix it, but I'm no longer convinced that anything here can GC. I wish I'd kept a copy of the hazard. I'm regenerating it now.
I'll probably want to change this code somehow regardless, because scopedArr
promises security that it does not deliver: scopedArr
becomes a glorified holder of a pointer and length after ComputeState
is called, so its scope doesn't mean anything for safety. But I'm still poking at this.
Comment 6•4 years ago
|
||
Can we root the typedarray where I was basing the scopedArr? That's what that code is trying to do.
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
Can we root the typedarray where I was basing the scopedArr? That's what that code is trying to do.
No, because the rooting will only keep the typed array alive. It won't prevent it from moving its data.
I attempted to make an API to modify the typed array to make sure it stored its data out of line. But that would require updating any existing pointers into the data. Which is somewhat doable, since we have backpointers to all views of an ArrayBuffer
(and if there's no ArrayBuffer
because the typed array is holding the data itself, then there are guaranteed to be no other views on it.) But that wouldn't work in 100% of the cases so it'd be a weird API, and currently those updates only happen during GC. It's messy.
Assignee | ||
Comment 8•4 years ago
|
||
For the record, here's the main hazard I'm trying to resolve with the current code:
Function 'void mozilla::ClientWebGLContext::TexImage(uint8, uint32, int32, uint32, const mozilla::avec3<int>*, const mozilla::avec3<int>*, int32, mozilla::webgl::PackingInfo*, mozilla::TexImageSource*) const' has unrooted 'scopedArr' of type 'mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8ClampedArray, JS_GetUint8ClampedArrayData, js::GetUint8ClampedArrayLengthAndData, JS_NewUint8ClampedArray>' live across GC call 'mozilla::Maybe<mozilla::webgl::TexUnpackBlobDesc>::~Maybe() [[complete_dtor]]' at dom/canvas/ClientWebGLContext.cpp:3997
ClientWebGLContext.cpp:3996: Assign(22,23, __temp_5.__scopedArr := scopedArr)
ClientWebGLContext.cpp:3996: Call(23,24, __temp_4 := __temp_5.operator()())
ClientWebGLContext.cpp:3997: Call(24,25, __temp_6 := desc.__conv_op ())
ClientWebGLContext.cpp:3997: Assume(25,26, !__temp_6*, true)
ClientWebGLContext.cpp:3997: Call(26,27, desc.~__dt_comp ()) [[GC call]]
ClientWebGLContext.cpp:3997: Call(27,28, scopedArr.~__dt_comp ())
GC Function: _ZN7mozilla5MaybeINS_5webgl17TexUnpackBlobDescEED1Ev$mozilla::Maybe<mozilla::webgl::TexUnpackBlobDesc>::~Maybe() [[complete_dtor]]
mozilla::Maybe<mozilla::webgl::TexUnpackBlobDesc>::~Maybe() [[base_dtor]]
mozilla::detail::MaybeStorage<T, false>::~MaybeStorage() [with T = mozilla::webgl::TexUnpackBlobDesc]
void mozilla::webgl::TexUnpackBlobDesc::~TexUnpackBlobDesc() [[complete_dtor]]
RefPtr<T>::~RefPtr() [with T = mozilla::gfx::DataSourceSurface]
RefPtr<T>::~RefPtr() [with T = mozilla::gfx::DataSourceSurface] [[base_dtor]]
static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = mozilla::gfx::DataSourceSurface; T = mozilla::gfx::DataSourceSurface]
static void mozilla::RefPtrTraits<U>::Release(U*) [with U = mozilla::gfx::DataSourceSurface]
void mozilla::detail::RefCounted<T, Atomicity>::Release() const [with T = mozilla::gfx::SourceSurface; mozilla::detail::RefCountAtomicity Atomicity = mozilla::detail::AtomicRefCount]
mozilla::gfx::SourceSurface.__dt_del :0
mozilla::layers::SourceSurfaceCanvasRecording.__dt_del :0
void mozilla::layers::SourceSurfaceCanvasRecording::~SourceSurfaceCanvasRecording() [[deleting_dtor]]
void mozilla::layers::SourceSurfaceCanvasRecording::~SourceSurfaceCanvasRecording()
void mozilla::layers::SourceSurfaceCanvasRecording::ReleaseOnMainThread(RefPtr<mozilla::layers::CanvasDrawEventRecorder>, mozilla::gfx::ReferencePtr, RefPtr<mozilla::gfx::SourceSurface>, RefPtr<mozilla::layers::CanvasChild>)
uint32 NS_DispatchToMainThread(already_AddRefed<nsIRunnable>*, uint32)
nsISerialEventTarget.Dispatch:0
(unknown-definition)
internal
To unpack that a little, it sees that when the lambda function returns, scopedArr
is in scope and therefore doesn't want to see a GC. scopedArr
is somewhat irrelevant because it's effectively dead, but the warning about a potential GC is valid because there is a modifiable pointer being held. The real question is whether a GC is actually possible or not.
(There's another hazard reported as well, but it looks even more like a false positive and looks like it might be fixable by teaching the analysis a little bit more about move semantics.)
Assignee | ||
Comment 9•4 years ago
|
||
Ok, I think this is pretty straightforward to fix after all. I don't have to worry about desc
being destructed, since scopedArr
and the data pointer are dead at that point.
The above hazard is on the line
if (!desc) return;
I just have to tell it that scopedArr
(and by extension, the problematic pointer) is dead here. That'll move the hazard to the end of the function:
Run<RPROC(TexImage)>(static_cast<uint32_t>(level), respecFormat,
CastUvec3(offset), pi, std::move(*desc));
I believe the data will be dead after this function returns -- it will have been uploaded to the webgl texture. So again, I can just declare scopedArr
to be dead at this point.
This means that scopedArr
, although it isn't really being used for much, is correctly marking the regions during which a GC is unsafe. I'm not sure it really needs to be passed into functions, since the analysis is already treating it as "you'd better not GC while this is live" (due to the added AutoCheckCannotGC
member.)
Updated•4 years ago
|
Assignee | ||
Comment 10•4 years ago
|
||
Given this outcome, this no longer needs to be marked as a security bug. It was a false positive in the hazard analysis.
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Comment 12•4 years ago
|
||
bugherder |
Assignee | ||
Comment 13•4 years ago
|
||
Comment on attachment 9194615 [details]
Bug 1684122 - Demarcate region where GC would break stuff
Beta/Release Uplift Approval Request
- User impact if declined: none. The only reason to land this patch is to make the hazard analysis clean if we decide now or later to backport bug 1680607. Without this change, the post-1680607 hazard analysis would report a false positive (false alarm) in this code. Which itself would only be a minor source of confusion, since no flaws would be exposed. I'm ok with either backporting this or not.
Note that I haven't even landed bug 1680607 yet, though I plan to do so very soon. If we wanted to get all of this stuff in, we could, but given that we're quite low on time in might be easier to not bother.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce: (run the hazard analysis with bug 1680607's changes landed in it)
- List of other uplifts needed: Bug 1684123
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This patch doesn't do anything. :-) It's only for explaining to the static analysis that the existing code is already safe.
- String changes made/needed: none
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: same as the beta argument
- User impact if declined: same as for beta
- Fix Landed on Version: 86
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): (see above)
- String or UUID changes made by this patch: none
Comment 14•4 years ago
|
||
Comment on attachment 9194615 [details]
Bug 1684122 - Demarcate region where GC would break stuff
Approved for 85.0b9 and 78.7esr.
Comment 15•4 years ago
|
||
bugherder uplift |
Comment 16•4 years ago
|
||
Comment on attachment 9194615 [details]
Bug 1684122 - Demarcate region where GC would break stuff
Actually, this code doesn't exist on ESR78 (introduced by bug 1607940). Per discussion with sfink, there's nothing to do here for ESR.
Updated•4 years ago
|
Comment 17•4 years ago
|
||
bugherder |
Description
•