Closed Bug 1382215 Opened 4 years ago Closed 4 years ago

Crash in ReentrantMonitorAutoEnter called from AsyncPanZoomController::GetCurrentAsyncTransform()

Categories

(Core :: Panning and Zooming, defect)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 + fixed

People

(Reporter: jesup, Assigned: kats)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file, 1 obsolete file)

Flags: needinfo?(jmathies)
Group: core-security → gfx-core-security
Keywords: testcase-wanted
Component: Graphics → Panning and Zooming
tracking as sec-high
I don't know this code, Kats, any ideas?
Flags: needinfo?(jmathies) → needinfo?(bugmail)
Looking at the code, it might be that the out-param we return at [1] gets destroyed before it is used at [2]. That could result in us calling GetCurrentAsyncTransformWithOverscroll on a garbage APZC instance, which would result in the crash in ReentrantMonitorAutoEnter. In general any pointers to APZC or HitTestingTreeNodes outside of a treelock should be refptrs and not raw pointers.

If my theory is correct, this would be a regression from bug 1349750. I can write a patch.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/apz/src/APZCTreeManager.cpp#2036
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/apz/src/APZCTreeManager.cpp#1058
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
Attached patch Speculative fix (obsolete) — Splinter Review
Attachment #8888811 - Flags: review?(botond)
Also, is there a way to search/filter crash reports by additional stack frames? Right now the ReentrantMonitorAutoEnter signature matches a lot of other stacks too and I'd like to filter to just see the ones coming from AsyncPanZoomController::GetCurrentAsyncTransform - it will help verify the regression range and the fix.
I guess the other query in comment 0 does it, just need to widen the date/version ranges a bit. Definitely seems like this UAF was introduced in 55 so it's consistent with being a regression from 1349750.
Comment on attachment 8888811 [details] [diff] [review]
Speculative fix

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

Thanks for looking into this!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +2032,5 @@
>  
>  already_AddRefed<AsyncPanZoomController>
>  APZCTreeManager::GetTargetAPZC(const ScreenPoint& aPoint,
>                                 HitTestResult* aOutHitResult,
> +                               RefPtr<HitTestingTreeNode>* aOutHitScrollbar)

|aOutHitScrollbar| is the old name of this parameter from back when it was a bool. Let's take this opportunity to rename it to its proper name, |aOutScrollbarNode| (I already did that in the declaration, just forgot to do it here in the definition).

@@ +2041,4 @@
>    ParentLayerPoint point = ViewAs<ParentLayerPixel>(aPoint,
>      PixelCastJustification::ScreenIsParentLayerForRoot);
>    RefPtr<AsyncPanZoomController> target = GetAPZCAtPoint(mRootNode, point,
> +      &hitResult, &hitScrollbar);

Since we're passing in a non-null pointer here, and this is the only call site, we can omit the null check of |aOutScrollbarNode| inside GetAPZCAtPoint().
Attachment #8888811 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #7)
> |aOutHitScrollbar| is the old name of this parameter from back when it was a
> bool. Let's take this opportunity to rename it to its proper name,
> |aOutScrollbarNode| (I already did that in the declaration, just forgot to
> do it here in the definition).

Will do.

> >    RefPtr<AsyncPanZoomController> target = GetAPZCAtPoint(mRootNode, point,
> > +      &hitResult, &hitScrollbar);
> 
> Since we're passing in a non-null pointer here, and this is the only call
> site, we can omit the null check of |aOutScrollbarNode| inside
> GetAPZCAtPoint().

Ok.
Attached patch Speculative fixSplinter Review
Carrying r=botond
Attachment #8888811 - Attachment is obsolete: true
Attachment #8888837 - Flags: review+
Comment on attachment 8888837 [details] [diff] [review]
Speculative fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Not very easily, I think. It involves a race condition between layer tree updates (which are throttled by gecko) and user input (which in theory the attacker doesn't control)

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- No, but it wouldn't be hard to be realize that upgrading a raw pointer to a RefPtr is intended to fix a UAF problem.

Which older supported branches are affected by this flaw?
- No released versions; 55 beta and newer.

If not all supported branches, which bug introduced the flaw?
- Bug 1349750

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- Not yet, but this patch should apply to 55 pretty easily.

How likely is this patch to cause regressions; how much testing does it need?
- Unlikely to cause regressions. Any breakages should be obvious upon exercising the code (by doing a scrollbar drag).
Attachment #8888837 - Flags: sec-approval?
Comment on attachment 8888837 [details] [diff] [review]
Speculative fix

Approval Request Comment
[Feature/Bug causing the regression]: bug 1349750
[User impact if declined]: Possible crash
[Is this code covered by automated tests?]: Some of it is
[Has the fix been verified in Nightly?]: not yet, hasn't landed yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: pretty small change, effectively upgrading a raw pointer to a RefPtr
[String changes made/needed]: none
Attachment #8888837 - Flags: approval-mozilla-beta?
Comment on attachment 8888837 [details] [diff] [review]
Speculative fix

sec-approval+ for trunk.

Ritu, can we take this on Beta?
Flags: needinfo?(rkothari)
Attachment #8888837 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/96cfe82f2bfe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8888837 [details] [diff] [review]
Speculative fix

apz crash/uaf fix for beta55, should be in 55.0b12
Flags: needinfo?(rkothari)
Attachment #8888837 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1383722
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.