Closed Bug 1177764 Opened 4 years ago Closed 4 years ago

Possible use-after-free in APZCCallbackHelper

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: kats, Assigned: baku)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [post-critsmash-triage])

Attachments

(1 file)

The RemovePostRefreshObserver call at [1] happens during the iteration at [2]. This modifies the array while we're iterating it, leading to a possible use-after-free. This manifested for me as a child-process crash while running a mochitest. The code originally landed in bug 918288 in TabChild.cpp and was moved to APZCCallbackHelper.cpp at some point.

I'm writing a patch for this.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=ee20787262f2#639
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=f858f1ba0ea5#1825
Oh, I guess the use-after-free part of this was only introduced in bug 1175245, when the iteration was changed from an index-based one to a range-based one. Prior to that it would have just been a bug where a PostRefreshObserver potentially didn't get run when it was supposed to.
Using nsTObserverArray instead nsTArray for mPostRefreshObservers should fix the problem.
Attached patch observer.patchSplinter Review
Attachment #8626616 - Flags: review?(bugmail.mozilla)
Comment on attachment 8626616 [details] [diff] [review]
observer.patch

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

Sweet, thanks! This fixes the problem I was having but I'm not a layout peer so it might be good to get another reviewer as well.
Attachment #8626616 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8626616 [details] [diff] [review]
observer.patch

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Sweet, thanks! This fixes the problem I was having but I'm not a layout peer
> so it might be good to get another reviewer as well.

I'm happy to r+ this as well; looks good. Thanks!
Attachment #8626616 - Flags: review+
BTW: This doesn't need sec-approval before landing, since this only became a sec issue due to a recent mozilla-central-only checkin (bug 1175245, per comment 1 here).

Per https://wiki.mozilla.org/Security/Bug_Approval_Process :
> Core-security bug fixes should just be landed by a developer without any explicit approval if:
>   [...]
>   The bug is a recent regression on mozilla-central
>   (this means that the specific regressing check-in has been identified on mozilla-central)

So: let's get this in ASAP!
Assignee: bugmail.mozilla → amarchesini
Status: NEW → ASSIGNED
There's a couple of other arrays in nsRefreshDriver.h which also hold observers (mStyleFlushObservers and mLayoutFlushObservers at least); it might be a good idea to convert those as well, perhaps in a follow-up bug.
For those two arrays at least, it looks like we iterate in reverse to support removal-during-iteration; so I think we're OK there.
Calling this sec-critical, since this is use-after-free, per comment 1.
Keywords: sec-critical
Component: Panning and Zooming → Layout
(In reply to Daniel Holbert [:dholbert] from comment #8)
> For those two arrays at least, it looks like we iterate in reverse to
> support removal-during-iteration; so I think we're OK there.

In general, I don't think that iterating in reverse is something
that makes the iteration safe when the array is modified.

The reason the mStyleFlushObservers loop is safe is that we actually
loop over a local copy:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=f858f1ba0ea5#1664
and check if the value we're about to use is still present
in original array (line 1671).

Ditto for mLayoutFlushObservers here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=f858f1ba0ea5#1708

If there are other loops on these members that don't follow
that pattern then I suspect they are unsafe.  I couldn't find
any though.
(In reply to Mats Palmgren (:mats) from comment #12)
> (In reply to Daniel Holbert [:dholbert] from comment #8)
> > For those two arrays at least, it looks like we iterate in reverse to
> > support removal-during-iteration; so I think we're OK there.
> 
> In general, I don't think that iterating in reverse is something
> that makes the iteration safe when the array is modified.

It's safe as long as:

  - The modification is removing the element being currently
    iterated (which is the scenario Daniel was talking about).

  - The container class only reallocates when it grows, not
    when it shrinks, which is true of nsTArray. For such a
    container, removing an element only invalidates iterators
    to elements beyond the one removed (because it shifts
    those elements down), not elements before. If we're
    iterating in reverse, we'll only iterate over the elements
    before.
(In reply to Botond Ballo [:botond] from comment #13)
> It's safe as long as:
> 
>   - The modification is removing the element being currently
>     iterated (which is the scenario Daniel was talking about).

My point is that it's very rare that one can make such guarantees.
In general, if there are paths that modifies an array it usually
means also insertions, appends, or multiple removes, can occur.
So, in general, I think it's bad advice to say "just iterate in
reverse, it's safe".

>   - The container class only reallocates when it grows, not
>     when it shrinks, which is true of nsTArray.

I'm aware nsTArray currently isn't re-allocating on remove
(compacting) but I don't think it's an assumption that layout
code should make.  FWIW, nsTArray.h doesn't mention such an
invariant.

IMHO, it's an unsafe practice to iterate over an nsTArray that
can be modified and I would strongly advice against it.  Because
it's very rare that you can guarantee what modifications can occur.
And if you make a mistake it's very likely you have a sec-critical
bug.  It's just not worth the risk.
(In reply to Mats Palmgren (:mats) from comment #14)
> (In reply to Botond Ballo [:botond] from comment #13)
> > It's safe as long as:
> > 
> >   - The modification is removing the element being currently
> >     iterated (which is the scenario Daniel was talking about).
> 
> My point is that it's very rare that one can make such guarantees.
> In general, if there are paths that modifies an array it usually
> means also insertions, appends, or multiple removes, can occur.
> So, in general, I think it's bad advice to say "just iterate in
> reverse, it's safe".

I don't think anyone was proposing such general advice. The scenario under discussion (starting in comment 8) is the very specific one of "removal-during-iteration".

> >   - The container class only reallocates when it grows, not
> >     when it shrinks, which is true of nsTArray.
> 
> I'm aware nsTArray currently isn't re-allocating on remove
> (compacting) but I don't think it's an assumption that layout
> code should make.  FWIW, nsTArray.h doesn't mention such an
> invariant.

That's true, though I think that's more likely an oversight than a deliberate choice to not provide iterator invalidation guarantees. I filed bug 1178146 about this.

By comparison, the C++ standard specifies precise iteration invalidation semantics for methods of standard container classes like std::vector.
People have expressed concern about security bugs like this as a result of using ranged-for loops (and perhaps even STL iterators in general?); it is both encouraging and unfortunate that they were right.

I sympathize with the desire to see the iterator invalidation semantics documented, but I think the code author's motivation, never mind the reviewer's, to trace through call chains to ensure that iterator invalidation semantics are respected is going to be slim to none.
https://hg.mozilla.org/mozilla-central/rev/94e64d1a94b0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [post-critsmash-triage]
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.