diff mbox

[lucid,lucid/fsl-imx51,CVE,5/5] deal with races in /proc/*/{syscall, stack, personality}

Message ID 1311254026-29719-7-git-send-email-apw@canonical.com
State New
Headers show

Commit Message

Andy Whitcroft July 21, 2011, 1:13 p.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

All of those are rw-r--r-- and all are broken for suid - if you open
a file before the target does suid-root exec, you'll be still able
to access it.  For personality it's not a big deal, but for syscall
and stack it's a real problem.

Fix: check that task is tracable for you at the time of read().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

(backported from commit a9712bc12c40c172e393f85a9b2ba8db4bf59509)
CVE-2011-1020
BugLink: http://bugs.launchpad.net/bugs/813026
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/proc/base.c |   69 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 50 insertions(+), 19 deletions(-)

Comments

Tim Gardner July 21, 2011, 6:13 p.m. UTC | #1 
On 07/21/2011 06:13 AM, Andy Whitcroft wrote:
> From: Al Viro<viro@zeniv.linux.org.uk>
>
> All of those are rw-r--r-- and all are broken for suid - if you open
> a file before the target does suid-root exec, you'll be still able
> to access it.  For personality it's not a big deal, but for syscall
> and stack it's a real problem.
>
> Fix: check that task is tracable for you at the time of read().
>
> Signed-off-by: Al Viro<viro@zeniv.linux.org.uk>
>
> (backported from commit a9712bc12c40c172e393f85a9b2ba8db4bf59509)
> CVE-2011-1020
> BugLink: http://bugs.launchpad.net/bugs/813026
> Signed-off-by: Andy Whitcroft<apw@canonical.com>
> ---
>   fs/proc/base.c |   69 ++++++++++++++++++++++++++++++++++++++++---------------
>   1 files changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index e901438..80d3a59 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -330,6 +330,23 @@ static int proc_pid_wchan(struct task_struct *task, char *buffer)
>   }
>   #endif /* CONFIG_KALLSYMS */
>
> +static int lock_trace(struct task_struct *task)
> +{
> +	int err = mutex_lock_killable(&task->cred_guard_mutex);
> +	if (err)
> +		return err;
> +	if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
> +		mutex_unlock(&task->cred_guard_mutex);
> +		return -EPERM;
> +	}
> +	return 0;
> +}
> +
> +static void unlock_trace(struct task_struct *task)
> +{
> +	mutex_unlock(&task->cred_guard_mutex);
> +}
> +
>   #ifdef CONFIG_STACKTRACE
>
>   #define MAX_STACK_TRACE_DEPTH	64
> @@ -339,6 +356,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>   {
>   	struct stack_trace trace;
>   	unsigned long *entries;
> +	int err;
>   	int i;
>
>   	entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
> @@ -349,15 +367,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>   	trace.max_entries	= MAX_STACK_TRACE_DEPTH;
>   	trace.entries		= entries;
>   	trace.skip		= 0;
> -	save_stack_trace_tsk(task,&trace);
>
> -	for (i = 0; i<  trace.nr_entries; i++) {
> -		seq_printf(m, "[<%p>] %pS\n",
> -			   (void *)entries[i], (void *)entries[i]);
> +	err = lock_trace(task);
> +	if (!err) {
> +		save_stack_trace_tsk(task,&trace);
> +
> +		for (i = 0; i<  trace.nr_entries; i++) {
> +			seq_printf(m, "[<%p>] %pS\n",
> +				   (void *)entries[i], (void *)entries[i]);
> +		}
> +		unlock_trace(task);
>   	}
>   	kfree(entries);
>
> -	return 0;
> +	return err;
>   }
>   #endif
>
> @@ -529,18 +552,22 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
>   {
>   	long nr;
>   	unsigned long args[6], sp, pc;
> +	int res = lock_trace(task);
> +	if (res)
> +		return res;
>
>   	if (task_current_syscall(task,&nr, args, 6,&sp,&pc))
> -		return sprintf(buffer, "running\n");
> -
> -	if (nr<  0)
> -		return sprintf(buffer, "%ld 0x%lx 0x%lx\n", nr, sp, pc);
> -
> -	return sprintf(buffer,
> +		res = sprintf(buffer, "running\n");
> +	else if (nr<  0)
> +		res = sprintf(buffer, "%ld 0x%lx 0x%lx\n", nr, sp, pc);
> +	else
> +		res = sprintf(buffer,
>   		       "%ld 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx\n",
>   		       nr,
>   		       args[0], args[1], args[2], args[3], args[4], args[5],
>   		       sp, pc);
> +	unlock_trace(task);
> +	return res;
>   }
>   #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
>
> @@ -2494,8 +2521,12 @@ static int proc_tgid_io_accounting(struct task_struct *task, char *buffer)
>   static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
>   				struct pid *pid, struct task_struct *task)
>   {
> -	seq_printf(m, "%08x\n", task->personality);
> -	return 0;
> +	int err = lock_trace(task);
> +	if (!err) {
> +		seq_printf(m, "%08x\n", task->personality);
> +		unlock_trace(task);
> +	}
> +	return err;
>   }
>
>   /*
> @@ -2514,13 +2545,13 @@ static const struct pid_entry tgid_base_stuff[] = {
>   	REG("environ",    S_IRUSR, proc_environ_operations),
>   	INF("auxv",       S_IRUSR, proc_pid_auxv),
>   	ONE("status",     S_IRUGO, proc_pid_status),
> -	ONE("personality", S_IRUSR, proc_pid_personality),
> +	ONE("personality", S_IRUGO, proc_pid_personality),
>   	INF("limits",	  S_IRUSR, proc_pid_limits),
>   #ifdef CONFIG_SCHED_DEBUG
>   	REG("sched",      S_IRUGO|S_IWUSR, proc_pid_sched_operations),
>   #endif
>   #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> -	INF("syscall",    S_IRUSR, proc_pid_syscall),
> +	INF("syscall",    S_IRUGO, proc_pid_syscall),
>   #endif
>   	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
>   	ONE("stat",       S_IRUGO, proc_tgid_stat),
> @@ -2548,7 +2579,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>   	INF("wchan",      S_IRUGO, proc_pid_wchan),
>   #endif
>   #ifdef CONFIG_STACKTRACE
> -	ONE("stack",      S_IRUSR, proc_pid_stack),
> +	ONE("stack",      S_IRUGO, proc_pid_stack),
>   #endif
>   #ifdef CONFIG_SCHEDSTATS
>   	INF("schedstat",  S_IRUGO, proc_pid_schedstat),
> @@ -2853,13 +2884,13 @@ static const struct pid_entry tid_base_stuff[] = {
>   	REG("environ",   S_IRUSR, proc_environ_operations),
>   	INF("auxv",      S_IRUSR, proc_pid_auxv),
>   	ONE("status",    S_IRUGO, proc_pid_status),
> -	ONE("personality", S_IRUSR, proc_pid_personality),
> +	ONE("personality", S_IRUGO, proc_pid_personality),
>   	INF("limits",	 S_IRUSR, proc_pid_limits),
>   #ifdef CONFIG_SCHED_DEBUG
>   	REG("sched",     S_IRUGO|S_IWUSR, proc_pid_sched_operations),
>   #endif
>   #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> -	INF("syscall",   S_IRUSR, proc_pid_syscall),
> +	INF("syscall",   S_IRUGO, proc_pid_syscall),
>   #endif
>   	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
>   	ONE("stat",      S_IRUGO, proc_tid_stat),
> @@ -2886,7 +2917,7 @@ static const struct pid_entry tid_base_stuff[] = {
>   	INF("wchan",     S_IRUGO, proc_pid_wchan),
>   #endif
>   #ifdef CONFIG_STACKTRACE
> -	ONE("stack",      S_IRUSR, proc_pid_stack),
> +	ONE("stack",      S_IRUGO, proc_pid_stack),
>   #endif
>   #ifdef CONFIG_SCHEDSTATS
>   	INF("schedstat", S_IRUGO, proc_pid_schedstat),
diff mbox

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e901438..80d3a59 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -330,6 +330,23 @@  static int proc_pid_wchan(struct task_struct *task, char *buffer)
 }
 #endif /* CONFIG_KALLSYMS */
 
+static int lock_trace(struct task_struct *task)
+{
+	int err = mutex_lock_killable(&task->cred_guard_mutex);
+	if (err)
+		return err;
+	if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
+		mutex_unlock(&task->cred_guard_mutex);
+		return -EPERM;
+	}
+	return 0;
+}
+
+static void unlock_trace(struct task_struct *task)
+{
+	mutex_unlock(&task->cred_guard_mutex);
+}
+
 #ifdef CONFIG_STACKTRACE
 
 #define MAX_STACK_TRACE_DEPTH	64
@@ -339,6 +356,7 @@  static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 {
 	struct stack_trace trace;
 	unsigned long *entries;
+	int err;
 	int i;
 
 	entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
@@ -349,15 +367,20 @@  static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 	trace.max_entries	= MAX_STACK_TRACE_DEPTH;
 	trace.entries		= entries;
 	trace.skip		= 0;
-	save_stack_trace_tsk(task, &trace);
 
-	for (i = 0; i < trace.nr_entries; i++) {
-		seq_printf(m, "[<%p>] %pS\n",
-			   (void *)entries[i], (void *)entries[i]);
+	err = lock_trace(task);
+	if (!err) {
+		save_stack_trace_tsk(task, &trace);
+
+		for (i = 0; i < trace.nr_entries; i++) {
+			seq_printf(m, "[<%p>] %pS\n",
+				   (void *)entries[i], (void *)entries[i]);
+		}
+		unlock_trace(task);
 	}
 	kfree(entries);
 
-	return 0;
+	return err;
 }
 #endif
 
@@ -529,18 +552,22 @@  static int proc_pid_syscall(struct task_struct *task, char *buffer)
 {
 	long nr;
 	unsigned long args[6], sp, pc;
+	int res = lock_trace(task);
+	if (res)
+		return res;
 
 	if (task_current_syscall(task, &nr, args, 6, &sp, &pc))
-		return sprintf(buffer, "running\n");
-
-	if (nr < 0)
-		return sprintf(buffer, "%ld 0x%lx 0x%lx\n", nr, sp, pc);
-
-	return sprintf(buffer,
+		res = sprintf(buffer, "running\n");
+	else if (nr < 0)
+		res = sprintf(buffer, "%ld 0x%lx 0x%lx\n", nr, sp, pc);
+	else
+		res = sprintf(buffer,
 		       "%ld 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx 0x%lx\n",
 		       nr,
 		       args[0], args[1], args[2], args[3], args[4], args[5],
 		       sp, pc);
+	unlock_trace(task);
+	return res;
 }
 #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
 
@@ -2494,8 +2521,12 @@  static int proc_tgid_io_accounting(struct task_struct *task, char *buffer)
 static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 				struct pid *pid, struct task_struct *task)
 {
-	seq_printf(m, "%08x\n", task->personality);
-	return 0;
+	int err = lock_trace(task);
+	if (!err) {
+		seq_printf(m, "%08x\n", task->personality);
+		unlock_trace(task);
+	}
+	return err;
 }
 
 /*
@@ -2514,13 +2545,13 @@  static const struct pid_entry tgid_base_stuff[] = {
 	REG("environ",    S_IRUSR, proc_environ_operations),
 	INF("auxv",       S_IRUSR, proc_pid_auxv),
 	ONE("status",     S_IRUGO, proc_pid_status),
-	ONE("personality", S_IRUSR, proc_pid_personality),
+	ONE("personality", S_IRUGO, proc_pid_personality),
 	INF("limits",	  S_IRUSR, proc_pid_limits),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",      S_IRUGO|S_IWUSR, proc_pid_sched_operations),
 #endif
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",    S_IRUSR, proc_pid_syscall),
+	INF("syscall",    S_IRUGO, proc_pid_syscall),
 #endif
 	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
@@ -2548,7 +2579,7 @@  static const struct pid_entry tgid_base_stuff[] = {
 	INF("wchan",      S_IRUGO, proc_pid_wchan),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUSR, proc_pid_stack),
+	ONE("stack",      S_IRUGO, proc_pid_stack),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat",  S_IRUGO, proc_pid_schedstat),
@@ -2853,13 +2884,13 @@  static const struct pid_entry tid_base_stuff[] = {
 	REG("environ",   S_IRUSR, proc_environ_operations),
 	INF("auxv",      S_IRUSR, proc_pid_auxv),
 	ONE("status",    S_IRUGO, proc_pid_status),
-	ONE("personality", S_IRUSR, proc_pid_personality),
+	ONE("personality", S_IRUGO, proc_pid_personality),
 	INF("limits",	 S_IRUSR, proc_pid_limits),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",     S_IRUGO|S_IWUSR, proc_pid_sched_operations),
 #endif
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",   S_IRUSR, proc_pid_syscall),
+	INF("syscall",   S_IRUGO, proc_pid_syscall),
 #endif
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
@@ -2886,7 +2917,7 @@  static const struct pid_entry tid_base_stuff[] = {
 	INF("wchan",     S_IRUGO, proc_pid_wchan),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUSR, proc_pid_stack),
+	ONE("stack",      S_IRUGO, proc_pid_stack),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat", S_IRUGO, proc_pid_schedstat),