Minor refactoring of setuid_sandbox_client.

Minor refactoring of setuid_sandbox_client.

This CL has some minor clean up.
- In IsFileSystemAccessDenied, now we check "/" instead of "/proc/self/exe".
- Remove SetuidSandboxClient::CreateInitProcessReaper.
  Now contents uses sandbox::CreateInitProcessReaper directly
  and noone uses the method.
- Clean up include directives in the header.
This is also the preparation of enabling Layer-one sandbox
(suid sandbox) on nacl_helper_nonsfi.

TEST=Ran trybots.
BUG=455087
Committed: https://crrev.com/10feab647ef8b96609d087f9936eee9908b057b7
Cr-Commit-Position: refs/heads/master@{#314529}

[hidehiko, 2015-02-04 07:38:48]

hidehiko@chromium.org changed reviewers:
+ mdempsky@chromium.org

[hidehiko, 2015-02-04 07:39:37]

PTAL.

My main motivation is preparation of nacl_helper_nonsfi sandbox implementation,
but regardless of it, this clean up is OK for sandbox, I believe.

Thank you for your review in advance,
- hidehiko

[mdempsky, 2015-02-04 07:47:41]

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.cc:34: base::ScopedFD
self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY)));
Can you change this function to instead test that "/" fails to open?

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.cc:253: if
(sandbox_binary.empty() && stat("/proc/self/exe", &st) == 0 &&
Let's leave this one as base::kProcSelfExe for now.  I'm going to extract this
function out into a separate file that won't need to be linked into
nacl_helper_nonsfi.

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
File sandbox/linux/suid/client/setuid_sandbox_client.h (right):

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.h:9: #include
"base/callback_forward.h"
I suspect we can remove this #include now too?

[hidehiko, 2015-02-04 08:14:15]

Thank you for quick review. PTAL (running bots, now).

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.cc:34: base::ScopedFD
self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY)));
On 2015/02/04 07:47:41, mdempsky wrote:
> Can you change this function to instead test that "/" fails to open?

Done.

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.cc:253: if
(sandbox_binary.empty() && stat("/proc/self/exe", &st) == 0 &&
On 2015/02/04 07:47:41, mdempsky wrote:
> Let's leave this one as base::kProcSelfExe for now.  I'm going to extract this
> function out into a separate file that won't need to be linked into
> nacl_helper_nonsfi.

Done.

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
File sandbox/linux/suid/client/setuid_sandbox_client.h (right):

https://codereview.chromium.org/885673007/diff/1/sandbox/linux/suid/client/se...
sandbox/linux/suid/client/setuid_sandbox_client.h:9: #include
"base/callback_forward.h"
On 2015/02/04 07:47:41, mdempsky wrote:
> I suspect we can remove this #include now too?

Done to fix include directives.
Note that, AFAIK, recently #include is preferred than forward declarations.
CommandLine and Environment can use forward declarations, but I couldn't find a
good resource to refer the manner in this directory. Please let me know, if
forward declaration is preferred.

[mdempsky, 2015-02-04 08:20:19]

On 2015/02/04 08:14:15, hidehiko wrote:
> Thank you for quick review. PTAL (running bots, now).

LGTM, feel free to CQ.

> Done to fix include directives.
> Note that, AFAIK, recently #include is preferred than forward declarations.
> CommandLine and Environment can use forward declarations, but I couldn't find
a
> good resource to refer the manner in this directory. Please let me know, if
> forward declaration is preferred.

Hm, I remember some discussion about that, but can't find the thread again. 
That recommendation also doesn't seem to have propagated to the Google/Chromium
style guides.

Either way though, this file isn't included enough to worry about it, so I'm
fine with #include's instead of forward declares.  Thanks!

[hidehiko, 2015-02-04 09:18:52]

On 2015/02/04 08:20:19, mdempsky wrote:
> On 2015/02/04 08:14:15, hidehiko wrote:
> > Thank you for quick review. PTAL (running bots, now).
> 
> LGTM, feel free to CQ.
> 
> > Done to fix include directives.
> > Note that, AFAIK, recently #include is preferred than forward declarations.
> > CommandLine and Environment can use forward declarations, but I couldn't
find
> a
> > good resource to refer the manner in this directory. Please let me know, if
> > forward declaration is preferred.
> 
> Hm, I remember some discussion about that, but can't find the thread again. 
> That recommendation also doesn't seem to have propagated to the
Google/Chromium
> style guides.
> 
> Either way though, this file isn't included enough to worry about it, so I'm
> fine with #include's instead of forward declares.  Thanks!

Thank you for quick review. Submitting.

[hidehiko, 2015-02-04 09:18:55]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-02-04 09:19:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/885673007/20001

[commit-bot: I haz the power, 2015-02-04 09:23:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-04 09:24:33]

Patchset 2 (id:??) landed as
https://crrev.com/10feab647ef8b96609d087f9936eee9908b057b7
Cr-Commit-Position: refs/heads/master@{#314529}

[jln (very slow on Chromium), 2015-02-05 00:12:52]

jln@chromium.org changed reviewers:
+ jln@chromium.org

[jln (very slow on Chromium), 2015-02-05 00:12:52]

https://codereview.chromium.org/885673007/diff/20001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/885673007/diff/20001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:34: base::ScopedFD
self_exe(HANDLE_EINTR(open("/", O_RDONLY)));
This seems like it could easily regress. What was the reason for this?

It's kind of a "small trick" that the setuid sandbox currently doesn't even let
access /. But we would consider the process sandboxed even if it was in an empty
chroot instead.

[mdempsky, 2015-02-05 01:41:19]

https://codereview.chromium.org/885673007/diff/20001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/885673007/diff/20001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:34: base::ScopedFD
self_exe(HANDLE_EINTR(open("/", O_RDONLY)));
On 2015/02/05 00:12:52, jln wrote:
> This seems like it could easily regress. What was the reason for this?

Removing base::kProcSelfExe was so this function could compile without dragging
in base/process/process_metrics_linux.cc and its transitive dependencies.

Changing from "/proc/self/exe" to "/" was my suggestion, because I think it's a
stronger test that we've actually dropped filesystem access.  Agreed that we
might in the future want to relax this and allow *some* filesystem access, but I
think it'll be easy enough to tweak this test again if/when necessary.