1247236 - UAF in Cameras Shutdown on channel errors

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Gian-Carlo Pascutto [:gcp]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1244493#c9

This comment is wrong if Shutdown() is called from the destructor before the Cameras/PBackground thread has been shut down:
https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/dom/media/systemservices/CamerasChild.cpp#518

This is not an owning reference (see https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/dom/media/systemservices/CamerasChild.h#106)

This case occurs if the protocol handlers get shut down due to an IPC error.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

We're calling a virtual function in previously feed memory. As far as I know, this is fatal because an attacker can construct his own vtable and manipulate the freed memory to point to a function of his choosing. So that's RCE.

It looks like you need to win a race between the deleteRunnable being dispatched, and the object being freed. Bug 1244493 shows that you can win that race from web content, and I'm guessing the timing is going to be quite reproducible. So we lose again.

The question now is if you can trigger the channel error that forces the broken shutdown and then still write the right stuff in (just freed) memory, while the relevant code is being shut down. That's not going to be easy, but I don't see why it wouldn't be possible.

So, this looks rather serious.

Comment 2

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

I can't see bug 1244493, but this sounds like a likely sec-crit even if it's very hard to leverage

Comment 3

[Gian-Carlo Pascutto [:gcp]]

I just realized that from the original description, provoking the unexpected shutdown is done by Ctl-C-ing the browser. That's not something web content can do (unless we have more bugs) so maybe this isn't quite so serious.

Comment 4

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

What if the Content process crashes, which might be provokable (especially via a null-deref)?  However, leveraging that would be very tough due to timing.  Very.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

The UAF is on the content/child side of the IPC link, so crashing content doesn't help you :-)

Comment 6

[Gian-Carlo Pascutto [:gcp]]

I'm able to reproduce this on the basic gum test page (https://mozilla.github.io/webrtc-landing/gum_test.html), just ctl-c-ing the process when gUM is up. But it's rather intermittent.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016513e3a161

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Clean up Cameras Shutdown sequence on errors

This adds the distinction between being shut down from MediaManager, in which case we do a normal shutdown and try to inform the parent, or our destructor being invoked without warning, in which case we only clean up our own side and don't do anything that requires us to stay alive

I'm no longer seeing the ASAN crash with this, but given that it was intermittent I can't be 100% sure I got it right.

Comment 9

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

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

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

::: dom/media/systemservices/CamerasChild.cpp
@@ +477,3 @@
>      // We don't want to cause everything to get fired up if we're
>      // really already shut down.
>      LOG(("Shutdown when already shut down"));

Should we consider MOZ_CRASH here?  and then return to avoid any problems, or even MOZ_RELEASE_ASSERT(child).

Comment 10

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #9)
> ::: dom/media/systemservices/CamerasChild.cpp
> @@ +477,3 @@
> >      // We don't want to cause everything to get fired up if we're
> >      // really already shut down.
> >      LOG(("Shutdown when already shut down"));
> 
> Should we consider MOZ_CRASH here?  and then return to avoid any problems,
> or even MOZ_RELEASE_ASSERT(child).

I'm not sure. This code is here as a side effect of the way things worked before this fix, where shutdown could be called twice. But I think that perhaps it's not a good idea to do add a release assert in this patch.

Comment 11

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

Ok, we can leave that (perhaps NS_ASSERTION to flag it more in automation?)

Dropping to high - requires the master process to go away, so compromise of the child (even if possible, which is unlikely or perhaps near impossible) doesn't get you much.  Might even be sec-moderate

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

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

See remarks above, this is likely not as serious as first thought.

Comment 13

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

Can you please answer the questions that the template asks when you set sec-approval? We need to know the answers.

Comment 14

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

(In reply to Al Billings [:abillings] from comment #13)
> Can you please answer the questions that the template asks when you set
> sec-approval? We need to know the answers.

He probably set sa? from the "Review" view of the patch, which doesn't ask the questions (the "Details" view does).

Comment 15

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #14)
> (In reply to Al Billings [:abillings] from comment #13)
> > Can you please answer the questions that the template asks when you set
> > sec-approval? We need to know the answers.
> 
> He probably set sa? from the "Review" view of the patch, which doesn't ask
> the questions (the "Details" view does).

You are right - I was confused why I didn't get a template. Time to file more bugs, I'll fill in the template on monday.

Comment 16

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

Any updates here?

Comment 17

[Gian-Carlo Pascutto [:gcp]]

>How easily can the security issue be deduced from the patch? 

Should be possible with the aid of the comments.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 

Sortof.

>Which older supported branches are affected by this flaw? 

Affects up to Firefox 43.

>If not all supported branches, which bug introduced the flaw? 

Bug 1104616.

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

Should be fairly trivial adaptions.

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

Baking a bit on Nightly seems OK to me. The problem is that WebRTC isn't used much so there's large latency before problems get reported back.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #11)
> Ok, we can leave that (perhaps NS_ASSERTION to flag it more in automation?)

I'm ok with trying that on Nightly, but not in any patch I'd want to backport.

Comment 19

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

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

sec-approval+.

We'll want Aurora and Beta patches made and nominated as well.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #9)
> ::: dom/media/systemservices/CamerasChild.cpp
> @@ +477,3 @@
> >      // We don't want to cause everything to get fired up if we're
> >      // really already shut down.
> >      LOG(("Shutdown when already shut down"));
> 
> Should we consider MOZ_CRASH here?  and then return to avoid any problems,
> or even MOZ_RELEASE_ASSERT(child).

FWIW, here's how it currently looks. Unnamed thread=MediaManager thread.

[Unnamed thread 0x7fd39a6d7220]: D/MediaManager MediaManager Thread Shutdown
[Unnamed thread 0x7fd39a6d7220]: D/GetUserMedia Shutdown
[Unnamed thread 0x7fd39a6d7220]: D/MediaManager virtual void mozilla::MediaEngineRemoteVideoSource::Shutdown()
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::Shutdown()   <---- once
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::CamerasChild::ShutdownAll()
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::CamerasChild::ShutdownParent()
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild Dispatching actor deletion
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::CamerasChild::ShutdownChild()
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild PBackground thread exists, dispatching close
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild Erasing sCameras & thread refs (original thread)
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::Shutdown()  <----- twice
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild Shutdown when already shut down
[Cameras IPC]: D/CamerasChild Closing BackgroundChild
[Unnamed thread 0x7fd39a6d7220]: D/GetUserMedia Shutdown
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild void mozilla::camera::Shutdown()  <----- thrice
[Unnamed thread 0x7fd39a6d7220]: D/CamerasChild Shutdown when already shut down
[Cameras IPC]: D/CamerasChild ~CamerasChild: 7fd39a6d78e0
[Cameras IPC]: D/CamerasChild void mozilla::camera::CamerasChild::ShutdownChild()
[Cameras IPC]: D/CamerasChild Shutdown called without PBackground thread
[Cameras IPC]: D/CamerasChild Erasing sCameras & thread refs (original thread)
[Cameras IPC]: D/CamerasChild PBackground thread exists, shutting down thread    <---- looks strange but I think its just the runnable that was queued earlier
[Main Thread]: D/MediaManager MediaManager shutdown lambda running, releasing MediaManager singleton and thread
[Main Thread]: D/MediaManager virtual void mozilla::MediaEngineRemoteVideoSource::Shutdown()  <---- not 4 times because of mInitDone check in that function
[Main Thread]: D/CamerasChild ~CamerasSingleton: 7fd39a659160

I think one mistake is that MediaEngineRemoteVideoSource is calling Shutdown(). It shouldn't, because if there's another source still active we don't want to start shutting things down yet. That will eliminate the first two, I think.

But we should fix this is another bug, it's not a security issue (any more after this patch).

Comment 21

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92827e1743bccc47d56a4dd312e5035f598262b8
Bug 1247236. r=jesup

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Filed bug 1249313 for further shutdown cleanups.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92827e1743bc

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gcp: Could you please nominate patches for uplift to Beta45 and Aurora46? Thanks!

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

Approval Request Comment
[Feature/regressing bug #]: Bug 1104616
[User impact if declined]: Potential security issue. High impact but looks very hard to exploit.
[Describe test coverage new/current, TreeHerder]: Landed on m-c a few days ago.
[Risks and why]: It's a shutdown change so I think that at worst it can hang.
[String/UUID change made/needed]: NA

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 45+ since this is sec-high.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

May be risky (shutdown hangs) but let's try this in aurora since it fixes a sec-high issue.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91fc36759515

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8718735 [details] [diff] [review]
Clean up Cameras Shutdown sequence on errors

Looks like I don't have much choice than taking it :/
Next time, it would be nice to have the uplift request earlier than for the last beta...
Should be in 45 beta 10

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20afdf1337fc

Comment 31

[Gian-Carlo Pascutto [:gcp]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> Comment on attachment 8718735 [details] [diff] [review]
> Clean up Cameras Shutdown sequence on errors
> 
> Looks like I don't have much choice than taking it :/


FWIW comment 11 argues this might be sec-moderate. I would not lose any sleep over it skipping a release.