Closed Bug 1443436 Opened 3 years ago Closed 3 years ago

Android dynamic toolbar code uses raw pointer to APZCTreeManager off the compositor thread (was getting CompositorBridgeParent in older versions)

Categories

(Firefox for Android Graveyard :: Toolbar, defect, P1)

57 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr6061+ fixed, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61+ fixed, firefox62+ fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ fixed
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: kats, Assigned: kats)

Details

(Keywords: csectype-uaf, sec-audit, Whiteboard: [adv-main61-][adv-esr60.1-])

Attachments

(1 file, 2 obsolete files)

Stack:

0  libxul.so!mozilla::layers::CompositorBridgeParent::GetCompositorBridgeParentFromLayersId [CompositorBridgeParent.cpp:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 1207 + 0x0]
1  libxul.so!mozilla::layers::AndroidDynamicToolbarAnimator::ProcessTouchDelta [AndroidDynamicToolbarAnimator.cpp:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 636 + 0x3]
2  libxul.so!mozilla::layers::AndroidDynamicToolbarAnimator::ReceiveInputEvent [AndroidDynamicToolbarAnimator.cpp:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 194 + 0x7]
3  libxul.so!mozilla::layers::APZCTreeManager::ReceiveInputEvent [APZCTreeManager.cpp:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 1111 + 0x5]
4  libxul.so!nsWindow::NPZCSupport::HandleMotionEvent [nsWindow.cpp:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 726 + 0x7]
5  libxul.so!mozilla::jni::NativeStub<mozilla::java::NativePanZoomController::HandleMotionEvent_t, nsWindow::NPZCSupport, mozilla::jni::Args<int, int, long long, int, const mozilla::jni::Ref<mozilla::jni::TypedObject<jintArray>, _jintArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &, const mozilla::jni::Ref<mozilla::jni::TypedObject<jfloatArray>, _jfloatArray *> &> >::Wrap<&nsWindow::NPZCSupport::HandleMotionEvent> [Natives.h:35ae7396d27dabd6f0c72c97c79ecdb358a79b70 : 648 + 0x1f]

GetCompositorBridgeParentFromLayersId returns a raw pointer and this code uses it off the compositor thread. In theory the CompositorBridgeParent might get torn down concurrently resulting in a UAF or other badness.
My patches for bug 1443301 fix this as a side-effect. I'd dupe this over but since this is potentially a sec issue I'll just close it as WFM to avoid malicious types from looking at bug 1443301 too closely.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
My patches for bug 1443301 changed, and this is still an issue. The code is now just using a raw mApz on the controller thread, and the mApz could get destroyed on the compositor thread while it's doing that.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee: nobody → bugmail
Priority: -- → P1
The problem was originally introduced in bug 1335895, which landed in firefox 55. So it goes back a few releases.
Assignee: bugmail → rbarker
Working on a patch. Maybe GetCompositorBridgeParentFromLayersId should assert if called off of the compositor thread?
Attached patch Patch for uplift. (obsolete) — Splinter Review
Attachment #8957282 - Flags: review?(bugmail)
Comment on attachment 8957282 [details] [diff] [review]
Patch for uplift.

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

Looks fine to me. The code looks different on master (as of bug 1443301 which just landed) so you'll need to rebase this for landing on m-c. But this patch can be used for the backports.
Attachment #8957282 - Flags: review?(bugmail) → review+
(In reply to Randall Barker [:rbarker] from comment #4)
> Working on a patch. Maybe GetCompositorBridgeParentFromLayersId should
> assert if called off of the compositor thread?

And yeah that would be good. I can do that separately if you don't feel like adding that to this patch.
Attached patch Rebased patch for m-c (obsolete) — Splinter Review
This is the rebased patch for m-c. Leaving previous patch for uplift.
Attachment #8957337 - Flags: review?(bugmail)
Attachment #8957282 - Attachment description: 0001-Bug-1443436-Ensure-GetCompositorBridgeParentFromLayersId-is-called-in-compositor-thread-in-Toolbar-Animator-r-kats-18030811-939a5277b154.patch → Patch for uplift.
Comment on attachment 8957337 [details] [diff] [review]
Rebased patch for m-c

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

Commit message needs updating, but otherwise looks good
Attachment #8957337 - Flags: review?(bugmail) → review+
Comment on attachment 8957337 [details] [diff] [review]
Rebased patch for m-c

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

Oh shoot, I just realized this patch just swaps one problem for another: the actual call to ProcessTouchVelocity needs to happen on the controller thread, since the implementation of that function uses mApzcForInputBlock which is controller-thread-only.
Attachment #8957337 - Flags: review+ → review-
Why not just keep the code on the controller thread and store the CBP in a RefPtr?
So right now GetCompositorBridgeParentFromLayersId returns a rawptr, we'd have to either make that return a RefPtr, or use the CallWithIndirectLayerTree helper to get it as a refptr. I think that would work fine for 59 and older (although CallWithIndirectLayerTree is only available on 59). For 60/current m-c we're not using the CBP at all, and I think the equivalent change would be to keep a RefPtr to the APZCTreeManager. This would probably be simpler if we can explicitly define the AndroidDynamicToolbarAnimator class to be owned by the APZCTreeManager, and make the UiCompositorControllerParent drop its refptr during APZCTreeManager::ClearTree or something, so we are guaranteed that the AndroidDynamicToolbarAnimator doesn't outlive the APZCTM.
Kats, rbarker, any news here? :-)
Flags: needinfo?(rbarker)
Flags: needinfo?(bugmail)
I have an idea on how to fix this
Assignee: rbarker → bugmail
Flags: needinfo?(rbarker)
Flags: needinfo?(bugmail)
Attachment #8957282 - Attachment is obsolete: true
Attachment #8957337 - Attachment is obsolete: true
Attachment #8972911 - Flags: review?(rbarker)
Summary: Android dynamic toolbar code gets raw pointer to CompositorBridgeParent off the compositor thread → Android dynamic toolbar code uses raw pointer to APZCTreeManager off the compositor thread (was getting CompositorBridgeParent in older versions)
Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

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

Thanks for fixing this!
Attachment #8972911 - Flags: review?(rbarker) → review+
Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, if at all. I think it would involve the user physically touching the screen during Fennec shutdown and I'm not sure if the cleanup sequence allows the bug to exploited at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Sort of, yeah - it's clear the problem is dereferencing a raw pointer on one thread which might be freed on another thread

Which older supported branches are affected by this flaw?
55 and up

If not all supported branches, which bug introduced the flaw?
It was introduced in bug 1335895 which landed in 55. The code was slightly different then but the fundamental problem was the same.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I haven't written the backport yet but it should be similar to the attached patch. Pretty low risk.

How likely is this patch to cause regressions; how much testing does it need?
Very low risk of regressions, basic testing of scrolling around with the dynamic toolbar enabled (which is the default behaviour) should be sufficient.
Attachment #8972911 - Flags: sec-approval?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> I haven't written the backport yet but it should be similar to the attached
> patch. Pretty low risk.

So the patch I attached to this bug will apply as-is to 60 onwards. And since 60 we're about to ship 60 I don't think the patch needs backporting at all, we can just apply it to the 60 release and ESR branches whenever we're ready to ship this fix.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Not easily, if at all. I think it would involve the user physically touching
> the screen during Fennec shutdown and I'm not sure if the cleanup sequence
> allows the bug to exploited at all.

... and after thinking about this some more I realized that the scenario is actually impossible, because the code in question (AndroidDynamicToolbarAnimator::ProcessTouchDelta) only ever gets called from APZCTreeManager::ReceiveInputEvent, so obviously whoever is calling APZCTreeManager::ReceiveInputEvent will be having a reference to the APZCTreeManager and keeping it alive. So there's no bug here, except for confusing code.

I'm happy to just close this bug, or replace this patch with one that just documents what's going on. Or we can land the patch anyway.
Attachment #8972911 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/9a58f78f574e
Group: firefox-core-security → core-security-release
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Please request Beta and ESR60 approval on this when you get a chance. It grafts cleanly to both as-landed.
Flags: needinfo?(bugmail)
Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

Approval Request Comment
[Feature/Bug causing the regression]: bug 1335895
[User impact if declined]: in practice nothing, but the code looks like it subject to a use-after-free race condition
[Is this code covered by automated tests?]: not really
[Has the fix been verified in Nightly?]: yes
[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?]: no
[Why is the change risky/not risky?]: pretty simple change, just passing a RefPtr around to make it more obvious that we're not doing a UAF
[String changes made/needed]: none
Flags: needinfo?(bugmail)
Attachment #8972911 - Flags: approval-mozilla-esr60?
Attachment #8972911 - Flags: approval-mozilla-beta?
Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

Low-risk fix, approved for 61.0b4. We'll look at the ESR60 requests later in the cycle.
Attachment #8972911 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

apz sec fix for 60.1esr
Attachment #8972911 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
If this is a Fennec/GeckoView android bug I'm not sure why we needed it on esr-60. Given the comments about exploitability I'll call this more of a sec-audit.
Keywords: sec-highsec-audit
Whiteboard: [adv-main61-][adv-esr60.1-]
Group: core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.