linux-kernel - Re: [PATCH 4.14 20/65] ptrace: reintroduce usage of subjective credentials in ptrace_has

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] [day] [month] [year] [list]

Date:   Mon, 20 Apr 2020 15:15:09 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sasha Levin <sashal@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Oleg Nesterov <oleg@...hat.com>,
        Eric Paris <eparis@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Serge Hallyn <serge@...lyn.com>, Jann Horn <jannh@...gle.com>,
        Christian Brauner <christian.brauner@...ntu.com>
Subject: Re: [PATCH 4.14 20/65] ptrace: reintroduce usage of subjective
 credentials in ptrace_has_cap()

On Fri, 2020-01-24 at 08:38 +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 23, 2020 at 03:01:29PM -0800, Guenter Roeck wrote:
> > On Wed, Jan 22, 2020 at 10:29:05AM +0100, Greg Kroah-Hartman wrote:
> > > From: Christian Brauner <christian.brauner@...ntu.com>
> > > 
> > > commit 6b3ad6649a4c75504edeba242d3fd36b3096a57f upstream.
[...]
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -258,12 +258,17 @@ static int ptrace_check_attach(struct ta
> > >  	return ret;
> > >  }
> > >  
> > > -static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> > > +static bool ptrace_has_cap(const struct cred *cred, struct user_namespace *ns,
> > > +			   unsigned int mode)
> > >  {
> > > +	int ret;
> > > +
> > >  	if (mode & PTRACE_MODE_NOAUDIT)
> > > -		return has_ns_capability_noaudit(current, ns, CAP_SYS_PTRACE);
> > > +		ret = security_capable(cred, ns, CAP_SYS_PTRACE);
> > >  	else
> > > -		return has_ns_capability(current, ns, CAP_SYS_PTRACE);
> > > +		ret = security_capable(cred, ns, CAP_SYS_PTRACE);
> > > +
> > > +	return ret == 0;
> > 
> > This results in
> > 	if (condition)
> > 		do_something;
> > 	else
> > 		do_the_same;
> > 
> > Is that really correct ? The upstream patch calls security_capable()
> > with additional CAP_OPT_NOAUDIT vs. CAP_OPT_NONE parameter, which does
> > make sense. But I don't really see the benefit of the change above.
> 
> Yeah, this is odd, and differs from the original version I applied to
> the staging queue.
> 
> Sasha, you made this change to the patch, I'm guessing to make it build
> properly in 4.14?  Should I just have dropped it from there instead?

To make this work properly, you would need to pick these for 4.14 as
well:

11c92f144bf3 apparmor: fix mediation of prlimit
(to avoid a conflict, but it's an important fix in its own right)

c1a85a00ea66 LSM: generalize flag passing to security_capable

I'm attaching a backport of the second commit.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                               A fail-safe circuit will destroy others.



View attachment "0001-LSM-generalize-flag-passing-to-security_capable.patch" of type "text/x-patch" (18959 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)