Explicitly test bit 30 in the system call number to distinguish between the new x32 API and older I…

sandbox/linux/seccomp-bpf/sandbox_bpf.cc

 // Copyright (c) 2012 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/linux/seccomp-bpf/sandbox_bpf.h"
 
 // The kernel gives us a sandbox, we turn it into a playground :-)
 // This is version 2 of the playground; version 1 was built on top of
 // pre-BPF seccomp mode.
 namespace playground2 {
@@ skipping 197 matching lines @@
   // We can't handle stacked evaluators, yet. We'll get there eventually
   // though. Hang tight.
   if (evaluators_.size() != 1) {
     die("Not implemented");
   }
 
   // If the architecture doesn't match SECCOMP_ARCH, disallow the
   // system call.
   std::vector<struct sock_filter> program;
   program.push_back((struct sock_filter)
-                    BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
-                             offsetof(struct arch_seccomp_data, arch)));
+    BPF_STMT(BPF_LD+BPF_W+BPF_ABS, offsetof(struct arch_seccomp_data, arch)));
   program.push_back((struct sock_filter)
     BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, SECCOMP_ARCH, 1, 0));
   program.push_back((struct sock_filter)
     BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));

[Chris Evans, 2012/06/07 01:02:38]

This should probably be SECCOMP_RET_KILL too, see below.

[Markus (顧孟勤), 2012/06/07 01:33:28]

I don't particularly like SECCOMP_RET_KILL, as it makes debugging pretty much
impossible. And I really want to avoid a situation where developers start
disabling the seccomp sandbox, because they think it prevents them from doing
their job.

And somebody calling us with the wrong API isn't necessarily dangerous. It
probably just means somebody linked against some third-party code that uses the
wrong API. This needs to be fixed, of course. But it's not a fatal security
problem that requires immediate termination.

Having said that, the best long-term solution is probably to generate a suitable
SECCOMP_RET_TRAP. But out trap handler is way too simplistic at this point. It
can't distinguish between different reasons for why it was called.

This is on my TODO list to be added. And then this whole discussion is hopefully
moot.

So, in the meantime, I'd be OK with returning SECCOMP_RET_KILL if that's what
you want. Or we could be nice to the users of this code (none at this moment),
and return SECCOMP_RET_ERRNO. At least, that's debuggable.

 
   // Grab the system call number, so that we can implement jump tables.
   program.push_back((struct sock_filter)
     BPF_STMT(BPF_LD+BPF_W+BPF_ABS, offsetof(struct arch_seccomp_data, nr)));
 
+  // On Intel architectures, verify that system call numbers are in the
+  // expected number range. The older i386 and x86-64 APIs clear bit 30
+  // on all system calls. The newer x86-32 API always sets bit 30.
+#if defined(__i386__) || defined(__x86_64__)

[Chris Evans, 2012/06/07 01:02:38]

I don't quite get this bit. If we're compiling for __i386__, why do anything? I
probably don't understand x86-32, but doesn't it only apply to 64-bit kernels?

[Markus (顧孟勤), 2012/06/07 01:33:28]

The preprocessor tests check for the expected API that our program is using.
They have nothing to do with what the kernel actually supports.

And any 64bit kernel allows programs to use both i386 and x86-64 system calls;
no matter whether the program happens to be 32bit or 64bit.

So, I really don't want to rely on the kernel disabling the x32 API when the
program happens to be compiled as a i386 32bit binary. That probably isn't
happening in current kernels.

In an ideal world, the test for the SECCOMP_ARCH already takes care of all of
this. But I can't find any guarantee that this is always going to be the case
(in fact, there I am not even sure there is an AUDIT_ARCH_X32). And there have
been bugs in this part of the kernel before.

+#if defined(__x86_64__) && defined(__ILP32__)
+  program.push_back((struct sock_filter)
+    BPF_JUMP(BPF_JMP+BPF_JSET+BPF_K, 0x40000000, 1, 0));
+#else
+  program.push_back((struct sock_filter)
+    BPF_JUMP(BPF_JMP+BPF_JSET+BPF_K, 0x40000000, 0, 1));
+#endif
+  program.push_back((struct sock_filter)
+    BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));

[Chris Evans, 2012/06/07 01:02:38]

If we get here, seems like something extraordinarily bad has happened.
SECCOMP_RET_KILL IMHO

[Markus (顧孟勤), 2012/06/07 01:33:28]

Let me know, what you prefer until we get a better SIGSYS handler in place.
There are downsides to killing the program, but since we don't really have users
at this very moment, it probably doesn't matter much.

If I don't hear from you, I'll just upload a new CL with SECCOMP_RET_KILL and a
comment that this should eventually become SECCOMP_RET_TRAP.

+#endif
+
   // Evaluate all possible system calls and depending on their
   // exit codes generate a BPF filter.
   // This is very inefficient right now. We need to be much smarter
   // eventually.
   // We currently incur a O(N) overhead on each system call, with N
   // being the number of system calls. It is easy to get this down to
   // O(log_2(M)) with M being the number of system calls that need special
   // treatment.
   EvaluateSyscall evaluateSyscall = evaluators_.begin()->first;
   for (int sysnum = MIN_SYSCALL; sysnum <= MAX_SYSCALL; ++sysnum) {
@@ skipping 71 matching lines @@
 }
 
 
 bool Sandbox::suppressLogging_          = false;
 Sandbox::SandboxStatus Sandbox::status_ = STATUS_UNKNOWN;
 int    Sandbox::proc_fd_                = -1;
 std::vector<std::pair<Sandbox::EvaluateSyscall,
                       Sandbox::EvaluateArguments> > Sandbox::evaluators_;
 
 }  // namespace