1023712 - Likely UAF in webrtc::ViEEncoder::~ViEEncoder

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

I observed this crash on a try run:

https://tbpl.mozilla.org/php/getParsedLog.php?id=41485204&tree=Try

Looking at the trace, it seems that we're using a bad function pointer, and looking at the d'tor code, one possibility stands out:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_encoder.cc?from=~ViEEncoder&case=true#302

This is a virtual class, and if the object is gone, the vptr could be bad. Looking at the header:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_encoder.h#216

and 

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h#698

This code must be run prior to destruction of the PeerConnectionImpl, at the very least. Looking at what caused the teardown, I see the following:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#423

and 

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#580

I see nowhere that explicitly destroys a PipelineListener, so it would seem that this happens after the MediaPipeline is destroyed. Trouble is, this only happens in the back-and-forth dispatch caused by PeerConnectionImpl::CloseInt(), which is not guaranteed to be done prior to destruction of PeerConnectionImpl:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp?from=~PeerConnectionImpl&case=true#530

We need to ensure that this load manager lives at least as long as the conduits. It doesn't look terribly hard to add some refptrs.

Comment 1

[Byron Campen [:bwc]]

I should note that this UAF is of the function-pointer variety, which opens the potential for execution of arbitrary code.

Comment 2

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

This would likely be connected to the LoadManager enable

Comment 3

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

I have a WIP patch to change the loadmanager to a singleton service... which as a side-effect will help resolve these issues.

Comment 4

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

Attachment: WIP patch to make the LoadManager a singleton

could still use code to either suspend polling when there are no observers, and/or exit if we know no one is watch us (would probably need to separate out the users of the factory from the singleton)

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&rev=51b85ba6534c

Comment 6

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

Attachment: loadmanager

Now shuts down the LoadMonitor thread when we're not using LoadManager, and cleaned up the usage so that there's a wrapper object owned by VideoConduit to avoid any chance of a lifetime issue (and simplify the code a bunch).

I also found LoadManager wasn't locking access to the observer list and internal vars used from the callbacks.

r? pkerr for the webrtc:: interfaces and removal from PeerConnectionImpl/CodecConfig/etc, r? bsmedberg for the singleton stuff and locking.

https://tbpl.mozilla.org/?tree=Try&rev=092304389e94
(I've built and run it on Linux so far, not others).  The earlier WIP patch I ran a try on for Android and linux ASAN.

Comment 7

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

Attachment: loadmanager

Corrected a missing include not noticed due to unified builds

https://tbpl.mozilla.org/?tree=Try&rev=c71885798672
(But note Try is having infra issues!)

Comment 8

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

Try is green (with one typo fix from the above note):

https://tbpl.mozilla.org/?tree=Try&rev=72da728f5b71

Comment 9

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

Comment on attachment 8439747 [details] [diff] [review]
loadmanager

adding ehsan since I think bsmedberg may not be around

Comment 10

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

for Ehsan/Bsmedberg - your part of the review is the XPCOM/Gecko stuff:
* the singleton impl
* how it gets shut down in xpcom-shutdown
* the lock on the callbacks (which was missing!)
* starting/stopping the LoadMonitor depending on if there are any observers

Comment 11

[Benjamin Smedberg]

Comment on attachment 8439747 [details] [diff] [review]
loadmanager

>diff --git a/content/media/webrtc/LoadManager.cpp b/content/media/webrtc/LoadManager.cpp

>+LoadManagerSingleton::~LoadManagerSingleton()
> {
>   LOG(("LoadManager: shutting down LoadMonitor"));
>-  mLoadMonitor->Shutdown();
>+  if (mLoadMonitor) {
>+    mLoadMonitor->Shutdown();
>+  }
>+}

In this method mLoadMonitor should always be null, because we should always have reached xpcom-shutdown (right?). Can we assert that it's null here, even if you keep the shutdown check in release mode?

Everything else looks fine.

Comment 12

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

Comment on attachment 8439747 [details] [diff] [review]
loadmanager

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not easily but believable.  The change is a refactor, so it doesn't help point to an exploit - the oranges already on bug 1022235 really are what point at the problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not strongly.  We move from 1/peerconnection to a singleton; that doesn't paint a bullseye.  I add a lock on the callbacks; that's clear, but isn't easy to exploit - possible though, as with any cross-thread mis-locking.

Which older supported branches are affected by this flaw? Aurora 32.  31 has the code IIRC, but it's disabled by default.

If not all supported branches, which bug introduced the flaw?  Bug 877954 (13 Mar 2014)

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

How likely is this patch to cause regressions; how much testing does it need? I have a green try and did local testing.  Low but not zero chance of regressions, though exposure would be limited.  Also did 20 crashtest retriggers on Android and and Linux64 ASAN after I had it working (not in the final Try)

Note: I plan to land it on Bug 1022235 (the intermittent)

Comment 13

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

sec-approval+ for trunk and we should get this on Aurora as well to avoid shipping it. Please submit an Aurora patch for Aurora approval.

Comment 14

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

Comment on attachment 8439747 [details] [diff] [review]
loadmanager

Patch applies cleanly to Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1022212/877954

User impact if declined: sec-crit bug

Testing completed (on m-c, etc.): ready to land on m-c; local testing, green try

Risk to taking this patch (and alternatives if risky): Risk is fairly low; makes a service long-lived until shutdown.  Changes are well-contained (and remove a rather dangerous raw pointer from a bunch of places and instead use a tightly-tied object in an nsAutoPtr).  Alternative would be to disable it, which would be a problem as it's important for B2G 2.0

String or IDL/UUID changes made by this patch: none

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c40063b2088
and already landed on inbound/m-c (see bug 1022235)

Note: affects B2G v2.0 if that has already branched - ryan?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.0 is on Aurora until the end of the current cycle.

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking qe-verify- due to lack of test case / STR. Please feel free to ask for verification if you have more for us to work with. Thank you.