Make a small change to the API. Instead of passing in a file

Make a small change to the API. Instead of passing in a file
descriptor to /proc/self/maps, we now pass in a file descriptor
to /proc/self and open "maps" ourselves. This is a more generic
API that will make it easier to add other features in the future
(e.g. merge the setuid sandbox into the seccomp sandbox, if the
kernel allow unprivileged calls to chroot).

BUG=none
TEST=make test
Committed: https://code.google.com/p/seccompsandbox/source/detail?r=180

[Markus (顧孟勤), 2012-04-27 22:39:35]

This is a changelist in preparation for merging (parts) of the new sandboxing
code. It makes sure that both the old and the new code expose the same API.

[jln (very slow on Chromium), 2012-04-27 23:00:29]

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h
File sandbox.h (left):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h#oldcode8
sandbox.h:8: extern "C" int  SupportsSeccompSandbox(int proc_fd);
LGTM

Could you document that the caller to SupportsSeccompSandbox should take care of
closing proc_fd if necessary ?

[Mark Seaborn, 2012-04-27 23:07:38]

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc
File sandbox.cc (right):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc#newcode254
sandbox.cc:254: void Sandbox::setProcSelf(int proc_self) {
The smallest change would be to make setProcSelf() do:

  proc_self_maps_ = openat(proc_self_fd, "maps", O_RDONLY, 0);
  close(proc_self_fd);

and change nothing else.

Otherwise it requires a bit more reasoning to check that the sandbox does not
leave the /proc/self FD open.  (I'm not sure how risky it would be to leave the
/proc/self FD open though.  Certainly leaving /proc open would be bad.)

Would it be worthwhile doing the simpler version?

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h
File sandbox.h (left):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h#oldcode8
sandbox.h:8: extern "C" int  SupportsSeccompSandbox(int proc_fd);
On 2012/04/27 23:00:29, Julien Tinnes wrote:
> LGTM
> 
> Could you document that the caller to SupportsSeccompSandbox should
> take care of closing proc_fd if necessary ?

Hmm, yes, that would be different from SeccompSandboxSetProcSelf(), which takes
ownership of its FD argument.

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h
File sandbox.h (right):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.h#newcode9
sandbox.h:9: extern "C" void SeccompSandboxSetProcSelf(int proc_self);
Would SeccompSandboxSetProcSelfFd(int proc_self_fd) be better as a more
descriptive name, perhaps?

[jln (very slow on Chromium), 2012-04-27 23:08:19]

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc
File sandbox.cc (right):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc#newcode268
sandbox.cc:268: NOINTR_SYS(sys.close(proc_self_));
Should we check for close() errors here ? Since they could have security
consequences we should perhaps die() ?

[jln (very slow on Chromium), 2012-04-27 23:11:03]

> Otherwise it requires a bit more reasoning to check that the sandbox does not
> leave the /proc/self FD open.  (I'm not sure how risky it would be to leave
the
> /proc/self FD open though.  Certainly leaving /proc open would be bad.)

I think it's roughly the same to leave /proc or /proc/self open. With the setuid
sandbox we don't chroot to /proc but to /proc/self/fdinfo (with a different
self).

So with either /proc or /proc/self you can openat() anything on the filesystem.

[Markus (顧孟勤), 2012-04-28 01:15:43]

I uploaded a new version that switches both supportsSandbox() and startSandbox()
to "/proc/self". That seems a little cleaner.

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc
File sandbox.cc (right):

https://chromiumcodereview.appspot.com/10178029/diff/1/sandbox.cc#newcode254
sandbox.cc:254: void Sandbox::setProcSelf(int proc_self) {
That actually breaks the existing API in a subtle way.

In the current API, you can call setProcSelf() and the file descriptor is still
accessible until you actually start the sandbox. With your proposed change, the
file descriptor is going to be closed.

This is a problem, if we later decide that we don't want to start the sandbox
after all. It is unclear who then owns and needs to close the file descriptor.

Imagine the following pseudo-code:

int fd = open("/proc/self")
Sandbox::setProcSelf(fd)
Sandbox2::setProcSelf(fd)
...
SuidSandbox::disableFilesystemAccess()
...
if (Sandbox2::supportsSandbox(fd)) {
  Sandbox2::startSandbox();
} else if (Sandbox::supportsSandbox(fd)) {
  Sandbox::startsSandbox();
} else {
  close(fd)
  die();
}
...

At least for the foreseeable future, we want to be able to pick between
different sandbox implementations. And the suid sandbox forces us to split
initialization (i.e. get access to /proc) and activation (start the sandbox)
across two very different parts of the code. That makes everything more
complicated.

I do agree that even with my proposed API this is still a little more complex
than I would like it to be.

If you can think of a cleaner API that actually works within the limitations of
our suid sandbox, then I am all for it. It is quite possible that I have been
thinking about this too much, and I don't see the obvious solution.

[jln (very slow on Chromium), 2012-05-01 00:39:49]

lgtm