Closed Bug 1382215 Opened 4 years ago Closed 4 years ago
Crash in Reentrant
Monitor Auto Enter called from Async Pan Zoom Controller::Get Current Async Transform()
This bug was filed from the Socorro interface and is report bp-ef389076-e5e9-4a40-b286-498d90170719. ============================================================= Clear UAF in 55bN and 56, when called from AsyncPanZoomController::GetCurrentAsyncTransform(). Current search for crashes: (29 in 56/55b8/9/10 in the last week with UAF signatures: https://crash-stats.mozilla.com/signature/?product=Firefox&version=56.0a1&version=55.0b&version=55.0b10&version=55.0b9&version=55.0b8&address=~e5e5&proto_signature=~AsyncPanZoomController%3A%3AGetCurrentAsyncTransform&signature=mozilla%3A%3AReentrantMonitorAutoEnter%3A%3AReentrantMonitorAutoEnter&date=%3E%3D2017-07-12T13%3A40%3A00.000Z&date=%3C2017-07-19T13%3A40%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1
Group: core-security → gfx-core-security
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  gets destroyed before it is used at . 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.  http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/apz/src/APZCTreeManager.cpp#2036  http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/layers/apz/src/APZCTreeManager.cpp#1058
Assignee: nobody → bugmail
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.
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?
Attachment #8888837 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/96cfe82f2bfee15c2cfbf66585df037720f580a8 This grafts cleanly to Beta as well.
Comment on attachment 8888837 [details] [diff] [review] Speculative fix apz crash/uaf fix for beta55, should be in 55.0b12
Attachment #8888837 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.