Re: [PATCH v4] seccomp: add ptrace options for suspend/resume

On Fri, Jun 12, 2015 at 04:29:00PM -0700, Kees Cook wrote:

On Fri, Jun 12, 2015 at 4:27 PM, Andy Lutomirski [off-list ref] wrote:

quoted

On Wed, Jun 10, 2015 at 1:18 PM, Kees Cook [off-list ref] wrote:

quoted

On Wed, Jun 10, 2015 at 10:20 AM, Andy Lutomirski [off-list ref] wrote:

quoted

On Wed, Jun 10, 2015 at 9:31 AM, Oleg Nesterov [off-list ref] wrote:

quoted

On 06/09, Andy Lutomirski wrote:

quoted

On Tue, Jun 9, 2015 at 5:49 PM, Tycho Andersen

quoted

@@ -556,6 +556,15 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data)
        if (data & ~(unsigned long)PTRACE_O_MASK)
                return -EINVAL;

+       if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) {

Well, we should do this if

                        (data & O_SUSPEND) && !(flags & O_SUSPEND)

or at least if

                        (data ^ flags) & O_SUSPEND

quoted

quoted

+               if (!config_enabled(CONFIG_CHECKPOINT_RESTORE) ||
+                   !config_enabled(CONFIG_SECCOMP))
+                       return -EINVAL;
+
+               if (!capable(CAP_SYS_ADMIN))
+                       return -EPERM;

I tend to think that we should also require that current not be using
seccomp.  Otherwise, in principle, there's a seccomp bypass for
privileged-but-seccomped programs.

Andy, I simply can't understand why do we need any security check at all.

OK, yes, in theory we can have a seccomped CAP_SYS_ADMIN process, seccomp
doesn't filter ptrace, you hack that process and force it to attach to
another CAP_SYS_ADMIN/seccomped process, etc, etc... Looks too paranoid
to me.

I've sometimes considered having privileged processes I write fork and
seccomp their child.  Of course, if you're allowing ptrace through
your seccomp filter, you open a giant can of worms, but I think we
should take the more paranoid approach to start and relax it later as
needed.  After all, for the intended use of this patch, stuff will
break regardless of what we do if the ptracer is itself seccomped.

I could be convinced that if the ptracer is outside seccomp then we
shouldn't need the CAP_SYS_ADMIN check.  That would at least make this
work in a user namespace.

But not if that namespace is running under a manager that has added a
seccomp filter to do things like drop finit_module, as lxc does.

In that case, criu isn't going to handle seccomp right regardless of
what our security check is, so I think we can safely deal with the
security aspects of that case once we figure out the functionality
part.

IOW, I think I still like the direct "you must not be seccomped in
order to suspend seccomp" rule.

Adding that restriction would be fine by me.

Ok, I just sent v5 with this change. I didn't carry your ack in the
hopes that I could get you to take this patch in the seccomp tree. Let
me know if that's not the right thing to do.

Thanks,

Tycho