Closed Bug 1027902 Opened 10 years ago Closed 9 years ago

Use `SetIntegrityLevel` to set initial GMP process integrity to "low" (in addition to using `SetDelayedIntegrityLevel` to set integrity to "untrusted" later)

Categories

(Core :: Security, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: TimAbraldes, Assigned: bobowen)

References

Details

Attachments

(1 file, 4 obsolete files)

See bug 1011491 comment 0 and bug 1011491 comment 5 for rationale.

We haven't done this yet because switching to `SetIntegrityLevel` causes the process to fail to start at all; we never even enter `NS_internal_main`.
Depends on: 1041775
Attached patch Patch v1 (obsolete) — Splinter Review
This could probably use a little bit of cleanup, but it does the following:
 * Make GMP child process establish channel before starting the sandbox (but still sandbox before loading the plugin DLL)
 * Enable `SandboxBroker` to whitelist specific files for read or for read/write
 * Make `GeckoChildProcessHost` take advantage of file whitelisting
 * Set delayed token level for GMP plugin processes to USER_LOCKDOWN and add the gmp plugin we're trying to load to the "read" whitelist for the process we're initializing
 * Removes our usage of the alternate desktop and alternate window station which increases our potential for shatter attacks. However, this should be mitigated by the fact that we now enable UIPI (see next bullet point)
 * Switch from `SetDelayedIntegrityLevel` to `SetIntegrityLevel` so that UIPI is enabled for the plugin process

This is a huge step forward in lowering the permissions of our GMP processes and I think represents all of the items in the "need to do" category, and as such I think would be a good candidate for uplift to Aurora. Any further ratcheting down of permissions could ride the trains.

Justin - I'm curious if you think there's anything missing here or any other easy wins that could be attained.
Flags: needinfo?(jschuh)
Status: NEW → ASSIGNED
If you're in USER_LOCKDOWN then you can probably drop down to untrusted integrity after warmup and UIPI takes effect (you can achieve this by using both SetIntegrityLevel and SetDelayedIntegrityLevel with low and untrusted respectively, like we do in Chrome). I'm not sure how much that improves things, but it's probably worth seeing if it works.

Your biggest ambient attack surface at this point would be the interactive desktop. UIPI will certainly mitigate that to a degree, but you'll want to see how hard it is to get your process off the interactive desktop entirely.
Flags: needinfo?(jschuh)
Attached patch Patch v2 (obsolete) — Splinter Review
Added delayed integrity level switch to "untrusted"

Added policy exception for communicating over named pipes starting with "chrome". I hadn't noticed earlier, but this is required for the channel to successfully start up and for communication to happen between the main process and the gmp plugin process.

Fiddled with path string manipulations - I haven't yet tested this thoroughly with different path strings.
Attachment #8461692 - Attachment is obsolete: true
Attachment #8462760 - Flags: review?(bobowencode)
Comment on attachment 8462760 [details] [diff] [review]
Patch v2

cpearce - you may want to take a look and test this out with your EME patches
Attachment #8462760 - Flags: feedback?(cpearce)
Comment on attachment 8462760 [details] [diff] [review]
Patch v2

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

Looks like you're getting this fairly well locked down now. :-)

I think I can only r+ the sandboxing parts, but they look fine barring my comments.

I've made general comments about the rest.
A lot of them are just naive questions ... this is only my third review at Mozilla and there's a lot I don't know about C++ as well.

::: content/media/gmp/GMPChild.cpp
@@ +52,5 @@
>  #ifdef MOZ_CRASHREPORTER
>    SendPCrashReporterConstructor(CrashReporter::CurrentThreadId());
>  #endif
>  #endif
> +  bool ret = Open(aChannel, aParentProcessHandle, aIOLoop);

I'm not sure |ret| adds much value.
Can we not just |if (!Open(...)) {| and then return |LoadPluginLibrary(...);|?

Actually, just gone to look at this code and it seems somebody already did something very similar to this for a slightly different reason. :)

::: content/media/gmp/GMPProcessParent.cpp
@@ +8,5 @@
>  
>  #include "base/string_util.h"
>  #include "base/process_util.h"
>  
> +#include "Shlwapi.h" // For `::PathFindFileName`

Is this windows only?
Same applies to other bits.

@@ +48,5 @@
>    args.push_back(mGMPPath);
> +
> +  std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;
> +  std::wstring wGMPPath = converter.from_bytes(mGMPPath.c_str());
> +  std::wstring filename(::PathFindFileName(wGMPPath.c_str()));

Should we be using PathFindFileNameW? ... or will UNICODE always be defined here (which makes this the same if I'm reading the Windows API docs correctly).

@@ +51,5 @@
> +  std::wstring wGMPPath = converter.from_bytes(mGMPPath.c_str());
> +  std::wstring filename(::PathFindFileName(wGMPPath.c_str()));
> +
> +  // Remove the "gmp-" prefix
> +  filename = filename.substr(4);

Can we use filename.erase(0, 4)?

@@ +57,5 @@
> +  // Trim trailing slash if present
> +  bool hasTrailingSlash =
> +      (std::wstring::npos != filename.find_last_of(L"/\\"));
> +  if (hasTrailingSlash) {
> +    filename = filename.substr(0, filename.length() - 1);

Can we not just use filename.pop_back()?
Or are we not guaranteed to have compiler support for that?

@@ +63,5 @@
> +
> +  mAllowedFilesRead.push_back(wGMPPath +
> +                              (hasTrailingSlash ? L"" : L"\\") +
> +                              filename +
> +                              L".dll");

I'm a bit confused by all of this.
From the code that calls this mGMPPath looks like a directory.
So, are we assuming that mGMPPath will always end with something like gmp-wibble and we will always have wibble.dll inside?

Could do with an overall comment or maybe a separate function.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +110,5 @@
>  
>    mPolicy->SetJobLevel(sandbox::JOB_LOCKDOWN, 0);
>    mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> +                         sandbox::USER_LOCKDOWN);
> +  mPolicy->SetIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);

Just an aside really - Aaron raised Bug 1011658 about the fact that we're not checking return values for any of this at the moment.
Should we be doing this, given that we're looking to ship some of this now?

@@ +112,5 @@
>    mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> +                         sandbox::USER_LOCKDOWN);
> +  mPolicy->SetIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> +  mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_UNTRUSTED);
> +  //mPolicy->SetAlternateDesktop(true);

Should we have a comment as to why this is commented out?

@@ +119,5 @@
> +  // in the \pipe\ namespace. We restrict it to pipes that start with
> +  // "chrome." so the sandboxed process cannot connect to system services.
> +  mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
> +                   sandbox::TargetPolicy::FILES_ALLOW_ANY,
> +                   L"\\??\\pipe\\chrome.*");

I've seen this in the warn only sandbox logs.
I'm assuming we can't make this any more specific?

@@ +124,5 @@
>    return true;
>  }
>  
> +bool
> +SandboxBroker::AllowReadFile(wchar_t const *file)

nit / question: do you know why we're binding the * to the right not the left in this code?

@@ +129,5 @@
> +{
> +  mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
> +                   sandbox::TargetPolicy::FILES_ALLOW_READONLY,
> +                   file);
> +  return true;

As we're returning a bool, I think we should be checking the return value of AddRule.

Also, if these are supposed to be for specific files, should we check for any wildcards in the string?
Attachment #8462760 - Flags: review?(bobowencode) → review+
hmm, my EME GMP doesn't start up at all with this patch applied... I had to fix some bitrot in GMPChild::Init(), but I think I did that OK...
And running with MOZ_DISABLE_GMP_SANDBOX=1 still works, so it's definitely some interaction with the sandbox changes. Can I enable logging or somesuch to figure out why this happening? I like the idea of a more locked down sandbox, so want to get this working.
When the child process starts up, I can see that there's a fatal assertion in the plugin-container with 2 threads, running:

Main thread:

ntdll.dll!_NtWaitForSingleObject@12()	Unknown
KernelBase.dll!_WaitForSingleObjectEx@12()	Unknown
KernelBase.dll!_WaitForSingleObject@8()	Unknown
xul.dll!base::WaitableEvent::Wait() Line 50	C++
xul.dll!base::Thread::StartWithOptions(const base::Thread::Options & options={...}) Line 99	C++
xul.dll!ChildThread::Run() Line 34	C++
xul.dll!ChildProcess::ChildProcess(ChildThread * child_thread=0x0436d060) Line 21	C++
xul.dll!mozilla::ipc::ProcessChild::ProcessChild(void * parentHandle=0x00000000) Line 22	C++
xul.dll!mozilla::gmp::GMPProcessChild::GMPProcessChild(void * parentHandle=0x00000000) Line 21	C++
xul.dll!XRE_InitChildProcess(int aArgc=5, char * * aArgv=0x0430e790, GeckoProcessType aProcess=GeckoProcessType_GMPlugin) Line 516	C++
plugin-container.exe!NS_internal_main(int argc=8, char * * argv=0x0430e790) Line 147	C++
plugin-container.exe!wmain(int argc=9, wchar_t * * argv=0x013722c8) Line 105	C++
plugin-container.exe!__tmainCRTStartup() Line 552	C
kernel32.dll!@BaseThreadInitThunk@12()	Unknown
ntdll.dll!__RtlUserThreadStart()	Unknown
ntdll.dll!__RtlUserThreadStart@8()	Unknown



Second thread, fatally asserting:
"Tried to create WaitableEvent from NULL handle"
KernelBase.dll!_DebugBreak@0()	Unknown
xul.dll!RealBreak() Line 504	C++
xul.dll!NS_DebugBreak(unsigned int aSeverity=3, const char * aStr=0x043312e0, const char * aExpr=0x00000000, const char * aFile=0x137065d8, int aLine=24) Line 427	C++
xul.dll!mozilla::Logger::~Logger() Line 47	C++
xul.dll!mozilla::LogWrapper::~LogWrapper()	C++
xul.dll!base::WaitableEvent::WaitableEvent(void * handle=0x00000000) Line 25	C++
xul.dll!IPC::Logging::Logging() Line 82	C++
xul.dll!DefaultSingletonTraits<IPC::Logging>::New() Line 21	C++
xul.dll!Singleton<IPC::Logging,DefaultSingletonTraits<IPC::Logging>,IPC::Logging>::get() Line 129	C++
xul.dll!IPC::Logging::current() Line 99	C++
xul.dll!ChildThread::Init() Line 97	C++
xul.dll!base::Thread::ThreadMain() Line 164	C++
xul.dll!`anonymous namespace'::ThreadFunc(void * closure=0x0436d068) Line 27	C++
kernel32.dll!@BaseThreadInitThunk@12()	Unknown
ntdll.dll!__RtlUserThreadStart()	Unknown
ntdll.dll!__RtlUserThreadStart@8()	Unknown


Fatal assert in IPC::Logging() ctor, the code is trying to create a "ChromeIPCLog.5088on" event using CreateEventW (with a null security descriptor, not sure if that matters), and the CreateEventW call is failing and returning a null handle, which triggers the assert.
The sandbox in USER_LOCKDOWN doesn't have permission to create any named objects; only anonymous objects work by default. You can either duplicate handles from the privileged process and pass them over IPC, or you can add policy exceptions for the specific object types and name patterns you need to allow.
Comment on attachment 8462760 [details] [diff] [review]
Patch v2

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

f- since this breaks plugins in debug builds then. I'm guessing it'll work in builds with logging disabled, but haven't tried.
Attachment #8462760 - Flags: feedback?(cpearce) → feedback-
Attached patch Patch v3 (obsolete) — Splinter Review
This works around the `CreateEvent` issue.

For some reason now I'm seeing clearkey.dll fail to load and the last error value is 5 (access denied). This is similar to what I worked around originally by creating and using `SandboxBroker::AllowReadFile` but seems to be a different issue because that call is being made correctly (i.e. with the correct path to clearkey.dll).
Attachment #8462760 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
This patch adds a bunch of policy exceptions that allow the EME plugin I'm testing with to actually load. Note that even though this patch allows read access to some registry locations and to one specific directory, it still provides significantly increased security over what is currently in m-c.

Requesting feedback from :cpearce - please let me know if everything looks like it's working with this patch applied.
Requesting review from :bent - you're able to review the two pieces (ipc/glue/* and content/*) that this patch touches and that haven't already been reviewed (security/sandbox/win/*).

This patch also addresses review comments:

> ::: content/media/gmp/GMPChild.cpp
> @@ +52,5 @@
> >  #ifdef MOZ_CRASHREPORTER
> >    SendPCrashReporterConstructor(CrashReporter::CurrentThreadId());
> >  #endif
> >  #endif
> > +  bool ret = Open(aChannel, aParentProcessHandle, aIOLoop);
> 
> I'm not sure |ret| adds much value.
> Can we not just |if (!Open(...)) {| and then return
> |LoadPluginLibrary(...);|?
> 
> Actually, just gone to look at this code and it seems somebody already did
> something very similar to this for a slightly different reason. :)

Indeed! This has been removed from my patch since it's been done elsewhere.

> ::: content/media/gmp/GMPProcessParent.cpp
> @@ +8,5 @@
> >  
> >  #include "base/string_util.h"
> >  #include "base/process_util.h"
> >  
> > +#include "Shlwapi.h" // For `::PathFindFileName`
> 
> Is this windows only?
> Same applies to other bits.

Yes it is. This bit has been removed but I'm going through the other parts of this patch to make sure I haven't failed to guard any Windows-specific parts with `#ifdef`s. The try run will also catch these.

> @@ +48,5 @@
> >    args.push_back(mGMPPath);
> > +
> > +  std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;
> > +  std::wstring wGMPPath = converter.from_bytes(mGMPPath.c_str());
> > +  std::wstring filename(::PathFindFileName(wGMPPath.c_str()));
> 
> Should we be using PathFindFileNameW? ... or will UNICODE always be defined
> here (which makes this the same if I'm reading the Windows API docs
> correctly).

I think we always define `UNICODE` but we should still use `PathFindFileNameW` explicitly. This has been removed from the current patch anyway (instead of allowing access specifically to the DLL, I've modified the patch to just allow read access to everything in the directory that we're loading the DLL from).

> @@ +51,5 @@
> > +  std::wstring wGMPPath = converter.from_bytes(mGMPPath.c_str());
> > +  std::wstring filename(::PathFindFileName(wGMPPath.c_str()));
> > +
> > +  // Remove the "gmp-" prefix
> > +  filename = filename.substr(4);
> 
> Can we use filename.erase(0, 4)?

Yup! That would have made more sense. This is also removed now, though.

> @@ +57,5 @@
> > +  // Trim trailing slash if present
> > +  bool hasTrailingSlash =
> > +      (std::wstring::npos != filename.find_last_of(L"/\\"));
> > +  if (hasTrailingSlash) {
> > +    filename = filename.substr(0, filename.length() - 1);
> 
> Can we not just use filename.pop_back()?
> Or are we not guaranteed to have compiler support for that?

I think my mental compiler lacks support for this feature. As with the other items, this has been removed in favor of allowing read-only access to every file in the directory.

> @@ +63,5 @@
> > +
> > +  mAllowedFilesRead.push_back(wGMPPath +
> > +                              (hasTrailingSlash ? L"" : L"\\") +
> > +                              filename +
> > +                              L".dll");
> 
> I'm a bit confused by all of this.
> From the code that calls this mGMPPath looks like a directory.
> So, are we assuming that mGMPPath will always end with something like
> gmp-wibble and we will always have wibble.dll inside?
> 
> Could do with an overall comment or maybe a separate function.

Yes that is the assumption that was being made. The code in `GMPChild::LoadPluginLibrary` makes the same assumptions so it felt safe. This has been removed though, since we now allow read-only access to all the files in the directory.

> ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
> @@ +110,5 @@
> >  
> >    mPolicy->SetJobLevel(sandbox::JOB_LOCKDOWN, 0);
> >    mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> > +                         sandbox::USER_LOCKDOWN);
> > +  mPolicy->SetIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> 
> Just an aside really - Aaron raised Bug 1011658 about the fact that we're
> not checking return values for any of this at the moment.
> Should we be doing this, given that we're looking to ship some of this now?

Updated to check return codes.

> @@ +112,5 @@
> >    mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> > +                         sandbox::USER_LOCKDOWN);
> > +  mPolicy->SetIntegrityLevel(sandbox::INTEGRITY_LEVEL_LOW);
> > +  mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_UNTRUSTED);
> > +  //mPolicy->SetAlternateDesktop(true);
> 
> Should we have a comment as to why this is commented out?

In this updated patch, I've switched which item we comment out: Instead of using the interactive desktop and trusting UIPI (by setting the process integrity to Low initially), we now use an alternate desktop and window station but we don't initially set the process integrity to Low. As a result, I should probably change the name of this bug or maybe move the patch to a different bug.

> @@ +119,5 @@
> > +  // in the \pipe\ namespace. We restrict it to pipes that start with
> > +  // "chrome." so the sandboxed process cannot connect to system services.
> > +  mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
> > +                   sandbox::TargetPolicy::FILES_ALLOW_ANY,
> > +                   L"\\??\\pipe\\chrome.*");
> 
> I've seen this in the warn only sandbox logs.
> I'm assuming we can't make this any more specific?

This is copy+pasted from Chromium code; I don't believe we can be more specific than this.

> @@ +124,5 @@
> >    return true;
> >  }
> >  
> > +bool
> > +SandboxBroker::AllowReadFile(wchar_t const *file)
> 
> nit / question: do you know why we're binding the * to the right not the
> left in this code?

Personal preference :)

If there is something in the style guide that says we should snuggle the * to the left, I'll be happy to change this.

> @@ +129,5 @@
> > +{
> > +  mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
> > +                   sandbox::TargetPolicy::FILES_ALLOW_READONLY,
> > +                   file);
> > +  return true;
> 
> As we're returning a bool, I think we should be checking the return value of
> AddRule.

Done!

> Also, if these are supposed to be for specific files, should we check for
> any wildcards in the string?

I think we can trust the consumers to know what they're doing. In the newest patch I'm actually using this function to allow a wildcard path. Maybe renaming the function would help? "path" vs "file" perhaps?
Attachment #8468794 - Attachment is obsolete: true
Attachment #8473201 - Flags: review?(bent.mozilla)
Attachment #8473201 - Flags: feedback?(cpearce)
Comment on attachment 8473201 [details] [diff] [review]
Patch v4

Actually I'm going to move this patch over to bug 1027906 since it's accomplishing the goal put forth there and not the goal of this bug.
Attachment #8473201 - Attachment is obsolete: true
Attachment #8473201 - Flags: review?(bent.mozilla)
Attachment #8473201 - Flags: feedback?(cpearce)
No longer blocks: 1027906
Re-assigning to :bobowen so this doesn't get lost as I transition out of sandboxing land.

Bob - feel free to unassign this and/or track however you see fit!
Assignee: tabraldes → bobowencode
As long as the patches for bug 1041775 stick we can now use an integrity level of low for the GMP processes.

Tim, just following your instructions in the comment here. :-)

Chris, we drop to untrusted before we load the plugin, but I've tested this with the ClearKey plugin anyway.  Including with |#define TEST_DECODING 1|.


Try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6bbf65f0cd12
Attachment #8525982 - Flags: review?(tabraldes)
Attachment #8525982 - Flags: feedback?(cpearce)
Comment on attachment 8525982 [details] [diff] [review]
Use an intial integrity level of low for the GMP sandbox on Windows. v1

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

This is one of those rare and lovely changes that both reduces the number of lines and enhances functionality :)
Attachment #8525982 - Flags: review?(tabraldes) → review+
Summary: Use `SetIntegrityLevel` instead of `SetDelayedIntegrityLevel` for GMP processes → Use `SetIntegrityLevel` to set initial GMP process integrity to "low" (in addition to using `SetDelayedIntegrityLevel` to set integrity to "untrusted" later)
Comment on attachment 8525982 [details] [diff] [review]
Use an intial integrity level of low for the GMP sandbox on Windows. v1

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

Hmm.... Does this apply on top of m-c? If I apply only this patch to today's m-c, my github-gmp-clearkey fails to load on people.mozilla.org/~cpearce/eme/ with TEST_DECODING defined, but if I pop this patch, it loads as expected...

(Note: you need the patch in bug 1103648 to prevent an exception on shutdown with EME today).
(In reply to Chris Pearce (:cpearce) from comment #17)
> Comment on attachment 8525982 [details] [diff] [review]
> Use an intial integrity level of low for the GMP sandbox on Windows. v1
> 
> Review of attachment 8525982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm.... Does this apply on top of m-c? If I apply only this patch to today's
> m-c, my github-gmp-clearkey fails to load on
> people.mozilla.org/~cpearce/eme/ with TEST_DECODING defined, but if I pop
> this patch, it loads as expected...

It will apply, but it requires bug 1041775 to land before it will work.
Unfortunately, that got backed-out due to PGO bustage, but I've sorted that now and just waiting on reviews.

The update from the Chromium sandboxing code includes a fix to allow us to use an initial integrity level of low at the same time as an alternate desktop.
Comment on attachment 8525982 [details] [diff] [review]
Use an intial integrity level of low for the GMP sandbox on Windows. v1

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

I tested this patch on inbound (which has the sandbox update) and WMF still work there with this patch.
Attachment #8525982 - Flags: feedback?(cpearce) → feedback+
https://hg.mozilla.org/mozilla-central/rev/134dbb88ff88
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.