Add support for running the NaCl plugin in the Linux SUID sandbox

Add support for running the NaCl plugin in the Linux SUID sandbox

 * Add a function for getting the pre-opened FD for /dev/urandom.
   This needs to be a C function because it will be used by
   nacl_secure_random.c.
 * Add an IPC message for creating shared memory segments, since
   /dev/shm is not available inside the sandbox.

BUG=36676
TEST=nacl_ui_tests in conjunction with NaCl changes

[Mark Seaborn, 2010-03-04 10:55:53]

The corresponding NaCl change is http://codereview.chromium.org/669056

[agl, 2010-03-04 15:05:32]

LGTM

http://codereview.chromium.org/669055/diff/1/2
File base/rand_util_c.h (right):

http://codereview.chromium.org/669055/diff/1/2#newcode14
base/rand_util_c.h:14: int GetUrandomFD(void);
There's C and then there's 20 year old C :)

(commenting on the '(void)' )

http://codereview.chromium.org/669055/diff/1/3
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/669055/diff/1/3#newcode59
base/rand_util_posix.cc:59: int GetUrandomFD(void) {
ditto

http://codereview.chromium.org/669055/diff/1/3#newcode61
base/rand_util_posix.cc:61: }
Would this function not be name mangled by the C++ compiler? If it's an exported
C function I thought that you needed extern "C" blocks around it.

http://codereview.chromium.org/669055/diff/1/4
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/669055/diff/1/4#newcode341
chrome/browser/renderer_host/render_sandbox_host_linux.cc:341:
base::SharedMemory shm;
There should be a limit on the size of the shared memory that we'll try and
create. For one, the argument to Create is a uint32_t so it needs to be <=
2**32.

http://codereview.chromium.org/669055/diff/1/6
File chrome/renderer/renderer_sandbox_support_linux.cc (right):

http://codereview.chromium.org/669055/diff/1/6#newcode78
chrome/renderer/renderer_sandbox_support_linux.cc:78: 
remove extra blank line.

http://codereview.chromium.org/669055/diff/1/7
File chrome/renderer/renderer_sandbox_support_linux.h (right):

http://codereview.chromium.org/669055/diff/1/7#newcode32
chrome/renderer/renderer_sandbox_support_linux.h:32: // Returns a file
descriptor for a shared memory segment.
I don't think this is the correct header. Let me read the rest of the review and
I'll swing back around to this.

Later:

Ok, there isn't anywhere very good to put it, so here will do. It should
probably go inside the namespace however. Sadly there will be a mixing of WebKit
style and Google style names, but that's always going to happen in Chrome.

[Mark Seaborn, 2010-03-04 16:37:35]

http://codereview.chromium.org/669055/diff/1/2
File base/rand_util_c.h (right):

http://codereview.chromium.org/669055/diff/1/2#newcode14
base/rand_util_c.h:14: int GetUrandomFD(void);
On 2010/03/04 15:05:33, agl wrote:
> There's C and then there's 20 year old C :)
> 
> (commenting on the '(void)' )

:-)  I've seen gcc complain in the past about doing
  void foo();
instead of
  void foo(void);

Perhaps gcc has since dropped the warning.

The former leaves the arguments unspecified.

http://codereview.chromium.org/669055/diff/1/3
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/669055/diff/1/3#newcode61
base/rand_util_posix.cc:61: }
On 2010/03/04 15:05:33, agl wrote:
> Would this function not be name mangled by the C++ compiler? If it's an
exported
> C function I thought that you needed extern "C" blocks around it.

No, declaring the prototype with 'extern "C"' is enough to define the function
the same way.  It's the same as with __attribute__ declarations.

http://codereview.chromium.org/669055/diff/1/4
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/669055/diff/1/4#newcode341
chrome/browser/renderer_host/render_sandbox_host_linux.cc:341:
base::SharedMemory shm;
On 2010/03/04 15:05:33, agl wrote:
> There should be a limit on the size of the shared memory that we'll try and
> create. For one, the argument to Create is a uint32_t so it needs to be <=
> 2**32.

I'll change it to be 32-bit.

Do you mean there should be a limit less than 2**32?

http://codereview.chromium.org/669055/diff/1/6
File chrome/renderer/renderer_sandbox_support_linux.cc (right):

http://codereview.chromium.org/669055/diff/1/6#newcode78
chrome/renderer/renderer_sandbox_support_linux.cc:78: 
On 2010/03/04 15:05:33, agl wrote:
> remove extra blank line.

Done.

http://codereview.chromium.org/669055/diff/1/7
File chrome/renderer/renderer_sandbox_support_linux.h (right):

http://codereview.chromium.org/669055/diff/1/7#newcode32
chrome/renderer/renderer_sandbox_support_linux.h:32: // Returns a file
descriptor for a shared memory segment.
On 2010/03/04 15:05:33, agl wrote:
> I don't think this is the correct header. Let me read the rest of the review
and
> I'll swing back around to this.
> 
> Later:
> 
> Ok, there isn't anywhere very good to put it, so here will do. It should
> probably go inside the namespace however. Sadly there will be a mixing of
WebKit
> style and Google style names, but that's always going to happen in Chrome.

I've moved it into the namespace.

[agl, 2010-03-04 16:42:29]

LGTM

http://codereview.chromium.org/669055/diff/1/4
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/669055/diff/1/4#newcode341
chrome/browser/renderer_host/render_sandbox_host_linux.cc:341:
base::SharedMemory shm;
On 2010/03/04 16:37:35, mseaborn1 wrote:
> On 2010/03/04 15:05:33, agl wrote:
> > There should be a limit on the size of the shared memory that we'll try and
> > create. For one, the argument to Create is a uint32_t so it needs to be <=
> > 2**32.
> 
> I'll change it to be 32-bit.
> 
> Do you mean there should be a limit less than 2**32?

Eh, maybe? This can be called by compromised renderers so there's a question of
how much damage can be caused. They could fill the file system with a huge file
via this call. But then, they could make multiple calls and fill the filesystem
via smaller files too.

So, apart from the 64->32 bit thing, it's not too important. I would add a
sanity check for >1GB just in case something down the road isn't careful, but I
understand that some people find such limits distasteful, so it's up to you.

[Mark Schneckloth, 2010-03-04 17:35:25]

LGTM

[gregoryd, 2010-03-04 17:44:12]

LGTM

[Mark Seaborn, 2010-03-04 18:40:44]

Adam, can you commit this on my behalf?  I'm not a Chromium committer yet. 
(Jeremy Orlow has nominated me for provisional committer status but I think that
will take a little while.)

Thanks,
Mark

[agl, 2010-03-04 18:42:45]

On Thu, Mar 4, 2010 at 1:40 PM,  <mseaborn@chromium.org> wrote:
> Adam, can you commit this on my behalf?  I'm not a Chromium committer yet.
> (Jeremy Orlow has nominated me for provisional committer status but I think
> that
> will take a little while.)

Will do.


AGL

[agl, 2010-03-04 20:24:14]

On Thu, Mar 4, 2010 at 1:42 PM, Adam Langley <agl@chromium.org> wrote:
> On Thu, Mar 4, 2010 at 1:40 PM,  <mseaborn@chromium.org> wrote:
> Will do.

Landed as r40647.


AGL

[Mark Seaborn, 2010-03-05 14:12:19]

On 2010/03/04 16:42:29, agl wrote:
> On 2010/03/04 16:37:35, mseaborn1 wrote:
> > Do you mean there should be a limit less than 2**32?
> 
> Eh, maybe? This can be called by compromised renderers so there's a question
of
> how much damage can be caused. They could fill the file system with a huge
file
> via this call. But then, they could make multiple calls and fill the
filesystem
> via smaller files too.

We wouldn't be limiting the authority of the renderer with a check in the
browser's make_shm() method because the renderer can still do ftruncate() to
enlarge the segment.  I suspect that this just creates a sparse file so the
memory isn't allocated until it is dirtied.  I could have put the ftruncate() on
the client side but it seemed neater doing it on the server side.  Maybe in the
future, with the seccomp sandbox, we can disable ftruncate() inside the sandbox.

BTW it won't just be compromised renderers that can call this.  Native Client
processes can create shared memory segments.

Maybe we should attempt to do resource accountability and have an overall limit
on the space that a renderer or its associated NaCl processes can consume, but
AFAIK we don't have any story for how to decide what such limits should be.

Cheers,
Mark

[agl, 2010-03-05 15:39:47]

On Fri, Mar 5, 2010 at 9:12 AM,  <mseaborn@chromium.org> wrote:
> We wouldn't be limiting the authority of the renderer with a check in the
> browser's make_shm() method because the renderer can still do ftruncate() to
> enlarge the segment.

With the SUID sandbox we can't stop that of course. However, with the
seccomp sandbox (or some other ones) we could stop the renderer from
calling ftruncate, so it's worth thinking about.

> Maybe in
> the
> future, with the seccomp sandbox, we can disable ftruncate() inside the
> sandbox.

Exactly, with the SUID sandbox we can't stop it. With the seccomp
sandbox (or SELinux etc) we could, so it's worth thinking about.

> BTW it won't just be compromised renderers that can call this.  Native
> Client
> processes can create shared memory segments.

Good to know.

> Maybe we should attempt to do resource accountability and have an overall
> limit
> on the space that a renderer or its associated NaCl processes can consume,
> but
> AFAIK we don't have any story for how to decide what such limits should be.

Indeed. I'm not saying that we need it right now, but at some point
it's probably a good idea.


AGL