Yama: add PTRACE exception tracking

+static spinlock_t ptracer_relations_lock;

security/yama/yama_lsm.c +16 -0

+	if (!relation)
+		return -ENOMEM;
+	relation->tracer = tracer;
+	relation->tracee = tracee;
+	spin_lock(&ptracer_relations_lock);
+	list_add(&relation->node, &ptracer_relations);
+	spin_unlock(&ptracer_relations_lock);
+
+	return 0;
+}
 
+static int ptracer_exception_found(struct task_struct *tracer,
+				   struct task_struct *tracee)
+{
+	int rc = 0;
+	struct ptrace_relation *relation;
+	struct task_struct *parent = NULL;

Quoted 1 line · Kees Cook

+static spinlock_t ptracer_relations_lock;

static DEFINE_SPINLOCK(ptracer_relations_lock);

security/yama/yama_lsm.c line 46

Quoted 1 line

security/yama/yama_lsm.c +1 -0

+	relation = kzalloc(sizeof(*relation), GFP_KERNEL);

You can use kmalloc() since all fields are initialized within this function.

Quoted 12 lines

security/yama/yama_lsm.c +12 -0

+static int ptracer_exception_found(struct task_struct *tracer,
+				   struct task_struct *tracee)
+{
+	int rc = 0;
+	struct ptrace_relation *relation;
+	struct task_struct *parent = NULL;
+
+	spin_lock(&ptracer_relations_lock);
+	list_for_each_entry(relation, &ptracer_relations, node)
+		if (relation->tracee == tracee) {
+			parent = relation->tracer;
+			break;

security/yama/yama_lsm.c line 190

Quoted 4 lines

security/yama/yama_lsm.c +4 -0

+		}
+	if (task_is_descendant(parent, tracer))
+		rc = 1;
+	spin_unlock(&ptracer_relations_lock);

Can't we release ptracer_relations_lock before calling task_is_descendant() since task_is_descendant() won't access "struct ptrace_relation" on ptracer_relations list.

security/yama/yama_lsm.c static int yama_ptrace_access_check(struct task_struct *child, · line 208

Quoted 6 lines

security/yama/yama_lsm.c +2 -0

@@ -32,27 +204,20 @@ static int yama_ptrace_access_check(struct task_struct *child,
 {
 	int rc;
 
+	/* If standard caps disallows it, so does Yama.  We should
+	 * should only tighten restrictions further.

s/should should/should/

Some application suites have external crash handlers that depend on being able to use PTRACE to generate crash reports (KDE, Chromium, etc). Since the inferior process generally knows the PID of the debugger, it can use PR_SET_PTRACER to allow a specific PID and its descendants to perform the PTRACE instead of only a direct ancestor.

Signed-off-by Kees Cook <[email protected]>

---

security/yama/yama_lsm.c +1 -10

-	if (rc != 0)
+	if (rc)
 		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;

security/yama/yama_lsm.c +7 -0

+	    !task_is_descendant(current, child) &&
+	    !ptracer_exception_found(current, child))
+		rc = -EPERM;
 
 	if (rc) {
 		char name[sizeof(current->comm)];

@@ -170,6 +335,8 @@ static struct security_operations yama_ops = {
 	.ptrace_access_check =	yama_ptrace_access_check,
 	.inode_follow_link =	yama_inode_follow_link,
 	.path_link =		yama_path_link,
+	.task_prctl =		yama_task_prctl,
+	.task_free =		yama_task_free,
 };
 
 #ifdef CONFIG_SYSCTL

@@ -221,6 +388,8 @@ static __init int yama_init(void)
 
 	printk(KERN_INFO "Yama: becoming mindful.\n");
 
+	spin_lock_init(&ptracer_relations_lock);
+
 	if (register_security(&yama_ops))
 		panic("Yama: kernel registration failed.\n");
 

Signature · 10 lines

1.7.1


-- 
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Kees Cook

Some application suites have external crash handlers that depend on being able to use PTRACE to generate crash reports (KDE, Chromium, etc). Since the inferior process generally knows the PID of the debugger, it can use PR_SET_PTRACER to allow a specific PID and its descendants to perform the PTRACE instead of only a direct ancestor.

Signed-off-by Kees Cook <[email protected]>

---

Hi Kees - very nice, overall. One little note though:

Quoted 1 line

rc = cap_ptrace_access_check(child, mode);

This means that if capable(CAP_SYS_PTRACE) we'll always shortcut here, so

security/yama/yama_lsm.c static int yama_ptrace_access_check(struct task_struct *child, · line 220

Quoted 6 lines

security/yama/yama_lsm.c +6 -0

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

You don't need the CAP_SYS_PTRACE check here AFAICS.

Hi Serge,

Quoted 11 lines · Serge E. Hallyn

Quoted 8 lines · Kees Cook

Some application suites have external crash handlers that depend on being able to use PTRACE to generate crash reports (KDE, Chromium, etc). Since the inferior process generally knows the PID of the debugger, it can use PR_SET_PTRACER to allow a specific PID and its descendants to perform the PTRACE instead of only a direct ancestor.

Signed-off-by Kees Cook <[email protected]>

---

Hi Kees - very nice, overall. One little note though:

Thanks for looking it over!

Quoted 5 lines

Quoted 1 line

rc = cap_ptrace_access_check(child, mode);

This means that if capable(CAP_SYS_PTRACE) we'll always shortcut here, so

Quoted 8 lines

security/yama/yama_lsm.c static int yama_ptrace_access_check(struct task_struct *child, · line 220

Quoted 6 lines

security/yama/yama_lsm.c +6 -0

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

You don't need the CAP_SYS_PTRACE check here AFAICS.

I don't think that's true -- the capable(CAP_SYS_PTRACE) tests are always done in the negative since we only ever abort with error

Some application suites have external crash handlers that depend on being able to use PTRACE to generate crash reports (KDE, Chromium, etc). Since the inferior process generally knows the PID of the debugger, it can use PR_SET_PTRACER to allow a specific PID and its descendants to perform the PTRACE instead of only a direct ancestor.

Signed-off-by Kees Cook <[email protected]>

Quoted 7 lines · Kees Cook

Some application suites have external crash handlers that depend on being able to use PTRACE to generate crash reports (KDE, Chromium, etc). Since the inferior process generally knows the PID of the debugger, it can use PR_SET_PTRACER to allow a specific PID and its descendants to perform the PTRACE instead of only a direct ancestor.

Signed-off-by Kees Cook <[email protected]>

any normal unpriv application:

while(1) {
        prctl(PR_SET_PTRACER, 1, 0, 0, 0);
}

watch kernel run out of memory and bring down the box. Seems like quite the DoS.....

security/yama/yama_lsm.c +2 -5

@@ -41,18 +41,30 @@ static DEFINE_SPINLOCK(ptracer_relations_lock);
 static int yama_ptracer_add(struct task_struct *tracer,
 			    struct task_struct *tracee)
 {
-	struct ptrace_relation *relation;
+	int rc = 0;
+	struct ptrace_relation *entry, *relation = NULL;
 
-	relation = kmalloc(sizeof(*relation), GFP_KERNEL);
-	if (!relation)
-		return -ENOMEM;
-	relation->tracer = tracer;

security/yama/yama_lsm.c +11 -1

+		if (!relation) {
+			rc = -ENOMEM;
+			goto unlock_out;
+		}
+		relation->tracee = tracee;
+		list_add(&relation->node, &ptracer_relations);
+	}
+	relation->tracer = tracer;
+
+unlock_out:
 	spin_unlock(&ptracer_relations_lock);
 
-	return 0;
+	return rc;
 }

Err, no. This is just a very clear sign that your ptrace restrictions were completely wrong to start with and break applications left, right and center. Just get rid of it instead of letting workarounds for your bad design creep into the core kernel and applications.

Err, no. This is just a very clear sign that your ptrace restrictions were completely wrong to start with and break applications left, right and center. Just get rid of it instead of letting workarounds for your bad design creep into the core kernel and applications.

Quoted 4 lines · Christoph Hellwig

Err, no. This is just a very clear sign that your ptrace restrictions were completely wrong to start with and break applications left, right and center. Just get rid of it instead of letting workarounds for your bad design creep into the core kernel and applications.

Indeed, I wasn't aware that there were further aspects to this -- I thought it was a relatively simple case of restricting a problematic OS feature for heavily locked down systems.

This is getting more complicated, with fine-grained security policy now being introduced, also with the need to modify applications.

There are several existing LSMs with the ability to control ptrace, but as part of a system-wide, coherent, analyzable policy -- often in support of specific security models for which there is concrete user demand and benefit.

If LSMs need to call into common code in Yama, or even do lightweight chaining, that's one thing, but for Yama to evolve into yet another standalone security scheme, is something entirely different.

Quoted 2 lines

This is getting more complicated, with fine-grained security policy now being introduced, also with the need to modify applications.

Well, I'm trying to solve what I think is a core problem with PTRACE -- it is too permissive. I'm happy to look at it from other angles if it doesn't make sense for this kind of thing to live in Yama. I'm already very happy with the existing restrictions available in Yama.

Quoted 4 lines

There are several existing LSMs with the ability to control ptrace, but as part of a system-wide, coherent, analyzable policy -- often in support of specific security models for which there is concrete user demand and benefit.

Sure. I am curious, though, is there a way for SELinux (or maybe Smack, since it has more dynamic labels) to declare this kind of on-runtime PTRACE relationship? Maybe I overlooked some options for this. I didn't see any

I still think simple chaining is the way to go. I want to review the earlier discussions first (I think Serge said it was in 2004ish?) before I write up anything. There is, I think, one sticking point, which is /proc/self/attr/current, but beyond that, I think some simple reorganization of LSM initialization routines and a list that security_* walks would be sufficient.

First off, if you consider PTRACE_PTRACEME, and just consider this a more finegrained targeted version of that, it doesn't seem all that gross. So maybe that's my fault for suggesting prctl as an easier-to-use in LSMs api, bc using a PTRACE_PTRACEDBY might just look cleaner.

Still, you say 'ptrace is too permissive', but a rebuttal to that is that, in a DAC system, ptrace uses what credentials it knows of to authorize. *You* can make it more finegrained by not insisting on running everything as a single user.

What you now are trying to do is find a new, natural relationship between tasks on a plain DAC system to provide finer-grained control. The one you picked - process ancestry - doesn't perfectly fit, so you add changes and make it less clean. The criticism of that is valid and needs to be discusssed.

Adding new relationships between tasks is what LSMs do - based on the policy-defined relationships between the security tasks of the respective domains. And it feeld natural there - so it's natural for SELinux and apparmor to confine firefox to a domain that can't ptrace anything else (and maybe not itself).

One q then is whether YAMA wants to provide task tracking of its own, or stack with apparmor.

Quoted 8 lines

Quoted 4 lines

There are several existing LSMs with the ability to control ptrace, but as part of a system-wide, coherent, analyzable policy -- often in support of specific security models for which there is concrete user demand and benefit.

Sure. I am curious, though, is there a way for SELinux (or maybe Smack, since it has more dynamic labels) to declare this kind of on-runtime PTRACE relationship? Maybe I overlooked some options for this. I didn't see any

In SELinux, you could give a debugger or crash handler an unprivileged, but allowed-to-ptrace-the-main-app domain.

Quoted 6 lines

I still think simple chaining is the way to go. I want to review the earlier discussions first (I think Serge said it was in 2004ish?) before I write up anything. There is, I think, one sticking point, which is /proc/self/attr/current, but beyond that, I think some simple reorganization of LSM initialization routines and a list that security_* walks would be sufficient.

In the past, output results for each LSM were simply split by \n or a :
or something, and input was prepended by the LSM name.

First off, if you consider PTRACE_PTRACEME, and just consider this a more finegrained targeted version of that, it doesn't seem all that gross. So maybe that's my fault for suggesting prctl as an easier-to-use in LSMs api, bc using a PTRACE_PTRACEDBY might just look cleaner.

Still, you say 'ptrace is too permissive', but a rebuttal to that is that, in a DAC system, ptrace uses what credentials it knows of to authorize. *You* can make it more finegrained by not insisting on running everything as a single user.

What you now are trying to do is find a new, natural relationship between tasks on a plain DAC system to provide finer-grained control. The one you picked - process ancestry - doesn't perfectly fit, so you add changes and make it less clean. The criticism of that is valid and needs to be discusssed.

One q then is whether YAMA wants to provide task tracking of its own, or stack with apparmor.

Quoted 3 lines

Sure. I am curious, though, is there a way for SELinux (or maybe Smack, since it has more dynamic labels) to declare this kind of on-runtime PTRACE relationship? Maybe I overlooked some options for this. I didn't see any

In SELinux, you could give a debugger or crash handler an unprivileged, but allowed-to-ptrace-the-main-app domain.

Quoted 6 lines

I still think simple chaining is the way to go. I want to review the earlier discussions first (I think Serge said it was in 2004ish?) before I write up anything. There is, I think, one sticking point, which is /proc/self/attr/current, but beyond that, I think some simple reorganization of LSM initialization routines and a list that security_* walks would be sufficient.

In the past, output results for each LSM were simply split by \n or a :
or something, and input was prepended by the LSM name.

Quoted 4 lines · Serge E. Hallyn

First off, if you consider PTRACE_PTRACEME, and just consider this a more finegrained targeted version of that, it doesn't seem all that gross. So maybe that's my fault for suggesting prctl as an easier-to-use in LSMs api, bc using a PTRACE_PTRACEDBY might just look cleaner.

Right, this was my thinking -- there is already one kind of declared relationship via TRACEME (though it's utility is for "pure" debugging). The other "regular" use of PTRACE is crash handlers, for which this is no declared relationship. (If you ignore simple DAC, of course.) The third PTRACE use is "arbitrary" debugging -- sysadmins or the like saying "wtf is that process DOING?"

When thinking about the PTRACE stuff originally, I hadn't realized the "crash handler" case. So "pure" was done via TRACEME, and "arbitrary" was done via CAP_SYS_PTRACE, but there wasn't a clear way to declare the "crash" case.

Quoted 10 lines

Still, you say 'ptrace is too permissive', but a rebuttal to that is that, in a DAC system, ptrace uses what credentials it knows of to authorize. *You* can make it more finegrained by not insisting on running everything as a single user.

What you now are trying to do is find a new, natural relationship between tasks on a plain DAC system to provide finer-grained control. The one you picked - process ancestry - doesn't perfectly fit, so you add changes and make it less clean. The criticism of that is valid and needs to be discusssed.

Actually, if you throw out process ancestry completely, and just use TRACEME and TRACEBY, everything still works. The idea would be to just toss out the old definition of DAC PTRACE permissions.

Quoted 2 lines

One q then is whether YAMA wants to provide task tracking of its own, or stack with apparmor.

This is why I asked the question below... I don't want to reinvent the wheel, but from what I can see, no other LSM can do what I want...

Quoted 6 lines

Quoted 3 lines

Sure. I am curious, though, is there a way for SELinux (or maybe Smack, since it has more dynamic labels) to declare this kind of on-runtime PTRACE relationship? Maybe I overlooked some options for this. I didn't see any

In SELinux, you could give a debugger or crash handler an unprivileged, but allowed-to-ptrace-the-main-app domain.

Right, same for AppArmor. With either system I can declare a binary as able to PTRACE another binary. This is _still_ too permissive, IMO. I

want a process to directly specify which other process should be allowed to do a PTRACE. The logic for this is, by its nature, only known to the tracee. (i.e. "Oh, I'm crashing now... trigger handler... allow PTRACE.")

(Though obviously this isn't safe if the crasher handler allows arbitrary control of the process -- otherwise "kill -SEGV ..." is all that's needed to subvert the tracee. The handler by its nature should just collect details and quit. It's not a "debugging" case, it's a "crash" case.)

Quoted 7 lines

Quoted 6 lines

I still think simple chaining is the way to go. I want to review the earlier discussions first (I think Serge said it was in 2004ish?) before I write up anything. There is, I think, one sticking point, which is /proc/self/attr/current, but beyond that, I think some simple reorganization of LSM initialization routines and a list that security_* walks would be sufficient.

Quoted 2 lines

In the past, output results for each LSM were simply split by \n or a :
or something, and input was prepended by the LSM name.

This doesn't appear to be true anymore. Looking at the fs/proc/base.c and security/selinux/hooks.c code, there is no checking for a prepended LSM name. Maybe that's the first chaining limitation -- you can't chain 2 LSMs that both declare setprocattr hooks.

Quoted 10 lines · Kees Cook

Quoted 9 lines

Quoted 6 lines

I still think simple chaining is the way to go. I want to review the earlier discussions first (I think Serge said it was in 2004ish?) before I write up anything. There is, I think, one sticking point, which is /proc/self/attr/current, but beyond that, I think some simple reorganization of LSM initialization routines and a list that security_* walks would be sufficient.

In the past, output results for each LSM were simply split by \n or a :
or something, and input was prepended by the LSM name.

Quoted 4 lines

This doesn't appear to be true anymore. Looking at the fs/proc/base.c and security/selinux/hooks.c code, there is no checking for a prepended LSM name. Maybe that's the first chaining limitation -- you can't chain 2 LSMs that both declare setprocattr hooks.

No no, Stephen and I were talking about in the stacker patchset, again around 2004-2005. Never went upstream (per 2005 or 2006 ksummit agreement).