Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180531144949.24995-1-tycho@tycho.ws> <20180531144949.24995-2-tycho@tycho.ws>
 <CAG48ez3x9HSkLZc-da6zjPbckyfdPvwyQa3p9P6EzQ5sGk53eg@mail.gmail.com> <20180604001812.GE15998@cisco>
In-Reply-To: <20180604001812.GE15998@cisco>
From:   Jann Horn <jannh@google.com>
Date:   Wed, 13 Jun 2018 17:32:57 +0200
Message-ID: <CAG48ez2ZD1fMFcTfro-axUzC7g2Nb4ACnHPKXgr2q=_BchJuYg@mail.gmail.com>
Subject: Re: [PATCH v3 1/4] seccomp: add a return code to trap to userspace
To:     Tycho Andersen <tycho@tycho.ws>
Cc:     kernel list <linux-kernel@vger.kernel.org>,
        containers@lists.linux-foundation.org,
        Kees Cook <keescook@chromium.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Tyler Hicks <tyhicks@canonical.com>,
        suda.akihiro@lab.ntt.co.jp, "Tobin C. Harding" <me@tobin.cc>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 4, 2018 at 2:18 AM Tycho Andersen <tycho@tycho.ws> 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@tycho.ws> 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.