sandbox: Fix RedirectToUserSpacePolicyWrapper

sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc

Index: sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc
index 0ce508be9c7eafbb49a4d632682df9a4715a72e2..f2e6d8ce0f970c7c89abf87fde4532db234a3ede 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc
@@ -504,16 +504,7 @@ ErrorCode GreyListedPolicy(SandboxBPF* sandbox, int sysno, int* aux) {
 
   // Some system calls must always be allowed, if our policy wants to make
   // use of UnsafeTrap()
-  if (sysno == __NR_rt_sigprocmask || sysno == __NR_rt_sigreturn
-#if defined(__NR_sigprocmask)
-      ||
-      sysno == __NR_sigprocmask
-#endif
-#if defined(__NR_sigreturn)
-      ||
-      sysno == __NR_sigreturn
-#endif
-      ) {
+  if (SandboxBPF::IsRequiredForUnsafeTrap(sysno)) {
     return ErrorCode(ErrorCode::ERR_ALLOWED);
   } else if (sysno == __NR_getpid) {
     // Disallow getpid()
@@ -637,18 +628,8 @@ ErrorCode RedirectAllSyscallsPolicy::EvaluateSyscall(SandboxBPF* sandbox,
 
   // Some system calls must always be allowed, if our policy wants to make
   // use of UnsafeTrap()
-  if (sysno == __NR_rt_sigprocmask || sysno == __NR_rt_sigreturn
-#if defined(__NR_sigprocmask)
-      ||
-      sysno == __NR_sigprocmask
-#endif
-#if defined(__NR_sigreturn)
-      ||
-      sysno == __NR_sigreturn
-#endif
-      ) {
+  if (SandboxBPF::IsRequiredForUnsafeTrap(sysno))
     return ErrorCode(ErrorCode::ERR_ALLOWED);
-  }
   return sandbox->UnsafeTrap(AllowRedirectedSyscall, NULL);
 }
 
@@ -2261,6 +2242,74 @@ SANDBOX_DEATH_TEST(SandboxBPF, StartSingleThreadedAsMultiThreaded,
 }
 #endif  // !defined(THREAD_SANITIZER)
 
+// A stub handler for the UnsafeTrap. Never called.
+intptr_t NoOpHandler(const struct arch_seccomp_data& args, void*) {
+  return -1;
+}
+
+class UnsafeTrapWithCondPolicy : public SandboxBPFPolicy {
+ public:
+  UnsafeTrapWithCondPolicy() {}
+  virtual ErrorCode EvaluateSyscall(SandboxBPF* sandbox,
+                                    int sysno) const OVERRIDE {
+    DCHECK(SandboxBPF::IsValidSyscallNumber(sysno));
+    setenv(kSandboxDebuggingEnv, "t", 0);
+    Die::SuppressInfoMessages(true);
+
+    if (SandboxBPF::IsRequiredForUnsafeTrap(sysno))
+      return ErrorCode(ErrorCode::ERR_ALLOWED);
+
+    switch (sysno) {
+      case __NR_close:
+        return sandbox->Cond(0,
+                             ErrorCode::TP_32BIT,
+                             ErrorCode::OP_EQUAL,
+                             100,
+                             ErrorCode(ErrorCode::ERR_ALLOWED),
+                             ErrorCode(EPERM));
+      case __NR_setgid:
+        return sandbox->Cond(0,
+                             ErrorCode::TP_32BIT,
+                             ErrorCode::OP_EQUAL,
+                             100,
+                             ErrorCode(ErrorCode(ENOMEM)),
+                             sandbox->Cond(0,
+                                           ErrorCode::TP_32BIT,
+                                           ErrorCode::OP_EQUAL,
+                                           200,
+                                           ErrorCode(ENOSYS),
+                                           ErrorCode(EPERM)));
+      case __NR_write:
+      case __NR_exit_group:
+        return ErrorCode(ErrorCode::ERR_ALLOWED);
+      case __NR_getppid:
+        return sandbox->UnsafeTrap(NoOpHandler, NULL);
+      default:
+        return ErrorCode(EPERM);
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(UnsafeTrapWithCondPolicy);
+};
+
+BPF_TEST_C(SandboxBPF, UnsafeTrapWithCond, UnsafeTrapWithCondPolicy) {
+  BPF_ASSERT_EQ(-1, syscall(__NR_close, 100));

[jln (very slow on Chromium), 2014/09/02 18:41:54]

In general it's not a good idea to add strong assumptions of how file
descriptors are going to look like. We're inheriting this from where the test
suite runs, and it runs on a variety of platforms, sometimes as root, etc..

I'm also a little uneasy disallowing close() given how fundamental a system call
it is, in case of test failure, it may be called in some important way.

How about using something less fundamental like uname(2), or madvise(2)?

[leecam, 2014/09/02 19:00:12]

I agree, I was trying to think of a syscall that takes a simple parameter (not a
userspace pointer) that can be checked by the policy, exists on all
architectures and will return the same result when ran as root or not. 

uname and madvise take pointers and I wasnt keen on that. I thought setuid(0)
but that would succeed with root and fail when not. So I picked close with a
high fd but agree its not ideal. I'll go through the list and try pick something
better. Maybe just uname(-1) which will fail consistently if the policy failed
to block it.  

+  BPF_ASSERT_EQ(EBADF, errno);
+
+  BPF_ASSERT_EQ(-1, syscall(__NR_close, 101));
+  BPF_ASSERT_EQ(EPERM, errno);
+
+  BPF_ASSERT_EQ(-1, syscall(__NR_setgid, 100));
+  BPF_ASSERT_EQ(ENOMEM, errno);
+
+  BPF_ASSERT_EQ(-1, syscall(__NR_setgid, 200));
+  BPF_ASSERT_EQ(ENOSYS, errno);
+
+  BPF_ASSERT_EQ(-1, syscall(__NR_setgid, 300));
+  BPF_ASSERT_EQ(EPERM, errno);
+}
+
 }  // namespace
 
 }  // namespace sandbox