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

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

The problem was originally introduced in bug 1335895, which landed in firefox 55. So it goes back a few releases.

Comment 4

[Randall Barker [:rbarker]]

Working on a patch. Maybe GetCompositorBridgeParentFromLayersId should assert if called off of the compositor thread?

Comment 5

[Randall Barker [:rbarker]]

Attachment: Patch for uplift.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 8

[Randall Barker [:rbarker]]

Attachment: Rebased patch for m-c

This is the rebased patch for m-c. Leaving previous patch for uplift.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 11

[Botond Ballo [:botond]]

Why not just keep the code on the controller thread and store the CBP in a RefPtr?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[Frederik Braun [:freddy]]

Kats, rbarker, any news here? :-)

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I have an idea on how to fix this

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: 1-rawptr-aa0bee39b1ac

Comment 16

[Randall Barker [:rbarker]]

Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

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

Thanks for fixing this!

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a58f78f574e7a081b3ffb4484cae0dfa8b558ed

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9a58f78f574e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Beta and ESR60 approval on this when you get a chance. It grafts cleanly to both as-landed.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a53ff7808556

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8972911 [details] [diff] [review]
1-rawptr-aa0bee39b1ac

apz sec fix for 60.1esr

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5a7eef7de073

Comment 29

[Daniel Veditz [:dveditz]]

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.