Closed Bug 1195968 Opened 4 years ago Closed 4 years ago

Check how CanvasFilterChainObserver accesses the CanvasRenderingContext2D


(Core :: Canvas: 2D, defect)

Not set



Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox-esr45 --- unaffected


(Reporter: mstange, Assigned: mstange)



(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage] sec-high if not fixed before pref on by default (bug 1173545))


(2 files, 2 obsolete files)

This needs to be checked:

(In reply to Mats Palmgren (:mats) from comment #25)
> I would also like to know more about CanvasFilterChainObserver.
> It has a raw CanvasRenderingContext2D* and calls UpdateFilter() on it,
> which seems unsafe unless we carefully tear these observers down
> before it goes away.  Where do we do that?

It's not immediately security critical because we're not creating CanvasFilterChainObserver unless canvas filters are enabled (using the pref canvas.filters.enabled), which they aren't currently by default.

Enabling them by default is tracked in bug 1173545; we need to fix this bug before we do that.
Whiteboard: sec-high if not fixed before pref on by default (bug 1173545)
Group: core-security → gfx-core-security
This makes sure we free CanvasFilterChainObservers properly. Should solve security concerns and fixes related memory leaks.
Attachment #8738903 - Flags: review?(mstange)
Comment on attachment 8738903 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers

I'm going to hand this review off to Mats.

Mats, the idea is that the observer is always torn down before the CanvasRenderingContext2D goes away. The only strong reference to the CanvasFilterChainObserver is the RefPtr in CanvasRenderingContext2D::ContextState. There is at least one other weak reference to it, in nsSVGFilterReference::mFilterChainObserver.

I'm not sure if this enough to address your concern, though. Should we make CanvasFilterChainObserver::mContext and nsSVGFilterReference::mFilterChainObserver RefPtrs, instead of raw pointers, and then add cycle collection unlink methods for those fields? I don't really know much about cycle collection so I could use some help here.

Bug 1062832 comment 7 is where I added the initial cycle collection code for these things.
Attachment #8738903 - Flags: review?(mstange) → review?(mats)
Thanks for the object graph, that really helps!

After plowing through the code there's one issue I'm concerned with: nsSVGIDRenderingObserver
(which inherits nsSVGFilterReference; calls our CanvasFilterChainObserver::DoUpdate()).
The nsSVGIDRenderingObserver is a mutation observer, and it calls Add[Remove]MutationObserver
in its ctor/dtor.  So we *depend on* the dtor being called before the mContext object
(CanvasRenderingContext2D*) is being destroyed.  AFAICT, there are no other strong refs
to these objects so deleting the CanvasRenderingContext2D should also destroy these
observer objects.

It's a bit fragile though, and there's a lot of complex code involved.  One small mistake,
like holding a RefPtr on the stack to a ChainObserver or nsSVGFilterReference while
calling a method that might flush, could lead to a use-after-free.  It might be worth
adding some defense / assertion against this.

We could for example add a DetachFromContext() method on CanvasFilterChainObserver
that nulls out mContext, calling it from the CC unlink phase.  Then add a runtime abort
if mContext is null in DoUpdate().  We shouldn't get any observer callbacks beyond
that point, IIUC.
Comment on attachment 8738903 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers

LGTM, but it wouldn't hurt with some extra precautions as suggested in
the last paragraph above.  r=mats
Attachment #8738903 - Flags: review?(mats) → review+
Addressed review comments. Mats, please have a final look, since I had to move some code around.
Attachment #8738903 - Attachment is obsolete: true
Attachment #8739499 - Flags: review?(mats)
Comment on attachment 8739499 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers (v2)

+    if (!mContext)
+      return;

nit: please add {}

Is there any reason to believe this can actually happen?

I don't see any, so I think we can replace the "return" with a MOZ_CRASH,
but we could start with a MOZ_ASSERT_UNREACHABLE before the return if
we're not sure yet.
Attachment #8739499 - Flags: review?(mats) → review+
Thanks for the reviews Mats! I changed the check for mContext to result in a MOZ_CRASH if null, since this should absolutely never happen. One last question here: Does this bug need to be made public before landing the patches?
Attachment #8739499 - Attachment is obsolete: true
Flags: needinfo?(mats)
sec-moderate bugs doesn't need approval before landing, so feel free to land.

We generally never open security bugs before landing, so you should
leave this one closed.  Someone will eventually open it once the fix
has trickled down to all supported branches.
Flags: needinfo?(mats)
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: gfx-core-security → core-security-release
Whiteboard: sec-high if not fixed before pref on by default (bug 1173545) → [post-critsmash-triage] sec-high if not fixed before pref on by default (bug 1173545)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.