Yama: add PTRACE exception tracking
in [Kernel]
|
Prev: x86: enlightenment for ticket spin locks - eliminate NOPs introduced by first patch
Next: [PATCH 2/3] AFS: Use i_generation not i_version for the vnode uniquifier [ver #2]
From: Tetsuo Handa on 29 Jun 2010 21:20 Kees Cook wrote: > +static spinlock_t ptracer_relations_lock; static DEFINE_SPINLOCK(ptracer_relations_lock); > +static int yama_ptracer_add(struct task_struct *tracer, > + struct task_struct *tracee) > +{ > + struct ptrace_relation *relation; > + > + relation = kzalloc(sizeof(*relation), GFP_KERNEL); You can use kmalloc() since all fields are initialized within this function. > + 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; > + > + spin_lock(&ptracer_relations_lock); > + list_for_each_entry(relation, &ptracer_relations, node) > + if (relation->tracee == tracee) { > + parent = relation->tracer; > + break; > + } > + 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. > @@ -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/ > + */ > @@ -221,6 +388,8 @@ static __init int yama_init(void) > > printk(KERN_INFO "Yama: becoming mindful.\n"); > > + spin_lock_init(&ptracer_relations_lock); > + You can statically initialize by using DEFINE_SPINLOCK(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on 30 Jun 2010 00:00 Hi Tetsuo, On Wed, Jun 30, 2010 at 10:09:54AM +0900, Tetsuo Handa wrote: > Kees Cook wrote: > > +static spinlock_t ptracer_relations_lock; > static DEFINE_SPINLOCK(ptracer_relations_lock); Ah, very cool, I missed that while reading through spinlock code. :) > > + relation = kzalloc(sizeof(*relation), GFP_KERNEL); > You can use kmalloc() since all fields are initialized within this function. I wasn't sure if list_add needed a zeroed ->node, so I opted for safety here. Is list_add safe to use on an uninitialized ->node? (Looks like it is on code review, I'll just use regular kmalloc.) > > +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; > > + } > > + 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. This is where it gets a little funny. I need to keep that lock so that task_is_descendant isn't racing yama_task_free. I don't want to be in the position where I've left the lock only to have another CPU free the task_struct that was just located, so I have to keep the lock until I've finished using "parent". (And I can't take the task with get_task since it's already too late, and if I take it during _add, the task will never be freed.) > > @@ -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/ Agh, thanks. -Kees -- Kees Cook Ubuntu Security Team -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Serge E. Hallyn on 30 Jun 2010 00:00 Quoting Kees Cook (kees.cook(a)canonical.com): > 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 <kees.cook(a)canonical.com> > --- Hi Kees - very nice, overall. One little note though: > @@ -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. > + */ > rc = cap_ptrace_access_check(child, mode); This means that if capable(CAP_SYS_PTRACE) we'll always shortcut here, so > - 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; > - walker = walker->real_parent; > - } > - if (walker->pid == 0) > - rc = -EPERM; > - read_unlock(&tasklist_lock); > - rcu_read_unlock(); > - } > + if (mode == PTRACE_MODE_ATTACH && > + ptrace_scope && > + !capable(CAP_SYS_PTRACE) && You don't need the CAP_SYS_PTRACE check here AFAICS. > + !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"); > > -- > 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 majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on 30 Jun 2010 01:30 Hi Serge, On Tue, Jun 29, 2010 at 10:56:09PM -0500, Serge E. Hallyn wrote: > Quoting Kees Cook (kees.cook(a)canonical.com): > > 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 <kees.cook(a)canonical.com> > > --- > > Hi Kees - very nice, overall. One little note though: Thanks for looking it over! > > rc = cap_ptrace_access_check(child, mode); > > This means that if capable(CAP_SYS_PTRACE) we'll always shortcut > here, so > > > + 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 instead of forcing an early "okay" (see also __ptrace_may_access() in kernel/ptrace.c, where capable(CAP_SYS_PTRACE) is called repeatedly while evaluating various negative conditions). For cap_ptrace_access_check, capable(CAP_SYS_PTRACE) is only tested if the tracee's process permitted caps are not a subset of the tracer's. i.e. cap_ptrace_access_check will return 0 when either cap_issubset or capable. In the case of normal user processes or a tracer with greater caps, capable(CAP_SYS_PTRACE) will never be tested in cap_ptrace_access_check. As a result, I have to include it in the test here too. I guess it's arguable that I should move it to the end of the series of &&s, but it logically doesn't really matter. -Kees (Interestingly, this means that having CAP_SYS_PTRACE means a process effectively has all capabilities...) -- Kees Cook Ubuntu Security Team -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 30 Jun 2010 03:40
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |