On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:

On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner [off-list ref] wrote:

quoted

On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:

quoted

On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner [off-list ref] wrote:

quoted

On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:

quoted

As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
version which can acquire filters is useful. There are at least two reasons
this is preferable, even though it uses ptrace:

1. You can control tasks that aren't cooperating with you
2. You can control tasks whose filters block sendmsg() and socket(); if the
   task installs a filter which blocks these calls, there's no way with
   SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.

So for the slow of mind aka me:
I'm not sure I completely understand this problem. Can you outline how
sendmsg() and socket() are involved in this?

I'm also not sure that this holds (but I might misunderstand the
problem) afaict, you could do try to get the fd out via CLONE_FILES and
other means so something like:

// let's pretend the libc wrapper for clone actually has sane semantics
pid = clone(CLONE_FILES);
if (pid == 0) {
        fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);

        // Now this fd will be valid in both parent and child.
        // If you haven't blocked it you can inform the parent what
        // the fd number is via pipe2(). If you have blocked it you can
        // use dup2() and dup to a known fd number.
}

quoted

v2: fix a bug where listener mode was not unset when an unused fd was not
    available
v3: fix refcounting bug (Oleg)
v4: * change the listener's fd flags to be 0
    * rename GET_LISTENER to NEW_LISTENER (Matthew)
v5: * add capable(CAP_SYS_ADMIN) requirement
v7: * point the new listener at the right filter (Jann)

Signed-off-by: Tycho Andersen <redacted>
CC: Kees Cook <redacted>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <redacted>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <redacted>
CC: Tyler Hicks <redacted>
CC: Akihiro Suda <redacted>
---
 include/linux/seccomp.h                       |  7 ++
 include/uapi/linux/ptrace.h                   |  2 +
 kernel/ptrace.c                               |  4 ++
 kernel/seccomp.c                              | 31 +++++++++
 tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
 5 files changed, 112 insertions(+)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 017444b5efed..234c61b37405 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h

@@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
 #ifdef CONFIG_SECCOMP_FILTER
 extern void put_seccomp_filter(struct task_struct *tsk);
 extern void get_seccomp_filter(struct task_struct *tsk);
+extern long seccomp_new_listener(struct task_struct *task,
+                              unsigned long filter_off);
 #else  /* CONFIG_SECCOMP_FILTER */
 static inline void put_seccomp_filter(struct task_struct *tsk)
 {

@@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
 {
      return;
 }
+static inline long seccomp_new_listener(struct task_struct *task,
+                                     unsigned long filter_off)
+{
+     return -EINVAL;
+}
 #endif /* CONFIG_SECCOMP_FILTER */

 #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..e80ecb1bd427 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h

@@ -73,6 +73,8 @@ struct seccomp_metadata {
      __u64 flags;            /* Output: filter's flags */
 };

+#define PTRACE_SECCOMP_NEW_LISTENER  0x420e
+
 /* Read signals from a shared (process wide) queue */
 #define PTRACE_PEEKSIGINFO_SHARED    (1 << 0)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 21fec73d45d4..289960ac181b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c

@@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
              ret = seccomp_get_metadata(child, addr, datavp);
              break;

+     case PTRACE_SECCOMP_NEW_LISTENER:
+             ret = seccomp_new_listener(child, addr);
+             break;
+
      default:
              break;
      }

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 44a31ac8373a..17685803a2af 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c

@@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,

      return ret;
 }
+
+long seccomp_new_listener(struct task_struct *task,
+                       unsigned long filter_off)
+{
+     struct seccomp_filter *filter;
+     struct file *listener;
+     int fd;
+
+     if (!capable(CAP_SYS_ADMIN))
+             return -EACCES;

I know this might have been discussed a while back but why exactly do we
require CAP_SYS_ADMIN in init_userns and not in the target userns? What
if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
use ptrace from in there?

See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/ (local)
. Basically, the problem is that this doesn't just give you capability
over the target task, but also over every other task that has the same
filter installed; you need some sort of "is the caller capable over
the filter and anyone who uses it" check.

Thanks.
But then this new ptrace feature as it stands is imho currently broken.
If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
if you are ns_cpabable(CAP_SYS_ADMIN) then either the new ptrace() api
extension should be fixed to allow for this too or the seccomp() way of
retrieving the pid - which I really think we want - needs to be fixed to
require capable(CAP_SYS_ADMIN) too.
The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
the preferred way to solve this.
Everything else will just be confusing.

First you say "broken", then you say "confusing". Which one do you mean?

Both. It's broken in so far as it places a seemingly unnecessary
restriction that could be fixed. You outlined one possible fix yourself
in the link you provided. And it's confusing in so far as there is a way
via seccomp() to get the fd without said requirement.

Regarding requiring ns_capable() for ptrace: That means that you'll
have to stash namespace information in the seccomp filter. You'd also
potentially be eliding the LSM check that would normally have to occur
between the tracer and the tracee; but I guess that's probably fine?
CAP_SYS_ADMIN in the init namespace already has some abilities that
LSMs can't observe; you could argue that CAP_SYS_ADMIN in another
namespace should have similar semantics, but I'm not sure whether that
matches what the LSM people want as semantics.