NaCl: Add sanity check for number of open FDs at startup

NaCl: Add sanity check for number of open FDs at startup

This is primarily for Non-SFI NaCl, where leaking FDs would be a
security hole.  For SFI NaCl, this is just for defence in depth.

I've put the check just before enabling the seccomp-bpf sandbox.  This
guards against creation of unusual FDs, e.g. via epoll_create(), which
might happen even after enabling the SUID sandbox (which mostly disables
open()).

BUG=358719
TEST=browser_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271583

[jln (very slow on Chromium), 2014-05-07 23:08:24]

Looks good!

The only requirement is to add a unittest for the new Credentials method.

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:97: if
(setuid_sandbox_client_->IsSuidSandboxChild()) {
Do you mind putting this in its own bool-returning function? It looks a little
weird to implement a sanity check in an initialization function rather than just
call it.

CHECK(HasExpectedNumberOfOpenFds());

Also I expect NaClSandbox and the sandbox in content/ to inherit from a common
class (in sandbox/), hopefully not too long for now) and this would help making
the divide natural.

Your choice though.

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:100: //  4)
/dev/urandom FD.
Do you want to reference base::GetUrandomFD() ?

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:104: //     (Only
present when running under the SUID sandbox.)
I will close this.

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:106: //     browser
process, kPrimaryIPCChannel.
It should only be open in SFI mode though:
https://codereview.chromium.org/255903004

Could you mention that it's a dummy in non-SFI mode?

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:111:
CHECK_EQ(credentials.CountOpenFds(proc_fd_.get()), 7);
I was puzzled as to why this would work in non-sfi mode and I remembered that I
replaced the kPrimaryIPCChannel with a dummy, instead of closing it.

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.cc (right):

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:176: int Credentials::CountOpenFds(int
proc_fd) {
You could please add a unit test?

Nothing fancy required. sandboxing tests are guaranteed monothreaded, so
counting fds, creating a pipe and asserting count + 2 is fine.

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:177: int proc_self_fd = openat(proc_fd,
"self/fd", O_DIRECTORY | O_RDONLY);
Could you DCHECK_LE(0, proc_fd); ?

Or feel free to  mimic the API offered by HasOpenDirectory() and allow -1 for
ease of use outside of sandboxing.

[jln (very slow on Chromium), 2014-05-07 23:17:02]

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:106: //     browser
process, kPrimaryIPCChannel.
Sorry, was confused with kSandboxIPCChannel.

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:111:
CHECK_EQ(credentials.CountOpenFds(proc_fd_.get()), 7);
On 2014/05/07 23:08:24, jln wrote:
> I was puzzled as to why this would work in non-sfi mode and I remembered that
I
> replaced the kPrimaryIPCChannel with a dummy, instead of closing it.

err, I meant kSandboxIPCChannel.

[Mark Seaborn, 2014-05-09 22:02:31]

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:97: if
(setuid_sandbox_client_->IsSuidSandboxChild()) {
On 2014/05/07 23:08:24, jln wrote:
> Do you mind putting this in its own bool-returning function?

I've created a separate function, but not a bool-returning one.

The problem with a bool-returning function is that I can't use CHECK_EQ to print
the actual number of FDs on error, which makes debugging failures more awkward.

> Also I expect NaClSandbox and the sandbox in content/ to inherit from a common
> class (in sandbox/), hopefully not too long for now) and this would help
making
> the divide natural.

FWIW, I tend to avoid implementation inheritance, though it depends on the
specifics.

https://codereview.chromium.org/276443003/diff/1/components/nacl/loader/sandb...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:100: //  4)
/dev/urandom FD.
On 2014/05/07 23:08:24, jln wrote:
> Do you want to reference base::GetUrandomFD() ?

Done.

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.cc (right):

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:176: int Credentials::CountOpenFds(int
proc_fd) {
On 2014/05/07 23:08:24, jln wrote:
> You could please add a unit test?
> 
> Nothing fancy required. sandboxing tests are guaranteed monothreaded, so
> counting fds, creating a pipe and asserting count + 2 is fine.

Done.

https://codereview.chromium.org/276443003/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:177: int proc_self_fd = openat(proc_fd,
"self/fd", O_DIRECTORY | O_RDONLY);
On 2014/05/07 23:08:24, jln wrote:
> Could you DCHECK_LE(0, proc_fd); ?

Done.

> Or feel free to  mimic the API offered by HasOpenDirectory() and allow -1 for
> ease of use outside of sandboxing.

I had started doing that before I sent the change for review, but I didn't want
the error handling path (which does "If not available, guess false"), and I
realised I didn't need to allow -1 anyway.

[jln (very slow on Chromium), 2014-05-09 23:55:23]

> Or feel free to  mimic the API offered by HasOpenDirectory() and allow -1 for
> ease of use outside of sandboxing.

I had started doing that before I sent the change for review, but I didn't want
the error handling path (which does "If not available, guess false"), and I
realised I didn't need to allow -1 anyway.

Ohh my, this fallback horror is still in there? I thought I had fixed it a long
time ago. I'll remove it.

https://codereview.chromium.org/276443003/diff/20001/components/nacl/loader/s...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/276443003/diff/20001/components/nacl/loader/s...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:106:
CHECK_EQ(credentials.CountOpenFds(proc_fd_.get()), 7);
Nit: style is CHECK_EQ(expected, actual); (and it's reflected in the actual
error message it prints).

https://codereview.chromium.org/276443003/diff/20001/sandbox/linux/services/c...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/276443003/diff/20001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:63: base::ScopedFD
proc_fd_closer(proc_fd);
Now that ScopedFD have been rewritten, they're not longer "pure closers".

So simply do base::ScopedFD proc_fd(open(..));
and use proc_fd.get()

[jln (very slow on Chromium), 2014-05-09 23:55:59]

lgtm, but please fix the ScopedFD.

[jln (very slow on Chromium), 2014-05-19 21:35:37]

Mark, ping?

It would be nice to land this soon :)

[Mark Seaborn, 2014-05-19 21:38:21]

https://codereview.chromium.org/276443003/diff/20001/components/nacl/loader/s...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/276443003/diff/20001/components/nacl/loader/s...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:106:
CHECK_EQ(credentials.CountOpenFds(proc_fd_.get()), 7);
On 2014/05/09 23:55:24, jln wrote:
> Nit: style is CHECK_EQ(expected, actual);

Fixed.

> (and it's reflected in the actual
> error message it prints).

FWIW, on failure it doesn't say "expected" or "actual":

[0519/212614:FATAL:nacl_sandbox_linux.cc(106)] Check failed: 8 ==
credentials.CountOpenFds(proc_fd_.get()) (8 vs. 7)

https://codereview.chromium.org/276443003/diff/20001/sandbox/linux/services/c...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/276443003/diff/20001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:63: base::ScopedFD
proc_fd_closer(proc_fd);
On 2014/05/09 23:55:24, jln wrote:
> Now that ScopedFD have been rewritten, they're not longer "pure closers".
> 
> So simply do base::ScopedFD proc_fd(open(..));
> and use proc_fd.get()

Done.

[jln (very slow on Chromium), 2014-05-19 21:47:24]

lgtm with a small nit.

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:61: base::ScopedFD
proc_fd(open("/proc", O_RDONLY | O_DIRECTORY));
Could you add "ASSERT_TRUE(proc_fd.is_valid());" ?

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:67: ASSERT_EQ(0, close(fd));
IGNORE_EINTR()

[Mark Seaborn, 2014-05-19 22:06:11]

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:61: base::ScopedFD
proc_fd(open("/proc", O_RDONLY | O_DIRECTORY));
On 2014/05/19 21:47:24, jln wrote:
> Could you add "ASSERT_TRUE(proc_fd.is_valid());" ?

Done.

https://codereview.chromium.org/276443003/diff/60001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:67: ASSERT_EQ(0, close(fd));
On 2014/05/19 21:47:24, jln wrote:
> IGNORE_EINTR()

Done.

[Mark Seaborn, 2014-05-20 00:09:49]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2014-05-20 00:10:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mseaborn@chromium.org/276443003/70001

[commit-bot: I haz the power, 2014-05-20 06:23:56]

Change committed as 271583