From: Serge E. Hallyn on
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
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
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
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
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/