UAF in Cameras Shutdown on channel errors

RESOLVED FIXED in Firefox 45

Status

()

defect
P1
critical
Rank:
5
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

({csectype-uaf, sec-high})

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main45+][post-critsmash-triage])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
I can't see bug 1244493, but this sounds like a likely sec-crit even if it's very hard to leverage
Assignee: nobody → gpascutto
Rank: 5
Priority: -- → P1
(Assignee)

Comment 3

3 years ago
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.
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.
(Assignee)

Comment 5

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

Comment 6

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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.
Attachment #8718735 - Flags: review?(rjesup)
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).
Attachment #8718735 - Flags: review?(rjesup) → review+
(Assignee)

Comment 10

3 years ago
(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.
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
Keywords: sec-criticalsec-high
(Assignee)

Comment 12

3 years ago
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.
Attachment #8718735 - Flags: sec-approval?
Can you please answer the questions that the template asks when you set sec-approval? We need to know the answers.
Flags: needinfo?(gpascutto)
(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).
(Assignee)

Comment 15

3 years ago
(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.
Any updates here?
Group: core-security → media-core-security
(Assignee)

Comment 17

3 years ago
>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.
Flags: needinfo?(gpascutto)
(Assignee)

Comment 18

3 years ago
(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 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.
Attachment #8718735 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 20

3 years ago
(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).
(Assignee)

Comment 22

3 years ago
Filed bug 1249313 for further shutdown cleanups.
https://hg.mozilla.org/mozilla-central/rev/92827e1743bc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: media-core-security → core-security-release
Gcp: Could you please nominate patches for uplift to Beta45 and Aurora46? Thanks!
Flags: needinfo?(gpascutto)
(Assignee)

Comment 25

3 years ago
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
Flags: needinfo?(gpascutto)
Attachment #8718735 - Flags: approval-mozilla-beta?
Attachment #8718735 - Flags: approval-mozilla-aurora?
Tracking for 45+ since this is sec-high.
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.
Attachment #8718735 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8718735 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 31

3 years ago
(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.
Whiteboard: [adv-main45+]
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Blocks: 1244493
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.