[PATCH] Yama: turn process ancestry check into function

[PATCH] Yama: turn process ancestry check into function

[Kees Cook]

This cleans up the ancestry check by separating it out into its own
function, which makes the code a bit more readable.  Additionally, it
now checks for and uses the thread group leader since otherwise we may
end up missing potential matches on attaches to threads.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 security/yama/yama_lsm.c |   55 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 72929d2..3b76386 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -21,6 +21,40 @@ static int protected_sticky_symlinks = 1;
 static int protected_nonaccess_hardlinks = 1;
 
 /**
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+			      struct task_struct *child)
+{
+	int rc = 0;
+	struct task_struct *walker = child;
+
+	if (!parent || !child)
+		return 0;
+
+	rcu_read_lock();
+	read_lock(&tasklist_lock);
+	while (walker->pid > 0) {
+		if (!thread_group_leader(walker))
+			walker = walker->group_leader;
+		if (walker == parent) {
+			rc = 1;
+			break;
+		}
+		walker = walker->real_parent;
+	}
+	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
+
+	return rc;
+}
+
+
+/**
  * yama_ptrace_access_check - validate PTRACE_ATTACH calls
  * @child: child task pointer
  * @mode: ptrace attach mode
@@ -37,22 +71,11 @@ static int yama_ptrace_access_check(struct task_struct *child,
 		return rc;
 
 	/* require ptrace target be a child of ptracer on attach */
-	if (mode == PTRACE_MODE_ATTACH && ptrace_scope &&
-	    !capable(CAP_SYS_PTRACE)) {
-		struct task_struct *walker = child;
-
-		rcu_read_lock();
-		read_lock(&tasklist_lock);
-		while (walker->pid > 0) {
-			if (walker == current)
-				break;
-			walker = walker->real_parent;
-		}
-		if (walker->pid == 0)
-			rc = -EPERM;
-		read_unlock(&tasklist_lock);
-		rcu_read_unlock();
-	}
+	if (mode == PTRACE_MODE_ATTACH &&
+	    ptrace_scope &&
+	    !task_is_descendant(current, child) &&
+	    !capable(CAP_SYS_PTRACE))
+		rc = -EPERM;
 
 	if (rc) {
 		char name[sizeof(current->comm)];
-- 
1.7.1


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] Yama: turn process ancestry check into function

[Tetsuo Handa]

Kees Cook wrote:
> +static int task_is_descendant(struct task_struct *parent,
> +			      struct task_struct *child)
> +{
> +	int rc = 0;
> +	struct task_struct *walker = child;
> +
> +	if (!parent || !child)
> +		return 0;

parent (== current) is !NULL and
child (in original code) is !NULL.
You can remove this check unless you are planning to call
this function from other places.

> +	if (mode == PTRACE_MODE_ATTACH &&
> +	    ptrace_scope &&
> +	    !task_is_descendant(current, child) &&
> +	    !capable(CAP_SYS_PTRACE))
> +		rc = -EPERM;

I don't know how heavy capable(CAP_SYS_PTRACE) is.
But checking !capable(CAP_SYS_PTRACE) before
!task_is_descendant(current, child) might be lighter.

Re: [PATCH] Yama: turn process ancestry check into function

[Kees Cook]

On Wed, Jul 14, 2010 at 09:19:09AM +0900, Tetsuo Handa wrote:
> Kees Cook wrote:
> > +static int task_is_descendant(struct task_struct *parent,
> > +			      struct task_struct *child)
> > +{
> > +	int rc = 0;
> > +	struct task_struct *walker = child;
> > +
> > +	if (!parent || !child)
> > +		return 0;
> 
> parent (== current) is !NULL and
> child (in original code) is !NULL.
> You can remove this check unless you are planning to call
> this function from other places.

I'd like the flexibility to call it with NULLs.  But yes, at present, it
never will be NULL.

> > +	if (mode == PTRACE_MODE_ATTACH &&
> > +	    ptrace_scope &&
> > +	    !task_is_descendant(current, child) &&
> > +	    !capable(CAP_SYS_PTRACE))
> > +		rc = -EPERM;
> 
> I don't know how heavy capable(CAP_SYS_PTRACE) is.
> But checking !capable(CAP_SYS_PTRACE) before
> !task_is_descendant(current, child) might be lighter.

That's the order I had before, but in looking at some of the other code, it
seemed like moving it to the end made more logical sense.  Since checking
PTRACE attach isn't a common or time-sensitive operation, I figured trying
to tune it wasn't critical.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] Yama: turn process ancestry check into function

[Serge E. Hallyn]

Quoting Kees Cook (kees.cook@canonical.com):
> On Wed, Jul 14, 2010 at 09:19:09AM +0900, Tetsuo Handa wrote:
> > > +	if (mode == PTRACE_MODE_ATTACH &&
> > > +	    ptrace_scope &&
> > > +	    !task_is_descendant(current, child) &&
> > > +	    !capable(CAP_SYS_PTRACE))
> > > +		rc = -EPERM;
> > 
> > I don't know how heavy capable(CAP_SYS_PTRACE) is.
> > But checking !capable(CAP_SYS_PTRACE) before
> > !task_is_descendant(current, child) might be lighter.
> 
> That's the order I had before, but in looking at some of the other code, it
> seemed like moving it to the end made more logical sense.  Since checking
> PTRACE attach isn't a common or time-sensitive operation, I figured trying
> to tune it wasn't critical.

Yes the reason to keep it like this is that capable(CAP_SYS_PTRACE)
will set PF_SUPERPRIV if it passes.  You don't want to do that unless
the capability was actually required.

-serge

Re: [PATCH] Yama: turn process ancestry check into function

[James Morris]

On Tue, 13 Jul 2010, Kees Cook wrote:

> This cleans up the ancestry check by separating it out into its own
> function, which makes the code a bit more readable.  Additionally, it
> now checks for and uses the thread group leader since otherwise we may
> end up missing potential matches on attaches to threads.
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

-- 
James Morris
<jmorris@namei.org>