Seccomp-bpf: first pass at a non controverial policy cleanup.

content/common/sandbox_seccomp_bpf_linux.cc

Index: content/common/sandbox_seccomp_bpf_linux.cc
diff --git a/content/common/sandbox_seccomp_bpf_linux.cc b/content/common/sandbox_seccomp_bpf_linux.cc
index a7d35dbddcc93d720c573210fcd40f4d1969309a..f32d67fe5f22f3756c4fb6566b647ff94b733d80 100644
--- a/content/common/sandbox_seccomp_bpf_linux.cc
+++ b/content/common/sandbox_seccomp_bpf_linux.cc
@@ -201,51 +201,52 @@ bool IsAmbiantFileSystemSyscall(int sysno) {
 bool IsFileSystemSyscall(int sysno) {
   switch (sysno) {
     case __NR_access:
-    // case __NR_chmod:
-    // case __NR_chown:
-    // case __NR_creat:
+    case __NR_chmod:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

All of these will ENOENT. I have a TODO to return the optimal errno for any
syscall.

+    case __NR_chown:
+    case __NR_creat:
     case __NR_execve:
-    // case __NR_faccessat:
-    // case __NR_fchmodat:
-    // case __NR_fchownat:  // Should be called chownat ?
-    // case __NR_futimesat:  // Should be called utimesat ?
-    // case __NR_getdents:
-    // case __NR_getdents64:
-    // case __NR_lchown:
-    // case __NR_link:
-    // case __NR_linkat:
-    // case __NR_lookup_dcookie:
+    case __NR_faccessat:
+    case __NR_fchmodat:
+    case __NR_fchownat:  // Should be called chownat ?
+    case __NR_futimesat:  // Should be called utimesat ?
+    case __NR_getdents:
+    case __NR_getdents64:
+    case __NR_lchown:
+    case __NR_link:
+    case __NR_linkat:
+    case __NR_lookup_dcookie:
     case __NR_lstat:
     case __NR_mkdir:
     case __NR_mkdirat:
     case __NR_mknod:
     case __NR_mknodat:
-    // case __NR_newfstatat:  // Should be called statat ?
+    case __NR_newfstatat:  // Should be called statat ?
     case __NR_open:
     case __NR_openat:
     case __NR_readlink:
     case __NR_readlinkat:
-    // case __NR_rename:
-    // case __NR_renameat:
-    // case __NR_rmdir:
+    case __NR_rename:
+    case __NR_renameat:
+    case __NR_rmdir:
     case __NR_stat:
-    // case __NR_statfs:
-    // case __NR_symlink:
-    // case __NR_symlinkat:
-    // case __NR_truncate:
-    // case __NR_unlink:
-    // case __NR_unlinkat:
-    // case __NR_uselib:
-    // case __NR_ustat:  // Deprecated.
-    // case __NR_utime:
-    // case __NR_utimensat:   // New.
-    // case __NR_utimes:
+    case __NR_statfs:
+    case __NR_symlink:
+    case __NR_symlinkat:
+    case __NR_truncate:
+    case __NR_unlink:
+    case __NR_unlinkat:
+    case __NR_uselib:
+    case __NR_ustat:  // Deprecated.
+    case __NR_utime:
+    case __NR_utimensat:   // New.
+    case __NR_utimes:
       return true;
     default:
       return false;
   }
 }
 
+// TODO(jln): these should be denied gracefully as well.
 bool IsAllowedFileSystemCapabilitySyscall(int sysno) {
   switch (sysno) {
     // case __NR_fadvise64:
@@ -256,10 +257,19 @@ bool IsAllowedFileSystemCapabilitySyscall(int sysno) {
     // case __NR_fdatasync:
     // case __NR_sync_file_range:
       return true;
+    default:
+      return false;
+  }
+}
+
+// EPERM is a good errno for any of these.
+bool IsDeniedFileSystemCapabilitySyscall(int sysno) {
+  switch (sysno) {
     case __NR_fallocate:
     case __NR_fchmod:
     case __NR_fchown:
     case __NR_ftruncate:
+      return true;
     default:
       return false;
   }
@@ -271,12 +281,12 @@ bool IsGetProcessIdSyscall(int sysno) {
     case __NR_getegid:
     case __NR_geteuid:
     case __NR_getgid:
-    // case __NR_getgroups:
-    // case __NR_getpid:
-    // case __NR_getppid:
-    // case __NR_getresgid:
-    // case __NR_getresuid:
-    // case __NR_getsid:
+    case __NR_getgroups:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

None of these should cause any problem.

+    case __NR_getpid:
+    case __NR_getppid:
+    case __NR_getresgid:
+    case __NR_getresuid:
+    case __NR_getsid:
     case __NR_gettid:
     case __NR_getuid:
       return true;
@@ -340,8 +350,8 @@ bool IsOperationOnFd(int sysno) {
   switch (sysno) {
     case __NR_close:
     case __NR_dup:
-    // case __NR_dup2:
-    // case __NR_dup3:
+    case __NR_dup2:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

This adds very little attack surface and makes us compatible with newer
code/glibc.

+    case __NR_dup3:
     case __NR_fcntl:
     case __NR_shutdown:
       return true;
@@ -370,8 +380,8 @@ bool IsAllowedProcessStartOrDeath(int sysno) {
     // case __NR_set_tid_address:
     // case __NR_unshare:
     // case __NR_vfork:
-    // case __NR_wait4:
-    // case __NR_waitid:
+    case __NR_wait4:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

Adds very little attack surface and it's really the right thing to do when we
allow clone.

[Markus (顧孟勤), 2012/08/08 10:08:42]

I tend to agree with your assessment about the attack surface.

But clone() is not necessarily a good justification for enabling this system
call. If we only allow clone() for creating threads, then waitXXX() is not very
useful. You have to use futexes to wait for threads. The waitXXX() functions
don't usually work for thread ids.

Also, if you enable wait4() and waitid(), please enable waitpid() where
available (i.e. x86-32)

[jln (very slow on Chromium), 2012/08/08 18:57:28]

Do you think I should put futex and the robust_list syscalls in this group ?
This is x86-64 only ATM.

[Markus (顧孟勤), 2012/08/08 21:55:35]

That would probably work. It's not a perfect fit, but it is definitely very
close.

On 2012/08/08 18:57:28, Julien Tinnes wrote:

[jln (very slow on Chromium), 2012/08/08 23:00:49]

IsFutex is a relatively well-defined set so as a compromise I decided to add a
comment and move IsFutex() just belox IsAllowedProcessStartOrDeath().
What do you think ?

+    case __NR_waitid:
       return true;
     case __NR_setns:  // Privileged.
     default:
@@ -439,14 +449,14 @@ bool IsAddressSpaceAccess(int sysno) {
     case __NR_brk:
     case __NR_madvise:
     // case __NR_mincore:
-    // case __NR_mlock:
+    case __NR_mlock:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

little attack surface, and the amount of locked memory is limited anyway.

     // case __NR_mlockall:
     case __NR_mmap:
     // case __NR_modify_ldt:
     case __NR_mprotect:
     // case __NR_mremap:
     // case __NR_msync:
-    // case __NR_munlock:
+    case __NR_munlock:
     // case __NR_munlockall:
     case __NR_munmap:
     // case __NR_readahead:
@@ -460,28 +470,28 @@ bool IsAddressSpaceAccess(int sysno) {
 bool IsAllowedGeneralIo(int sysno) {
   switch (sysno) {
     case __NR_lseek:
-    // case __NR_poll:
-    // case __NR_ppoll:
+    case __NR_poll:
+    case __NR_ppoll:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

Some attack surface, but relatively well tested and widely used functions. We
already had to allow poll() in some of the policies.

     // case __NR_pread64:
     // case __NR_preadv:
-    // case __NR_pselect6:
+    case __NR_pselect6:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

Same here.

     // case __NR_pwrite64:
     // case __NR_pwritev:
     case __NR_read:
-    // case __NR_readv:
-    // case __NR_recvfrom:  // Could specify source.
+    case __NR_readv:
+    case __NR_recvfrom:  // Could specify source.

[jln (very slow on Chromium), 2012/08/08 01:29:57]

recvfrom allows to specify the source, but so does recvmsg which we already
allowed.

     // case __NR_recvmmsg:  // Could specify source.
     case __NR_recvmsg:   // Could specify source.
-    // case __NR_select:
+    case __NR_select:
     // case __NR_sendfile:
     // case __NR_sendmmsg:  // Could specify destination.
     case __NR_sendmsg:   // Could specify destination.
-    // case __NR_sendto:    // Could specify destination.
+    case __NR_sendto:    // Could specify destination.

[jln (very slow on Chromium), 2012/08/08 01:29:57]

Same as for recvfrom: we allow sendmsg anyway.

[Markus (顧孟勤), 2012/08/08 10:08:42]

Allowing sendmsg() is pretty crazy. And yes, if you do that, allowing all the
other functions doesn't add any more attack surface.

And of course, none of this works on x86-32 anyway, as it uses socketcall()

[jln (very slow on Chromium), 2012/08/08 18:57:28]

That might be something we'll want to broker out. In the meantime it's nice to
have a more consistent policy.
This is x86-64 only for now.

     // case __NR_splice:
     // case __NR_tee:
     // case __NR_vmsplice:
     case __NR_write:
-    // case __NR_writev:
+    case __NR_writev:
       return true;
     default:
     case __NR_ioctl:  // Can be very powerful.
@@ -502,8 +512,8 @@ bool IsPrctl(int sysno) {
 bool IsAllowedBasicScheduler(int sysno) {
   switch (sysno) {
     case __NR_sched_yield:
-    // case __NR_pause:
-    // case __NR_nanosleep:
+    case __NR_pause:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

harmless.

+    case __NR_nanosleep:
     // case __NR_getpriority:
       return true;
     case __NR_setpriority:
@@ -633,7 +643,7 @@ bool IsGlobalSystemStatus(int sysno) {
 bool IsEventFd(int sysno) {
   switch (sysno) {
     case __NR_eventfd:
-    // case __NR_eventfd2:
+    case __NR_eventfd2:

[jln (very slow on Chromium), 2012/08/08 01:29:57]

newest glibc actually uses eventfd2.

       return true;
     default:
       return false;
@@ -871,19 +881,10 @@ bool IsGpuAndFlashPolicyWatched_x86_64(int sysno) {
 // x86_64 only because it references system calls that are multiplexed on IA32.
 playground2::Sandbox::ErrorCode GpuProcessPolicy_x86_64(int sysno) {
   switch(sysno) {
-    case __NR_getpid:  // Nvidia binary driver.
-    case __NR_getppid:  // ATI binary driver.
     case __NR_ioctl:
-    case __NR_mlock:
-    case __NR_munlock:
-    case __NR_poll:
-    case __NR_recvfrom:
-    case __NR_writev:
       return playground2::Sandbox::SB_ALLOWED;
     case __NR_socket:
       return EACCES;  // Nvidia binary driver.
-    case __NR_fchmod:
-      return EPERM;  // ATI binary driver.
     case __NR_open:
       // Accelerated video decode is enabled by default only on Chrome OS.
       if (IsAcceleratedVideoDecodeEnabled()) {
@@ -905,10 +906,16 @@ playground2::Sandbox::ErrorCode GpuProcessPolicy_x86_64(int sysno) {
       }
       // Generally, filename-based syscalls will fail with ENOENT to behave
       // similarly to a possible future setuid sandbox.
+      // TODO(jln): some system calls in those sets are not supposed to
+      // return ENOENT. Return the appropriate error.
       if (IsFileSystemSyscall(sysno) || IsAmbiantFileSystemSyscall(sysno)) {
         return ENOENT;
       }
 
+      if (IsDeniedFileSystemCapabilitySyscall(sysno)) {
+        return EPERM;
+      }
+
       if (IsGpuAndFlashPolicyWatched_x86_64(sysno)) {
           // Previously unseen syscalls. TODO(jln): some of these should
           // be denied gracefully right away.
@@ -926,7 +933,6 @@ playground2::Sandbox::ErrorCode FlashProcessPolicy_x86_64(int sysno) {
     case __NR_sched_setscheduler:
     // These are under investigation, and hopefully not here for the long term.
     case __NR_times:
-    case __NR_wait4:
       return playground2::Sandbox::SB_ALLOWED;
     case __NR_ioctl:
       return ENOTTY;  // Flash Access.
@@ -937,9 +943,16 @@ playground2::Sandbox::ErrorCode FlashProcessPolicy_x86_64(int sysno) {
           IsSystemVSharedMemory(sysno)) {
         return playground2::Sandbox::SB_ALLOWED;
       }
+      // TODO(jln): some system calls in those sets are not supposed to
+      // return ENOENT. Return the appropriate error.
       if (IsFileSystemSyscall(sysno) || IsAmbiantFileSystemSyscall(sysno)) {
         return ENOENT;
       }
+
+      if (IsDeniedFileSystemCapabilitySyscall(sysno)) {
+        return EPERM;
+      }
+
       if (IsGpuAndFlashPolicyWatched_x86_64(sysno)) {
           // Previously unseen syscalls. TODO(jln): some of these should
           // be denied gracefully right away.