1463494 - Crash in nsTArray_Impl<T>::Clear | GeckoAppShellSupport::OnSensorChanged

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-71f7b3e5-b093-489b-a343-95f4b0180522.
=============================================================

Seen while looking at crash stats. Marking as security sensitive due to addresses: https://bit.ly/2IVsRnN. Crashes started using 20180522100454.

Possible regression window based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=226437245483e931aa1413c2db7668435f03bff9&tochange=dc1868d255be744a7d2d462216be205086cc60af

Top 10 frames of crashing thread:

0 libxul.so nsTArray_Impl<BroadcastListener*, nsTArrayInfallibleAllocator>::Clear 
1 libxul.so GeckoAppShellSupport::OnSensorChanged widget/android/nsAppShell.cpp:341
2 libxul.so mozilla::jni::detail::ProxyNativeCall<GeckoAppShellSupport, mozilla::java::GeckoAppShell, true, false, int, float, float, float, float, int, long long>::Call<true, false, 0, 1, 2, 3, 4, 5, 6> widget/android/jni/Natives.h:418
3 libxul.so mozilla::jni::detail::ProxyNativeCall<GeckoAppShellSupport, mozilla::java::GeckoAppShell, true, false, int, float, float, float, float, int, long long>::operator widget/android/jni/Natives.h:499
4 libxul.so mozilla::detail::RunnableFunction<mozilla::jni::detail::ProxyNativeCall<GeckoAppShellSupport, mozilla::java::GeckoAppShell, true, false, int, float, float, float, float, int, long long> >::Run xpcom/threads/nsThreadUtils.h:552
5 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1090
6 libxul.so NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
7 libxul.so mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
8 libxul.so MessageLoop::Run ipc/chromium/src/base/message_loop.cc:326
9 libxul.so nsBaseAppShell::Run widget/nsBaseAppShell.cpp:157

=============================================================

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Regression from bug 1425930 maybe?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

So a regression from there seems likely - the Clear() call was added in bug 1425930, though it replaced an implicit ~nsTArray<> of a local array.  However: the array is one of raw ptrs, not refcounted objects; on Clear() it should be doing little more than setting the Length() to 0 and maybe freeing the vector.  So how would this UAF?  The only obvious answer would be this is accessing this->mBroadcastCopy, which is the code we're running.  This comes from gSensorObservers[] in Hal.cpp.

You'd think that would be fine, and nothing would rip it out from under us... but in UnregisterSensorObserver (which calls UnregisterObserver(), which is what required us to handle inside Broadcast() in bug 1425930) it checks if there are any sensor observers left in any class, and if there are none left, it deletes the gSensorObservers array.  If this happened inside of Broadcast, this would reliably UAF now.  Before, we were freeing an object that was still in use and running, with dangerous possible side-effects, though depending on the code you may get away with it (and the code only accesses a member once, at the start, so until someone touches the Broadcast() code it would happen to not crash - and I made it touch a member, so Boom).

Safe solution: don't free gSensorObservers[] - 8 objects with two nsTArrays each.  I'm tempted to do that.  Slightly fancier solution would be to defer the free via a runnable to MainThread (and thus not within the Broadcast()).  froyd, snorp: preferences?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Randell Jesup [:jesup] from comment #2)
> 
> Safe solution: don't free gSensorObservers[] - 8 objects with two nsTArrays
> each.  I'm tempted to do that.  Slightly fancier solution would be to defer
> the free via a runnable to MainThread (and thus not within the Broadcast()).
> froyd, snorp: preferences?

I have not followed that bug very closely, but it seems like it'll be fine to take option 1 here. My guess is that we probably have the observer array allocated most of the time anyway.

Comment 4

[Nathan Froyd [:froydnj]]

I think it will be complicated to tell the leak checker that it's OK to leak gSensorObservers *and* the arrays contained therein.  That means we need to carefully release the list via the event loop.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I think it will be complicated to tell the leak checker that it's OK to leak
> gSensorObservers *and* the arrays contained therein.  That means we need to
> carefully release the list via the event loop.

Well, the arrays within the objects are all going to be empty, one presumes (we were only freeing this if all of them were empty).  snorp: And do we run the leak checker on Android?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #4)
> > I think it will be complicated to tell the leak checker that it's OK to leak
> > gSensorObservers *and* the arrays contained therein.  That means we need to
> > carefully release the list via the event loop.
> 
> Well, the arrays within the objects are all going to be empty, one presumes
> (we were only freeing this if all of them were empty).  snorp: And do we run
> the leak checker on Android?

AFAIK we don't, but I think Hal.cpp gets used on other platforms.

Comment 7

[Nathan Froyd [:froydnj]]

We certainly had problems with the patch from bug 1425930 on Linux, so Hal.cpp is used there.

nsTArray calls into MOZ_COUNT_CTOR/DTOR, so even if the arrays are empty, we won't call the matching dtor if we leak gSensorObservers, right?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: delete the sensor observerlist array in a deferred manner

if there's a better runnable-from-lambda that's not in media::, let me know.  Tried it on Linux, but nothing triggers it (likely no sensors get added)

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8979999 [details] [diff] [review]
delete the sensor observerlist array in a deferred manner

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

r=me with the NS_NewRunnableFunction change.

::: hal/Hal.cpp
@@ +24,5 @@
>  #include "mozilla/StaticPtr.h"
>  #include "mozilla/dom/ContentChild.h"
>  #include "mozilla/dom/ContentParent.h"
>  #include "mozilla/dom/ScreenOrientation.h"
> +#include "mozilla/media/MediaUtils.h"

Please make this nsThreadUtils.h.

@@ +447,5 @@
> +
> +  // We want to destroy gSensorObservers if all observer lists are
> +  // empty, but we have to defer the deallocation via a runnable to
> +  // mainthread (since we may be inside NotifySensorChange()/Broadcast()
> +  // when it calls UnregisterSensorObserver()).

Unsure if this comment paints a bulls-eye on the issue or not.

@@ +453,3 @@
>    gSensorObservers = nullptr;
> +
> +  NS_DispatchToMainThread(media::NewRunnableFrom([sensorlists]() {

Please use NS_NewRunnableFunction, like all code outside of media/ does.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

No bullseye, since until the regressing patch landed this was a latent bug (nothing touched "this" after the Broadcast loop started calling Notify)

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: delete the sensor observerlist array in a deferred manner

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8980036 [details] [diff] [review]
delete the sensor observerlist array in a deferred manner

carry forward r+

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Latent sec-high UAF which is not needed on ESR unless we touch the Broadcast code (as in the regressing bug here)

User impact if declined: Latent sec bug

Fix Landed on Version: 62

Risk to taking this patch (and alternatives if risky): Low risk

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: N/A long ago

[User impact if declined]: We'd have to fix bug 1425930 in some other manner

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: No

[Needs manual test from QE? If yes, steps to reproduce]: Unsure how to reproduce

[List of other uplifts needed for the feature/fix]: bug 1425930 

[Is the change risky?]: Not very

[Why is the change risky/not risky?]: simply defers freeing the memory

[String changes made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very Hard/impossible (bug is latent until bug 1425930)

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Somewhat, but no more so than the regressing bug, and no exploit of this is possible without the regressing bug.

Which older supported branches are affected by this flaw? all

If not all supported branches, which bug introduced the flaw? unknown (long ago)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial

How likely is this patch to cause regressions; how much testing does it need? Unlikely; minimal testing

Comment 13

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

Making this a sec-high.

sec-approval+ for trunk and I'll give the other approvals as well.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b837a2a32aebe36307641774f0960c8435082b1d

Before landing I ran an Android Try run, all green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8690f79d4dd693d41cbaaccc8f3a0c7fd40fb6e

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

leaks the runnable and array list: mochitest-plain-headless-e10s-8 on linux64 debug. 

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/89bd1716b6fa 

Probably the infamous "DispatchToMainThread() failure during shutdown" case. :-(

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up sensorlist if Dispatch fails

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8980405 [details] [diff] [review]
clean up sensorlist if Dispatch fails

Try issues

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up sensorlist if DispatchToMainThread fails

passes locally (the failing test is dom/system/tests/test_bug1197901.html).  Pushing try to verify.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c48c76887af092b57a83080134bc5f185e16983&group_state=expanded

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/494d77b62c333c2b71097d4ecf41c1a60dc495a2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/494d77b62c33

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f803318f71f
https://hg.mozilla.org/releases/mozilla-esr60/rev/a5ab68f0fdf0

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8980431 [details] [diff] [review]
clean up sensorlist if DispatchToMainThread fails

Canceling review on this, since jchen got to it before I did.

Comment 24

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

Why is this marked as Firefox 60 is unaffected but ESR60 is affected? 

I see we checked into Beta (61), which implies (with the ESR60 status) that Firefox 60 *is* affected. Comment 12 says "all" for what older branches are affected as well. Bug 1425930 is referenced, which affected Firefox 60 as well.

Comment 25

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

NI'ing Ryan since he marked the flags.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I blame the two being right next to each other on the drop-down.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Actually, it's probably because this first appeared to be a regression from bug 1425930, which was never uplifted to Fx60. But upon further inspection, it was a longstanding issue just exposed by that bug.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Correct

Comment 29

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

Thanks. Putting it in the roll up advisory.