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: Serge E. Hallyn on 30 Jun 2010 08:40 Quoting Kees Cook (kees.cook(a)canonical.com): > 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 Haha, you're right, I looked at that wrong :) -serge -- 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: Eric Paris on 30 Jun 2010 11:50 On Tue, Jun 29, 2010 at 8:40 PM, Kees Cook <kees.cook(a)canonical.com> wrote: > 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> 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..... -Eric -- 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 11:50 Hi Christoph, On Wed, Jun 30, 2010 at 03:31:58AM -0400, Christoph Hellwig wrote: > 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. It's not my bad design; PTRACE is a terrible interface. In an effort to eliminate PTRACE, there are a few legitimate uses: direct debugging, and crash handlers. The crash handlers are an odd case because all they want is a backtrace and register details, but there's no way to do that on the fly without PTRACE, so that's how they've implemented it. In those cases, the crashing program knows who will attach to it, so there needs to be a safe way to declare that relationship instead of just giving up and saying "oh well, everything can PTRACE everything else". What is so objectionable about using a single PR_* value out of the 2147483614 available? -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: Kees Cook on 30 Jun 2010 12:00 Hi Eric, On Wed, Jun 30, 2010 at 11:41:26AM -0400, Eric Paris wrote: > On Tue, Jun 29, 2010 at 8:40 PM, Kees Cook <kees.cook(a)canonical.com> wrote: > > 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> > > 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..... Yes, thanks for noticing this; it seems the version I sent did not include the fixes I made at some point to correctly replace exceptions: diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c index f24b6b3..4f160db 100644 --- a/security/yama/yama_lsm.c +++ b/security/yama/yama_lsm.c @@ -32,7 +32,7 @@ static LIST_HEAD(ptracer_relations); static DEFINE_SPINLOCK(ptracer_relations_lock); /** - * yama_ptracer_add - add an exception for this tracer/tracee pair + * yama_ptracer_add - add/replace an exception for this tracer/tracee pair * @tracer: the task_struct of the process doing the PTRACE * @tracee: the task_struct of the process to be PTRACEd * @@ -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; - relation->tracee = tracee; spin_lock(&ptracer_relations_lock); - list_add(&relation->node, &ptracer_relations); + list_for_each_entry(entry, &ptracer_relations, node) + if (entry->tracee == tracee) { + relation = entry; + break; + } + if (!relation) { + relation = kmalloc(sizeof(*relation), GFP_KERNEL); + 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; } /** -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: Tetsuo Handa on 30 Jun 2010 17:40
Kees Cook wrote: > @@ -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; > - relation->tracee = tracee; > spin_lock(&ptracer_relations_lock); > - list_add(&relation->node, &ptracer_relations); > + list_for_each_entry(entry, &ptracer_relations, node) > + if (entry->tracee == tracee) { > + relation = entry; > + break; > + } > + if (!relation) { > + relation = kmalloc(sizeof(*relation), GFP_KERNEL); You can't use GFP_KERNEL here because a spinlock is held. > + 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; > } -- 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/ |