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

sandbox.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sandbox_impl.h"
 
 #include "library.h"
 #include "syscall_entrypoint.h"
 #include "system_call_table.h"
 
 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_;
 int                                 Sandbox::processFdPub_;
 int                                 Sandbox::cloneFdPub_
   // This is necessary to locate the symbol from assembly code on
   // x86-64 (with %rip-relative addressing) in order for this to work
   // in relocatable code (a .so or a PIE).  On i386 this is not
   // necessary but it does not hurt.
   __attribute__((visibility("internal")));
@@ skipping 186 matching lines @@
       return 0;
     case 0: {
       int devnull = sys.open("/dev/null", O_RDWR, 0);
       if (devnull >= 0) {
         sys.dup2(devnull, 0);
         sys.dup2(devnull, 1);
         sys.dup2(devnull, 2);
         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);
 
       // Try to tell the trusted thread to shut down the entire process in an
       // orderly fashion
       defaultSystemCallHandler(__NR_exit_group, 0, 0, 0, 0, 0, 0);
 
       // If that did not work (e.g. because the kernel does not know about the
       // exit_group() system call), make a direct _exit() system call instead.
@@ skipping 12 matching lines @@
       } else {
         status_ = STATUS_AVAILABLE;
       }
       int rc;
       (void)NOINTR_SYS(sys.waitpid(pid, &rc, 0));
       (void)NOINTR_SYS(sys.close(fds[0]));
       return status_ != STATUS_UNSUPPORTED;
   }
 }
 
-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() {
   if (status_ == STATUS_UNSUPPORTED) {
     die("The seccomp sandbox is not supported on this computer");
   } else if (status_ == STATUS_ENABLED) {
     return;
   }
 
   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) {
       die("Cannot access \"/proc/self/maps\"");
     }
   }
 
   // The pid is unchanged for the entire program, so we can retrieve it once
   // and store it in a global variable.
   pid_                     = sys.getpid();
@@ skipping 103 matching lines @@
   } entrypoint;
   *entrypoint.get_syscall_entrypoint() = syscallEntryPointNoFrame;
 
   // We can no longer check for sandboxing support at this point, but we also
   // know for a fact that it is available (as we just turned it on). So update
   // the status to reflect this information.
   status_ = STATUS_ENABLED;
 }
 
 } // namespace