Closed Bug 991285 Opened 6 years ago Closed 6 years ago

nsRenderingContext is inconsistently refcounted (some code uses nsRefPtr<nsRenderingContext>, some uses stack instances), which is a double-free waiting to happen

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: sec-audit, sec-low, Whiteboard: [adv-main31-])

Attachments

(2 files, 5 obsolete files)

nsRenderingContext is a refcounted class:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.h#47
...and is wrapped in nsRefPtr in various places:
http://mxr.mozilla.org/mozilla-central/search?string=nsRefPtr<nsRenderingContext

However, in nsCanvasFrame, we simply declare an instance on the stack and then pass a pointer to it to a helper-function:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsCanvasFrame.cpp#218

This is dangerous, because if anything that we pass it to wraps it in a nsRefPtr (as it should be able to expect it's allowed to do), then we'll get a double-free.

From some quick skimming, I didn't see any code that actually does this (wraps the passed-in nsRenderingContext in a nsRefPtr), but that doesn't meant that such code doesn't exist. Hence, filing as a security bug to be on the safe side.

(Discovered while addressing bug 984786 in /gfx and giving nsRenderingContext a private constructor, which turns this bogus stack instance into a compile error)
Looks like this instance was added in one of the DLBI patches:
  http://hg.mozilla.org/mozilla-central/rev/7a376ff3ae84#l3.40
Assignee: nobody → dholbert
Blocks: dlbi
Status: NEW → ASSIGNED
Attached patch fix v2 (obsolete) — Splinter Review
Actually, we don't need to allocate the nsRenderingContext until the clause where we Init() it.

(I was initially worried about allocating it inside of the '#ifdnef' block here, in case that meant we'd pass a null pointer to PaintInternal when that block is preprocessed away. That won't be a problem, though -- if the block is preprocessed away, then "(surf || dt)" will be false, since neither will be initialized, and so we'll pass aCtx instead of context to PaintInternal.)
Attachment #8400863 - Attachment is obsolete: true
Attachment #8400863 - Flags: review?(roc)
Attachment #8400869 - Flags: review?(roc)
Actually, nsFilterInstance has a few of these, too. Patch coming up to fix those as well.
Summary: nsCanvasFrame creates an instance of refcounted class 'nsRenderingContext' on the stack → nsCanvasFrame and nsFilterInstance creates an instance of refcounted class 'nsRenderingContext' on the stack
Attachment #8400869 - Attachment is obsolete: true
Attachment #8400869 - Flags: review?(roc)
Attached patch fix v4 (obsolete) — Splinter Review
Sorry for review-spam; apparently there are a few more of these that I missed. I think I caught them all now (as verified by being able to build successfully after giving nsRenderingContext a private destructor).

This is a bunch of new heap allocations, though. :-/ Maybe instead of this, we should make nsRenderingContext non-refcounted? (not sure if that's feasible in general, but if it is, it would make all of the code touched by this patch kosher...)
Attachment #8400895 - Flags: feedback?(roc)
From attempting to make it non-refcounted, it looks like most of the refcounting is to support PresShell::GetReferenceRenderingContext(), which returns a freshly-allocated/initialized already_AddRefed nsRenderingContext, which also has had "Scale()" called on it if we're printing.

Two observations:
 (A) For some of the nsRenderingContext instances touched by the patches that I've attached so far, I suspect we really should be using GetReferenceRenderingContext() instead of a dumb local stack-variable... (We definitely should if we exercise them during printing.)

 (B) It looks like GetReferenceRenderingContext() doesn't really need to return an already_AddRefed heap-allocated value -- its callers only use the returned value briefly, so GetReferenceRenderingContext() could return a reference to a temporary (and use C++'s returning-reference-to-temporary behavior to let the caller keep this nsRenderingContext around until it returns). That would save us some heap allocation and let us be consistent about how nsRenderingContext lifetime is managed.
Attachment #8400895 - Attachment is obsolete: true
Attachment #8400895 - Flags: feedback?(roc)
Attachment #8400884 - Attachment is obsolete: true
Attachment #8400884 - Flags: review?(roc)
Summary: nsCanvasFrame and nsFilterInstance creates an instance of refcounted class 'nsRenderingContext' on the stack → nsCanvasFrame and various SVG classes create instances of refcounted class 'nsRenderingContext' on the stack
So GetReferenceRenderingContext() actually can't return a reference-to-temporary, because that only works if the returned reference is 'const', and most things that deal with a nsRenderingContext take a non-const pointer. (It's possible that we can just do a near-global constification, but I'm betting some code might actually need to modify it.)

We also probably can't just directly return a nsRenderingContext and expect to benefit from return value optimization, since GetReferenceRenderingContext() is a virtual method, which means the caller doesn't necessarily know what the method-implementation will be (which I'm assuming breaks RVO).

So we might need to change GetReferenceRenderingContext() to InitReferenceRenderingContext(nsRenderingContext&), and have it simply initialize a nsRenderingContext that the caller has declared.
Looks like we could benefit from RVO after all (virtualness doesn't impact that).

So we could either have:
  nsRenderingContext GetReferenceRenderingContext();
or:
  void InitReferenceRenderingContext(nsRenderingContext& aRenderingContext);
in ns[I]PresShell.

roc, any preference / any other thoughts on this bug?
Flags: needinfo?(roc)
Summary: nsCanvasFrame and various SVG classes create instances of refcounted class 'nsRenderingContext' on the stack → nsRenderingContext is inconsistently refcounted (some code uses nsRefPtr<nsRenderingContext>, some uses stack instances), which is a double-free waiting to happen
I think we should just take the extra heap allocations.

Then, on top of that, convert some of these to use GetReferenceRenderingContext.
Flags: needinfo?(roc)
Keywords: sec-audit
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I think we should just take the extra heap allocations.

Cool. I'll do this part; that makes this pretty straightforward.

> Then, on top of that, convert some of these to use
> GetReferenceRenderingContext.

I'm not as sure about this part. After a bit more poking, it looks like all of the stack-allocated nsRenderingContext instances are intentionally *not* using GetReferenceRenderingContext() because they want to call nsRenderingContext::Init() with their own custom args (e.g. with a specially-created drawtarget and/or gfxContext), instead of letting GetReferenceRenderingContext() call Init internally.

It's possible that some of this custom stuff is unnecessary, but it requires a bit of code-reading / hg archeology to be sure, and I'm not sure it's high enough priority to bother investing in too much (unlike the first part of this bug, which is potentially a sec vuln).  So I'm going to punt on this second part to a followup bug.
(same patch again, now w/ more lines of context)
Attachment #8405677 - Attachment is obsolete: true
Attachment #8405677 - Flags: review?(roc)
Attachment #8405678 - Flags: review?(roc)
Attachment #8405677 - Attachment description: part 1: consistently allocate nsRenderingContext on heap & store in nsRefPtr → part 1: (oops, only 3 lines of context) consistently allocate nsRenderingContext on heap & store in nsRefPtr
This patch adds a private destructor and MOZ_FINAL (the bug 984786 treatment) to verify that this is fixed (& keep it fixed).
Attachment #8405679 - Flags: review?(roc)
Comment on attachment 8405678 [details] [diff] [review]
part 1: consistently allocate nsRenderingContext on heap & store in nsRefPtr

Review of attachment 8405678 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: layout/generic/nsCanvasFrame.cpp
@@ +214,5 @@
>    nsCanvasFrame* frame = static_cast<nsCanvasFrame*>(mFrame);
>    nsPoint offset = ToReferenceFrame();
>    nsRect bgClipRect = frame->CanvasArea() + offset;
>  
> +  nsRefPtr<nsRenderingContext> context(new nsRenderingContext());

Move the allocation of 'context' down to where we also Init it below. This nsRenderingContext is very rarely used but this function is very frequently called.
Attachment #8405678 - Flags: review?(roc) → review+
I'm marking this as "sec-low", since AFAICT there's no way for this bug to cause problems currently.

In particular: Problematic code would be from a case where we have
  nsRenderingContext ctx;
  DoStuff(&ctx);
where DoStuff (or something that it calls) includes:
  nsRefPtr<nsRenderingContext> myCtx = aCtx;

I did a local grep for nsRefPtr<nsRenderingContext> and found only one instance of this pattern, which turned out to be non-problematic because in practice, the nsRenderingContext* passed there is always null. (So I removed the argument entirely in bug 996319.)

In every other occurrence of nsRefPtr<nsRenderingContext>, we immediately set it to one of the following...
 a) new nsRenderingContext
 b) nsDeviceContext::CreateRenderingContext()
 c) nsPresShell::GetReferenceRenderingContext (which I'm renaming to CreateReferenceRenderingContext in bug 996351)
...and *not* to a passed-in pointer value.

So I think this should be safe to land (i.e. it won't zero-day us), and it isn't particularly important to invest time in backporting this, though if the patch applies cleanly, I suppose we might as well backport it.
Keywords: sec-low
Comment on attachment 8405678 [details] [diff] [review]
part 1: consistently allocate nsRenderingContext on heap & store in nsRefPtr

Requesting sec-approval, to be on the safe side, from a process perspective.

See previous comment for why I think this should be fine (security-wise) to land (and probably to just un-hide, really).
Attachment #8405678 - Flags: sec-approval?
Comment on attachment 8405678 [details] [diff] [review]
part 1: consistently allocate nsRenderingContext on heap & store in nsRefPtr

This looks fine to land by me. I'll defer to Dan about unhiding it but nothing in this seems to raise a red flag.
Attachment #8405678 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/22cf932abe41
https://hg.mozilla.org/mozilla-central/rev/920ab5ed8ee5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Group: layout-core-security → core-security
How far back does this issue go?
"This issue" is really a *pattern* which occured in various places in the code. Each of those places presumably has a different ancestry, so "how far back does this issue go" doesn't have a clear answer

There were 7 distinct places fixed in the first patch here, so to answer the question completely, one would need to do 7 independent threads of hg archeology. Given that none of the usages fixed here actually seem to be dangerous (per comment 16), I'm not sure it's worth doing that archeology. (If you disagree, perhaps you wouldn't mind doing it? :))

The first instance fixed by this bug (in nsCanvasFrame.cpp) dates back to July 2012, in this cset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a376ff3ae84#l3.40

The ancestry of the others can be excavated if/as-needed.
(In reply to Daniel Holbert [:dholbert] from comment #22)
> The first instance fixed by this bug (in nsCanvasFrame.cpp) dates back to
> July 2012, in this cset:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7a376ff3ae84#l3.40

(first as in "top of the patch", not as in "of most ancient origin")
Whiteboard: [adv-main31-]
Marking [qa-] since this stems from a code audit without a concrete exploit attached to it. If a test case appears or if further testing is desired, let us know and we'll gladly help.
QA Whiteboard: [qa-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.