Re: [RFC 5/5] user namespaces: Allow ptrace from non-init user namespaces

[Serge Hallyn <serge.hallyn@canonical.com>]

Thanks for reviewing, Eric.

Quoting Eric W. Biederman (ebiederm@xmission.com):
> > +static inline int may_ptrace_ns(struct task_struct *t)
> 
> Can we name this ptrace_capable?  Since you are only
> wrapping the capability check?  With a name like may_ptrace_ns
> I imagine very different semantics.

Hm, the whole structure here could probably stand to be improved
anyway.  I just can't quite think how.  I'll rename it as you
suggest for starters, just not sure if it'll continue to exist.

> 
> > +{
> > +	struct user_namespace *ns;
> > +	int ret;
> > +
> > +	rcu_read_lock();
> > +	ns = task_cred_xxx(t, user)->user_ns;
> > +	ret = ns_capable(ns, CAP_SYS_PTRACE);
> > +	rcu_read_unlock();
> > +
> > +	return ret;
> > +}
> > +
> >  int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> >  {
> >  	const struct cred *cred = current_cred(), *tcred;
> > @@ -134,21 +147,24 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> >  		return 0;
> >  	rcu_read_lock();
> >  	tcred = __task_cred(task);
> > -	if ((cred->uid != tcred->euid ||
> > -	     cred->uid != tcred->suid ||
> > -	     cred->uid != tcred->uid  ||
> > -	     cred->gid != tcred->egid ||
> > -	     cred->gid != tcred->sgid ||
> > -	     cred->gid != tcred->gid) &&
> > -	    !capable(CAP_SYS_PTRACE)) {
> > -		rcu_read_unlock();
> > -		return -EPERM;
> > -	}
> > +	if (cred->user->user_ns == tcred->user->user_ns &&
> > +	    (cred->uid == tcred->euid ||
> > +	     cred->uid == tcred->suid ||
> > +	     cred->uid == tcred->uid  ||
> > +	     cred->gid == tcred->egid ||
> > +	     cred->gid == tcred->sgid ||
> > +	     cred->gid == tcred->gid))
> > +		goto ok;
> 
> This needs to be:
> > +	if (cred->user->user_ns == tcred->user->user_ns &&
> > +	    (cred->uid == tcred->euid &&
> > +	     cred->uid == tcred->suid &&
> > +	     cred->uid == tcred->uid  &&
> > +	     cred->gid == tcred->egid &&
> > +	     cred->gid == tcred->sgid &&
> > +	     cred->gid == tcred->gid))
> > +		goto ok;

Hm, I started to explain why it doesn't, but you're right.
If any of the uids are different, then you must have
CAP_SYS_PTRACE or be denied.

> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -136,12 +136,20 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
> >  int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
> >  {
> >  	int ret = 0;
> > +	struct cred *cred, *tcred;
> >  
> >  	rcu_read_lock();
> > -	if (!cap_issubset(__task_cred(child)->cap_permitted,
> > -			  current_cred()->cap_permitted) &&
> > +	cred = current_cred();
> > +	tcred = __task_cred(child);
> > +	if (cred->user->user_ns != tcred->user->user_ns) {
> 
> This probably deserves a comment about why cap_issubset isn't
> needed here.  Aka we implicitly have all caps in child user namespaces
> so if we have CAP_SYS_PTRACE we know we have them all.

(going strictly by the rules which fall out from the original intent
of ns_capable) :

There is a case where that isn't true - if I'm user B in userns 3, and
user A in userns 3 created the userns 4 in which this target task, owned
by user C, sits.  Then user B does not have all capabilities to userns 4,
but any calculated capabilities which B has, are also valid in userns 4.

I'd still claim that capabilities aren't really comparable (because
they are targeted at different user namespaces), and therefore the
CAP_SYS_PTRACE should be sufficient for this case.  But maybe that's
not as practical.  Maybe the cap_issubset check should be there after
all.

> > +		if (!ns_capable(tcred->user->user_ns, CAP_SYS_PTRACE))
> > +			ret = -EPERM;
> > +		goto out;
> > +	}
> > +	if (!cap_issubset(tcred->cap_permitted, cred->cap_permitted) &&
> >  	    !capable(CAP_SYS_PTRACE))
> >  		ret = -EPERM;
> > +out:
> >  	rcu_read_unlock();
> >  	return ret;
> >  }
> > @@ -156,12 +164,20 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
> >  int cap_ptrace_traceme(struct task_struct *parent)
> >  {
> >  	int ret = 0;
> > +	struct cred *cred, *tcred;
> >  
> >  	rcu_read_lock();
> > -	if (!cap_issubset(current_cred()->cap_permitted,
> > -			  __task_cred(parent)->cap_permitted) &&
> > -	    !has_capability(parent, CAP_SYS_PTRACE))
> > +	cred = __task_cred(parent);
> > +	tcred = current_cred();
> > +	if (cred->user->user_ns != tcred->user->user_ns) {
> > +		if (!has_ns_capability(parent, tcred->user->user_ns, CAP_SYS_PTRACE))
> > +			ret = -EPERM;
> > +		goto out;
> > +	}
> > +	if (!cap_issubset(tcred->cap_permitted, cred->cap_permitted) &&
> > +	    !has_ns_capability(parent, tcred->user->user_ns, CAP_SYS_PTRACE))
> >  		ret = -EPERM;
> > +out:
> >  	rcu_read_unlock();
> >  	return ret;
> >  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html