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

sandbox.cc

Index: sandbox.cc
===================================================================
--- sandbox.cc	(revision 179)
+++ sandbox.cc	(working copy)
@@ -11,6 +11,7 @@
 namespace playground {
 
 // Global variables
+int                                 Sandbox::proc_self_ = -1;
 int                                 Sandbox::proc_self_maps_ = -1;
 enum Sandbox::SandboxStatus         Sandbox::status_ = STATUS_UNKNOWN;
 int                                 Sandbox::pid_;
@@ -217,7 +218,7 @@
         sys.close(devnull);
       }
       if (proc_fd >= 0) {
-        setProcSelfMaps(sys.openat(proc_fd, "self/maps", O_RDONLY, 0));
+        setProcSelf(sys.openat(proc_fd, "self", O_RDONLY|O_DIRECTORY, 0));
       }
       startSandbox();
       write(sys, fds[1], "", 1);
@@ -250,8 +251,8 @@
   }
 }
 
-void Sandbox::setProcSelfMaps(int proc_self_maps) {
-  proc_self_maps_ = proc_self_maps;
+void Sandbox::setProcSelf(int proc_self) {

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

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?

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

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.

+  proc_self_ = proc_self;
 }
 
 void Sandbox::startSandbox() {
@@ -262,6 +263,11 @@
   }
 
   SysCalls sys;
+  if (proc_self_ >= 0) {
+    proc_self_maps_ = sys.openat(proc_self_, "maps", O_RDONLY, 0);
+    NOINTR_SYS(sys.close(proc_self_));

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

Should we check for close() errors here ? Since they could have security
consequences we should perhaps die() ?

+    proc_self_ = -1;
+  }
   if (proc_self_maps_ < 0) {
     proc_self_maps_        = sys.open("/proc/self/maps", O_RDONLY, 0);
     if (proc_self_maps_ < 0) {