The primary exception to Yama's descendant-based PTRACE restrictions
is when an application has a predefined crash handler that is spawned
in parallel with the crashed application (e.g. KDE, Chromium). These
applications want to bypass the common RLIMIT_CORE=0, and gather state
information from the process for upstream problem reporting. When the
main application crashes, it generally has access to the PID of what
will debug it (e.g. when a KDE app crashes, it knows the parent PID of
the debugger that will be spawned).
So, since this programmatic method of PTRACEing is useful, there should be
a way for processes to actively declare who can PTRACE them. This patch
adds a prctl hook for Yama so that processes can exempt themselves from
the PTRACE restrictions in the case of a crash when they know their
debugger's PID.
As a matter of demonstration, here is what the patch to KDE4 would look
like to support Yama, or other PTRACE-restricting LSMs that wanted to grant
a similar exception:
--- kde4libs-4.4.90.orig/kdeui/util/kcrash.cpp 2010-06-28 17:07:28.667869954 -0700
+++ kde4libs-4.4.90/kdeui/util/kcrash.cpp 2010-06-28 17:09:32.089958401 -0700
@@ -41,6 +41,7 @@
#include <sys/wait.h>
#include <sys/un.h>
#include <sys/socket.h>
+#include <sys/prctl.h>
#include <errno.h>
#include <qwindowdefs.h>
@@ -437,6 +438,7 @@
//if the process was started directly, use waitpid(), as it's a child...
while(waitpid(-1, NULL, 0) != pid) {}
} else {
+ prctl(PR_SET_PTRACER, pid, 0, 0, 0);
//...else poll its status using kill()
while(kill(pid, 0) >= 0) {
sleep(1);
--
Kees Cook
Ubuntu Security Team
The current LSM interface to cred_free is not sufficient for allowing
an LSM to track the life and death of a task. This patch adds the
task_free hook so that an LSM can clean up resources on task death.
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/security.h | 8 ++++++++
kernel/fork.c | 1 +
security/capability.c | 4 ++++
security/security.c | 5 +++++
4 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 723a93d..8007495 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -637,6 +637,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* manual page for definitions of the @clone_flags.
* @clone_flags contains the flags indicating what should be shared.
* Return 0 if permission is granted.
+ * @task_free:
+ * @task task being freed
+ * Handle release of task-related resources.
* @cred_alloc_blank:
* @cred points to the credentials.
* @gfp indicates the atomicity of any memory allocations.
@@ -1481,6 +1484,7 @@ struct security_operations {
int (*dentry_open) (struct file *file, const struct cred *cred);
int (*task_create) (unsigned long clone_flags);
+ void (*task_free) (struct task_struct *task);
int (*cred_alloc_blank) (struct cred *cred, gfp_t gfp);
void (*cred_free) (struct cred *cred);
int (*cred_prepare)(struct cred *new, const struct cred *old,
@@ -1732,6 +1736,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
int security_file_receive(struct file *file);
int security_dentry_open(struct file *file, const struct cred *cred);
int security_task_create(unsigned long clone_flags);
+void security_task_free(struct task_struct *task);
int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
void security_cred_free(struct cred *cred);
int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
@@ -2232,6 +2237,9 @@ static inline int security_task_create(unsigned long clone_flags)
return 0;
}
+static inline int security_task_free(struct task_struct *task)
+{ }
+
static inline int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
{
return 0;
diff --git a/kernel/fork.c b/kernel/fork.c
index b6cce14..7fbc5e3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -183,6 +183,7 @@ void __put_task_struct(struct task_struct *tsk)
WARN_ON(atomic_read(&tsk->usage));
WARN_ON(tsk == current);
+ security_task_free(tsk);
exit_creds(tsk);
delayacct_tsk_free(tsk);
put_signal_struct(tsk->signal);
diff --git a/security/capability.c b/security/capability.c
index 4aeb699..3649d4b 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -353,6 +353,9 @@ static int cap_task_create(unsigned long clone_flags)
return 0;
}
+static void cap_task_free(struct task_struct *task)
+{ }
+
static int cap_cred_alloc_blank(struct cred *cred, gfp_t gfp)
{
return 0;
@@ -936,6 +939,7 @@ void __init security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, file_receive);
set_to_cap_if_null(ops, dentry_open);
set_to_cap_if_null(ops, task_create);
+ set_to_cap_if_null(ops, task_free);
set_to_cap_if_null(ops, cred_alloc_blank);
set_to_cap_if_null(ops, cred_free);
set_to_cap_if_null(ops, cred_prepare);
diff --git a/security/security.c b/security/security.c
index e8c87b8..103c35f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -691,6 +691,11 @@ int security_task_create(unsigned long clone_flags)
return security_ops->task_create(clone_flags);
}
+void security_task_free(struct task_struct *task)
+{
+ security_ops->task_free(task);
+}
+
int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
{
return security_ops->cred_alloc_blank(cred, gfp);
--
1.7.1
--
Kees Cook
Ubuntu Security Team
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]>
---
include/linux/prctl.h | 6 ++
security/yama/yama_lsm.c | 205 ++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 193 insertions(+), 18 deletions(-)
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index a3baeb2..e435624 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -102,4 +102,10 @@
#define PR_MCE_KILL_GET 34
+/*
+ * Set specific pid that is allowed to PTRACE the current task.
+ * A value of 0 mean "no process".
+ */
+#define PR_SET_PTRACER 35
+
#endif /* _LINUX_PRCTL_H */
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 72929d2..0e989c4 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -14,15 +14,187 @@
#include <linux/security.h>
#include <linux/sysctl.h>
#include <linux/ptrace.h>
+#include <linux/prctl.h>
#include <linux/ratelimit.h>
static int ptrace_scope = 1;
static int protected_sticky_symlinks = 1;
static int protected_nonaccess_hardlinks = 1;
+/* describe a PTRACE relationship for potential exception */
+struct ptrace_relation {
+ struct task_struct *tracer;
+ struct task_struct *tracee;
+ struct list_head node;
+};
+
+static LIST_HEAD(ptracer_relations);
+static spinlock_t ptracer_relations_lock;
+
+/**
+ * yama_ptracer_add - add 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
+ *
+ * Returns 0 if relationship was added, -ve on error.
+ */
+static int yama_ptracer_add(struct task_struct *tracer,
+ struct task_struct *tracee)
+{
+ struct ptrace_relation *relation;
+
+ relation = kzalloc(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);
+ spin_unlock(&ptracer_relations_lock);
+
+ return 0;
+}
+
+/**
+ * yama_ptracer_del - remove exceptions related to the given tasks
+ * @tracer: remove any relation where tracer task matches
+ * @tracee: remove any relation where tracee task matches
+ */
+static void yama_ptracer_del(struct task_struct *tracer,
+ struct task_struct *tracee)
+{
+ struct ptrace_relation *relation;
+ struct list_head *list, *safe;
+
+ spin_lock(&ptracer_relations_lock);
+ list_for_each_safe(list, safe, &ptracer_relations) {
+ relation = list_entry(list, struct ptrace_relation, node);
+ if (relation->tracee == tracee ||
+ relation->tracer == tracer) {
+ list_del(&relation->node);
+ kfree(relation);
+ }
+ }
+ spin_unlock(&ptracer_relations_lock);
+}
+
+/**
+ * yama_task_free - check for task_pid to remove from exception list
+ * @task: task being removed
+ */
+static void yama_task_free(struct task_struct *task)
+{
+ yama_ptracer_del(task, task);
+}
+
+/**
+ * yama_task_prctl - check for Yama-specific prctl operations
+ * @option: operation
+ * @arg2: argument
+ * @arg3: argument
+ * @arg4: argument
+ * @arg5: argument
+ *
+ * Return 0 on success, -ve on error. -ENOSYS is returned when Yama
+ * does not handle the given option.
+ */
+static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+ unsigned long arg4, unsigned long arg5)
+{
+ int rc;
+
+ rc = cap_task_prctl(option, arg2, arg3, arg4, arg5);
+ if (rc != -ENOSYS)
+ return rc;
+
+ switch (option) {
+ case PR_SET_PTRACER:
+ if (arg2 == 0) {
+ yama_ptracer_del(NULL, current);
+ rc = 0;
+ } else {
+ struct task_struct *tracer;
+
+ rcu_read_lock();
+ tracer = find_task_by_vpid(arg2);
+ if (tracer)
+ get_task_struct(tracer);
+ else
+ rc = -EINVAL;
+ rcu_read_unlock();
+
+ if (tracer) {
+ rc = yama_ptracer_add(tracer, current);
+ put_task_struct(tracer);
+ }
+ }
+ break;
+ }
+
+ return rc;
+}
+
+/**
+ * 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 (walker == parent) {
+ rc = 1;
+ break;
+ }
+ walker = walker->real_parent;
+ }
+ read_unlock(&tasklist_lock);
+ rcu_read_unlock();
+
+ return rc;
+}
+
+/**
+ * ptracer_exception_found - tracer registered as exception for this tracee
+ * @tracer: the task_struct of the process attempting PTRACE
+ * @tracee: the task_struct of the process to be PTRACEd
+ *
+ * Returns 1 if tracer has is ptracer exception ancestor for tracee.
+ */
+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);
+
+ return rc;
+}
+
/**
* yama_ptrace_access_check - validate PTRACE_ATTACH calls
- * @child: child task pointer
+ * @child: task that current task is attempting to PTRACE
* @mode: ptrace attach mode
*
* Returns 0 if following the ptrace is allowed, -ve on error.
@@ -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);
- 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) &&
+ !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
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().
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
Quoting Kees Cook ([email protected]):
> 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:
> @@ -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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Serge,
On Tue, Jun 29, 2010 at 10:56:09PM -0500, Serge E. Hallyn wrote:
> Quoting Kees Cook ([email protected]):
> > 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!
> > 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
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.
Quoting Kees Cook ([email protected]):
> Hi Serge,
>
> On Tue, Jun 29, 2010 at 10:56:09PM -0500, Serge E. Hallyn wrote:
> > Quoting Kees Cook ([email protected]):
> > > 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!
>
> > > 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
On Tue, Jun 29, 2010 at 8:40 PM, Kees Cook <[email protected]> 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 <[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.....
-Eric
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
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 <[email protected]> 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 <[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.....
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
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;
> }
On Wed, 30 Jun 2010, 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.
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 people won't use any of SELinux, Smack, Tomoyo or AppArmor, then I
don't think providing an ad-hoc assortment of workarounds with no overall
design is going to help them either.
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.
- James
--
James Morris
<[email protected]>
Hi James,
On Thu, Jul 01, 2010 at 11:39:01AM +1000, James Morris wrote:
> On Wed, 30 Jun 2010, 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.
>
> 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.
That's certainly my intent, but PTRACE is kind of a nasty beast.
> 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.
> 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
way for the kernel to intuit the relationship without the tracee
specifically declaring which process could PTRACE it, though. Regardless
of the method, I think userspace changes would be needed for this kind of
thing. I spent a little time discussing this within Ubuntu[1].
> 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.
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.
Thanks,
-Kees
[1] https://lists.ubuntu.com/archives/ubuntu-devel/2010-June/030939.html
--
Kees Cook
Ubuntu Security Team
Quoting Kees Cook ([email protected]):
> > 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.
I've been jumping from one conviction to another to yet another and back
again on this.
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.
> > 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.
> 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.
-serge
On Thu, 2010-07-01 at 08:20 -0500, Serge E. Hallyn wrote:
> 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.
TRACEME is an alternative mechanism (to ATTACH) for establishing a
tracing relationship, not a mechanism for expressing policy over ATTACH
requests. The prctl was solely for configuring (Yama-specific) policy.
> 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).
Or to express whatever ptrace relationships you want to allow,
regardless of process ancestry.
> One q then is whether YAMA wants to provide task tracking of its own, or
> stack with apparmor.
Last I looked, apparmor did not support any kind of fine-grained ptrace
constraints, just a simple unconfined || same-profile || CAP_SYS_PTRACE
check. Whereas SELinux lets you control it based on the particular
domain pairing.
> > > 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.
Exactly. What we do not want to do is to have the applications
configuring policy on the fly.
> > 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.
I think the final form of the stacker patches put each value on its own
line with the module name in parentheses after it. But it required a
compatibility hack for SELinux and ultimately I think a libselinux patch
to support SELinux stacked with anything else that wanted to
use /proc/self/attr.
--
Stephen Smalley
National Security Agency
On Thu, Jul 01, 2010 at 08:20:39AM -0500, Serge E. Hallyn wrote:
> 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.
> 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.
> 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...
> > 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.)
> > 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.
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.
-Kees
--
Kees Cook
Ubuntu Security Team
Quoting Kees Cook ([email protected]):
> On Thu, Jul 01, 2010 at 08:20:39AM -0500, Serge E. Hallyn wrote:
> > 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.
>
> > 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.
>
> > 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...
(see below)
> > > 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
In SELinux you can specify which security contexts can be targets too.
I.e.
allow serge_t serge_firefox_t:process ptrace;
I guess that in apparmor, it probably wouldn't quite be conventional to
specify '/usr/bin/firefox' as a target meaning any task confined in the
/usr/bin/firefox profile...
In smack, tasks have simple labels just like objects, and I believe
ptrace is taken as rw access to the label.
So, you say that
> wheel, but from what I can see, no other LSM can do what I want...
It depends on if you want exactly what you say you want. You can do
fine-grained ptrace controls based on security domains of both source
and target. As for specifying one specific pid of a task which may
ptrace me, no, that doesn't work right now, but I'm not convinced it'd
be a good thing.
> 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.)
>
> > > 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.
>
> 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).
-serge
On Thu, 2010-07-01 at 14:41 -0500, Serge E. Hallyn wrote:
> Quoting Kees Cook ([email protected]):
> > > > 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.
> >
> > 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).
Patch series was also available from:
http://sourceforge.net/projects/lsm-stacker/files/
Looks like it was last updated in 2006.
--
Stephen Smalley
National Security Agency