linux-kernel - Re: [PATCH v2] seccomp: add ptrace options for suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150604210529.GJ3160@smitten>
Date:	Thu, 4 Jun 2015 15:05:29 -0600
From:	Tycho Andersen <tycho.andersen@...onical.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	Kees Cook <keescook@...omium.org>,
	Andy Lutomirski <luto@...capital.net>,
	Will Drewry <wad@...omium.org>,
	Roland McGrath <roland@...k.frob.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	"Serge E. Hallyn" <serge.hallyn@...ntu.com>
Subject: Re: [PATCH v2] seccomp: add ptrace options for suspend/resume

Hi Oleg,

On Thu, Jun 04, 2015 at 08:31:49PM +0200, Oleg Nesterov wrote:
> On 06/03, Tycho Andersen wrote:
> >
> > @@ -556,6 +557,11 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data)
> >  	if (data & ~(unsigned long)PTRACE_O_MASK)
> >  		return -EINVAL;
> >
> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > +	if (data & PTRACE_O_SUSPEND_SECCOMP && !may_suspend_seccomp())
> > +		return -EPERM;
> > +#endif
> > +
> 
> Well. This -EPERM doesn't look consistent...
> 
> if config_enabled(CONFIG_CHECKPOINT_RESTORE) == F, we return success
> but PTRACE_O_SUSPEND_SECCOMP has no effect because of another ifdef in
> seccomp.
>
> OTOH, if CONFIG_SECCOMP=n, this option has no effect too but we return
> -EPERM even.

Yes, something like:

if (unlikely(data & PTRACE_O_SUSPEND_SECCOMP)) {
        if (!config_enabled(CONFIG_CHECKPOINT_RESTORE) ||
            !config_enabled(CONFIG_SECCOMP))
                return -EINVAL;

        if (!may_suspend_seccomp())
                return -EPERM;
}

I guess.

> Also. Suppose that the tracer sets SUSPEND_SECCOMP and then drops
> CAP_SYS_ADMIN. After that it can't set or clear other ptrace options.

Is this a case we're concerned about? I think this should be ok (i.e.
"don't do that" :).

> So if we really want the security checks (I still think we do not ;)
> then we should probably check "flags & SUSPEND_SECCOMP" as well.

Good point.

> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > +bool may_suspend_seccomp(void)
> > +{
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return false;
> > +
> > +	if (current->seccomp.mode != SECCOMP_MODE_DISABLED)
> > +		return false;
> 
> Heh. OK, I won't argue with the new check too ;)

Actually now that I think about it I agree with you, these checks
don't seem necessary. Even inside a user namespace, if you can ptrace
a process you can make it do whatever you want irrespective of
seccomp, as long as it has the necessary capabilities. Once the
seccomp checks are run after ptrace, they'll be enforced so you
couldn't have it call whatever you want in the first place.

Still, perhaps I'm missing something...

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/