[PATCH] proc: fix a race in do_io_accounting()

[PATCH] proc: fix a race in do_io_accounting()

[Vasiliy Kulikov]

There is a ptrace_may_access() check in do_io_accounting() to prevent
gathering information of setuid'ed and similar binaries.  However, there
is a race against execve().  Holding task->signal->cred_guard_mutex
while gathering the information should protect against the race.

The order of locking is similar to the one inside of
ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Cc: stable@kernel.org
---
 fs/proc/base.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 083a4f2..c605ee1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2711,9 +2711,12 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result = -EACCES;
+
+	mutex_lock(&task->signal->cred_guard_mutex);
 
 	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+		goto out_unlock;
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;
@@ -2724,7 +2727,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"
@@ -2739,6 +2742,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
-- 
1.7.0.4

Re: [PATCH] proc: fix a race in do_io_accounting()

[Linus Torvalds]

On Sun, Jul 3, 2011 at 3:39 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
>
> The order of locking is similar to the one inside of
> ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

Hmm. mm_for_maps() uses mutex_lock_killable(), as does lock_trace.

And neither proc_pid_wchan() nor the fd following ones
(proc_pid_follow_link etc) use anything at all.

So I'm not sure. And do we really even care about the theoretical
race? Even if we do hit the race window and happen to get it just as a
process turns setuid, it would seem to be totally harmless (we're not
going to see any of the sensitive IO anyway).

So considering the lack of consistency in this area, I can't really
find it in myself to care very deeply.

That said, the lack of consistency itself is a bit annoying and
worrisome. Maybe some kind of helper like we do have for
"mm_for_maps()" would be a good idea - not because the potential races
are all that worrisome, but because inconsistencies in the kernel are
always signs of confusion, and confusion is always bad and a breeding
ground for potential bugs.

                           Linus

Re: [PATCH] proc: fix a race in do_io_accounting()

[Vasiliy Kulikov]

On Sun, Jul 03, 2011 at 12:24 -0700, Linus Torvalds wrote:
> On Sun, Jul 3, 2011 at 3:39 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> >
> > The order of locking is similar to the one inside of
> > ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().
> 
> Hmm. mm_for_maps() uses mutex_lock_killable(), as does lock_trace.

Killable/interruptable here makes sense.


> And neither proc_pid_wchan() nor the fd following ones
> (proc_pid_follow_link etc) use anything at all.
> 
> So I'm not sure. And do we really even care about the theoretical
> race? Even if we do hit the race window and happen to get it just as a
> process turns setuid, it would seem to be totally harmless (we're not
> going to see any of the sensitive IO anyway).

I consider this as a theoretical race too unless there is a crazy bug in
scheduler/timer.  But IMO it's better to just fully remove the risk
(even purely theoretical) given the lock is simple and it doesn't cost
much.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[PATCH v2] proc: fix a race in do_io_accounting()

[Vasiliy Kulikov]

There is a ptrace_may_access() check in do_io_accounting() to prevent
gathering information of setuid'ed and similar binaries.  However, there
is a race against execve().  Holding task->signal->cred_guard_mutex
while gathering the information should protect against the race.

The order of locking is similar to the one inside of
ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

v2 - use mutex_lock_killable() instead of mutex_lock().

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Cc: stable@kernel.org
---
 fs/proc/base.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 083a4f2..4b9f159 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2711,9 +2711,16 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (result)
+		return result;
+
+	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+		result = -EACCES;
+		goto out_unlock;
+	}
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;
@@ -2724,7 +2731,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"
@@ -2739,6 +2746,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
-- 
1.7.0.4

Re: [PATCH v2] proc: fix a race in do_io_accounting()

[Andrew Morton]

<wakes up>

On Tue, 5 Jul 2011 00:13:39 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> There is a ptrace_may_access() check in do_io_accounting() to prevent
> gathering information of setuid'ed and similar binaries.  However, there
> is a race against execve().  Holding task->signal->cred_guard_mutex
> while gathering the information should protect against the race.
> 
> The order of locking is similar to the one inside of
> ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().
> 
> v2 - use mutex_lock_killable() instead of mutex_lock().
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> Cc: stable@kernel.org

If a patch is to be backported into -stable then its changelog had
better explain why such a thing is needed.  This one doesn't.

Please provide a full description of the conseuqences of the bug.  One
which will permit the -stable maintainers to understand why they're
merging the patch, and one which will help distribution maintainers
decide whether they want to merge it as well.

[PATCH v3] proc: fix a race in do_io_accounting()

[Vasiliy Kulikov]

If inode's mode permits to open /proc/PID/io and the resulted file
descriptor is kept across execve() of setuid or similar binary, the
ptrace_may_access() check tries to prevent using this fd against the
task with escalated privileges.  Unfortunately, there is a race of the
check against execve().  If execve() is processed after the ptrace
check, but before the actual io information gathering, io statistics
will be gathered from the privileged process.  At least in theory this
might lead to gathering sensible information (like ssh/ftp password
length) that wouldn't be available otherwise.

Holding task->signal->cred_guard_mutex while gathering the io
information should protect against the race.

The order of locking is similar to the one inside of
ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

v3 - better description.
v2 - use mutex_lock_killable() instead of mutex_lock().

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Cc: stable@kernel.org
---
 fs/proc/base.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 083a4f2..4b9f159 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2711,9 +2711,16 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (result)
+		return result;
+
+	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+		result = -EACCES;
+		goto out_unlock;
+	}
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;
@@ -2724,7 +2731,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"
@@ -2739,6 +2746,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
-- 
1.7.0.4

Re: [PATCH v3] proc: fix a race in do_io_accounting()

[Vasiliy Kulikov]

Hi Linus,

On Wed, Jul 06, 2011 at 20:34 +0400, Vasiliy Kulikov wrote:
> If inode's mode permits to open /proc/PID/io and the resulted file
> descriptor is kept across execve() of setuid or similar binary, the
> ptrace_may_access() check tries to prevent using this fd against the
> task with escalated privileges.  Unfortunately, there is a race of the
> check against execve().  If execve() is processed after the ptrace
> check, but before the actual io information gathering, io statistics
> will be gathered from the privileged process.  At least in theory this
> might lead to gathering sensible information (like ssh/ftp password
> length) that wouldn't be available otherwise.
> 
> Holding task->signal->cred_guard_mutex while gathering the io
> information should protect against the race.
> 
> The order of locking is similar to the one inside of
> ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

Any problems with the patch?

> v3 - better description.
> v2 - use mutex_lock_killable() instead of mutex_lock().
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> Cc: stable@kernel.org
> ---
>  fs/proc/base.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 083a4f2..4b9f159 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2711,9 +2711,16 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  {
>  	struct task_io_accounting acct = task->ioac;
>  	unsigned long flags;
> +	int result;
>  
> -	if (!ptrace_may_access(task, PTRACE_MODE_READ))
> -		return -EACCES;
> +	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
> +	if (result)
> +		return result;
> +
> +	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
> +		result = -EACCES;
> +		goto out_unlock;
> +	}
>  
>  	if (whole && lock_task_sighand(task, &flags)) {
>  		struct task_struct *t = task;
> @@ -2724,7 +2731,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  
>  		unlock_task_sighand(task, &flags);
>  	}
> -	return sprintf(buffer,
> +	result = sprintf(buffer,
>  			"rchar: %llu\n"
>  			"wchar: %llu\n"
>  			"syscr: %llu\n"
> @@ -2739,6 +2746,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  			(unsigned long long)acct.read_bytes,
>  			(unsigned long long)acct.write_bytes,
>  			(unsigned long long)acct.cancelled_write_bytes);
> +out_unlock:
> +	mutex_unlock(&task->signal->cred_guard_mutex);
> +	return result;
>  }
>  
>  static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
> -- 
> 1.7.0.4
> 

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH v3] proc: fix a race in do_io_accounting()

[Linus Torvalds]

On Thu, Jul 14, 2011 at 11:38 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
>
> Any problems with the patch?

I still think that the problem you fixed is simply not important, and
the problem that I actually might care about (the ad-hoc and
inconsistent rules) remains.

So the patch is just a "not important enough to care about" for me,
and if it comes in through Andrew I'll probably take it, but it's
merge-window material rather than anything else.

                              Linus