Re: [PATCH v3 1/4] seccomp: add a return code to trap to userspace

[Jann Horn]

On Mon, Jun 4, 2018 at 2:18 AM Tycho Andersen <tycho@xxxxxxxx> wrote:
>
> Hi Jann,
>
> On Sun, Jun 03, 2018 at 08:41:01PM +0200, Jann Horn wrote:
> > On Sun, Jun 3, 2018 at 2:29 PM Tycho Andersen <tycho@xxxxxxxx> wrote:
> > >
> > > This patch introduces a means for syscalls matched in seccomp to notify
> > > some other task that a particular filter has been triggered.
> > >
> > > The motivation for this is primarily for use with containers. For example,
> > > if a container does an init_module(), we obviously don't want to load this
> > > untrusted code, which may be compiled for the wrong version of the kernel
> > > anyway. Instead, we could parse the module image, figure out which module
> > > the container is trying to load and load it on the host.
> > >
> > > As another example, containers cannot mknod(), since this checks
> > > capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
> > > /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
> > > coding some whitelist in the kernel. Another example is mount(), which has
> > > many security restrictions for good reason, but configuration or runtime
> > > knowledge could potentially be used to relax these restrictions.
> > >
> > > This patch adds functionality that is already possible via at least two
> > > other means that I know about, both of which involve ptrace(): first, one
> > > could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> > > Unfortunately this is slow, so a faster version would be to install a
> > > filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> > > Since ptrace allows only one tracer, if the container runtime is that
> > > tracer, users inside the container (or outside) trying to debug it will not
> > > be able to use ptrace, which is annoying. It also means that older
> > > distributions based on Upstart cannot boot inside containers using ptrace,
> > > since upstart itself uses ptrace to start services.
> > >
> > > The actual implementation of this is fairly small, although getting the
> > > synchronization right was/is slightly complex.
> > >
> > > Finally, it's worth noting that the classic seccomp TOCTOU of reading
> > > memory data from the task still applies here, but can be avoided with
> > > careful design of the userspace handler: if the userspace handler reads all
> > > of the task memory that is necessary before applying its security policy,
> > > the tracee's subsequent memory edits will not be read by the tracer.
> > [...]
> > > @@ -857,13 +1020,28 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > >         if (IS_ERR(prepared))
> > >                 return PTR_ERR(prepared);
> > >
> > > +       if (flags & SECCOMP_FILTER_FLAG_GET_LISTENER) {
> > > +               listener = get_unused_fd_flags(O_RDWR);
> >
> > I think you want either 0 or O_CLOEXEC here?
>
> Do we? I suppose it makes sense to be able to set CLOEXEC, but I could
> imagine a case where a handler wanted to fork+exec to handle
> something. I'm happy to make the change, but it's not obvious to me
> that it's what we want by default.

I said "either 0 or O_CLOEXEC" - I just meant that O_RDWR doesn't make
much sense to me here, given that that's not a property of the fd and
will be ignored by the function you're calling.

On whether 0 or O_CLOEXEC is better: If you look at
get_unused_fd_flags() calls in e.g. various ioctl handlers, it's a mix
of places that hardcode 0, places that hardcode O_CLOEXEC, and places
that allow the caller to specify the flag. Either should work - but
personally, I believe that if the caller can't pass a flag,
get_unused_fd_flags(O_CLOEXEC) is the better choice because you can
still clear the O_CLOEXEC flag using fcntl() if necessary, while
setting the flag using fcntl() is potentially racy in a multi-threaded
context.