Closed Bug 1684122 Opened 4 years ago Closed 4 years ago

Rooting hazard in FromImageData

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox84 --- wontfix
firefox85 --- fixed
firefox86 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file)

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.

Depends on: 1684123
Assignee: nobody → sphink
Status: NEW → ASSIGNED

Please CC me so that I can access that revision for review!

Flags: needinfo?(sphink)

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!

Flags: needinfo?(sphink)

Note that I wanted to get the support code reviewed before deciding whether this was ready for review. But any feedback is certainly welcome.

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.

Can we root the typedarray where I was basing the scopedArr? That's what that code is trying to do.

(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.

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.)

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.)

Attachment #9194615 - Attachment description: Bug 1684122 - Fix WebGL hazards → Bug 1684122 - Demarcate region where GC would break stuff

Given this outcome, this no longer needs to be marked as a security bug. It was a false positive in the hazard analysis.

Group: core-security
Depends on: 1686532
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b02f5b16330 Demarcate region where GC would break stuff r=jgilbert
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

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
Attachment #9194615 - Flags: approval-mozilla-esr78?
Attachment #9194615 - Flags: approval-mozilla-beta?

Comment on attachment 9194615 [details]
Bug 1684122 - Demarcate region where GC would break stuff

Approved for 85.0b9 and 78.7esr.

Attachment #9194615 - Flags: approval-mozilla-esr78?
Attachment #9194615 - Flags: approval-mozilla-esr78+
Attachment #9194615 - Flags: approval-mozilla-beta?
Attachment #9194615 - Flags: approval-mozilla-beta+

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.

Attachment #9194615 - Flags: approval-mozilla-esr78+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: