969559 - Set delayed restricted integrity in child process to block off pipe and file access after LowerToken call

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Brian R. Bondy [:bbondy]]

We need to set the delayed integrity token to restricted.  
Then we need to move the LowerToken call to after the initial pipes are setup.

After this, no new pipes or files can be created.

---

I'm also creating a new call in the sandbox broker in this bug for:
AllowPipe(const wchar_t *aPath)

This allows creation of a pipe path, but does not seem to help when connecting to one. That being said this function isn't currently used, but it should remain for future use.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev1

This review is only for the xpcom def changes and the toolkit/xre change.
I'll pass this patch through cpeterson after your review as well for the rest.

Basically the sandbox lib needs to be linked directly to the sandboxed exe, but I need a call to LowerToken from within something from something detected in xul.dll.  So I pass a function pointer up to call when needed for enabling the sandbox.  

The sandbox can't be enabled too early because we need pipes connected first.

Comment 2

[Benjamin Smedberg]

Comment on attachment 8372505 [details] [diff] [review]
Patch rev1

>diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp

>+void(*geckoChildProcessCreated)();

This is a variable, it should be named "gGeckoChildProcessCreatedCallback" or something to make it clear. Although see more general API comments below.

>diff --git a/xpcom/build/nsXULAppAPI.h b/xpcom/build/nsXULAppAPI.h

>+XRE_API(nsresult,
>+        XRE_InitChildProcessCallback, (int aArgc,
>+                                       char* aArgv[],
>+                                       GeckoProcessType aProcess,
>+                                       void(*GeckoChildProcessCreated)()))

This is very under-documented, and in particular "GeckoChildProcessCreated" is a weird name for this: I would have expected this API to do initialization, call the callback, and then start the event loop, but that's not what it does. This callback is something specific to launching the sandbox, right?

As I understand it, this has to be a callback because the sandbox code has to live in the child container (plugin-runtime.exe) but the code which calls it has to live in libxul.so. Is that correct? If so, can we just make this an API to set/get a specific function pointer and not modify XRE_InitChildProcess at all?

Style: the function pointer is hard to read. Please use a typedef (or just use PRFuncPtr) to make this easier to read.

Also I hate that any of this lives in nsXULAppAPI since it is very specific to our content-process code. It should really live in a private header.

Comment 3

[Brian R. Bondy [:bbondy]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 8372505 [details] [diff] [review]
> Patch rev1
> 
> >diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp
> 
> This callback is something specific to launching the
> sandbox, right?

Yep I'll name it better.

> As I understand it, this has to be a callback because the sandbox code has
> to live in the child container (plugin-runtime.exe) but the code which calls
> it has to live in libxul.so. Is that correct?

Yes.

> If so, can we just make this
> an API to set/get a specific function pointer and not modify
> XRE_InitChildProcess at all?

Yep, I can do that instead.
 
> Style: the function pointer is hard to read. Please use a typedef (or just
> use PRFuncPtr) to make this easier to read.

No problem,will do.

> Also I hate that any of this lives in nsXULAppAPI since it is very specific
> to our content-process code. It should really live in a private header.

Will do, thanks for the feedback.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev2

Created a new sandboxTarget.h file which contains a singleton. It is class imported into the exe and exported from xul.dll.  It allows setting a function pointer from the exe and then later calling it when ready in the dll.  Note: The singleton instance is shared between both modules.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8374959 [details] [diff] [review]
Patch rev2

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

I am definitely not the right person to review this code. <:)

::: security/sandbox/win/src/sandboxtarget/sandboxTarget.h
@@ +39,5 @@
> +    mStartSandboxCallback = aStartSandboxCallback;
> +  }
> +
> +  /**
> +   * Called by the library that wants to start the sandbox, wich in turns

s/wich/which/

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8374959 [details] [diff] [review]
Patch rev2

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

::: security/sandbox/win/src/sandboxtarget/sandboxTarget.h
@@ +39,5 @@
> +    mStartSandboxCallback = aStartSandboxCallback;
> +  }
> +
> +  /**
> +   * Called by the library that wants to start the sandbox, wich in turns

s/turns/turn/

Comment 7

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/20152316a608

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20152316a608

Comment 9

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

Comment on attachment 8374959 [details] [diff] [review]
Patch rev2

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

::: dom/ipc/ContentChild.cpp
@@ +582,5 @@
> +
> +#if defined(MOZ_CONTENT_SANDBOX)
> +#if defined(XP_WIN)
> +  mozilla::SandboxTarget::Instance()->StartSandbox();
> +#else if defined(XP_UNIX) && !defined(XP_MACOSX)

Slight typo here: should be #elif.  This is treated as #else; the rest of the line is discarded.

Using --enable-warnings-as-errors seems to catch this, at least with GCC, but only if --enable-content-sandbox is also set to turn on the surrounding #if, and nothing on TBPL has both of those.

At the moment it's not burning the tree, and I have a patch in progress that changes these ifdefs (and so will fix this as a side-effect), but I wanted to mention it here in case it's relevant for uplifts or anything like that.

Comment 10

[Brian R. Bondy [:bbondy]]

Thanks Jed!