[PATCH] sanitize task->comm to avoid leaking escape codes

[PATCH] sanitize task->comm to avoid leaking escape codes

[Kees Cook]

Through get_task_comm() and many direct uses of task->comm in the kernel,
it is possible for escape codes and other non-printables to leak into
dmesg, syslog, etc.  In the worst case, these strings could be used to
attack administrators using vulnerable terminal emulators, and at least
cause confusion through the injection of \r characters.

This patch sanitizes task->comm to only contain printable characters
when it is set.  Additionally, it redefines get_task_comm so that it
cannot be misused by callers (presently nothing was incorrectly calling
get_task_comm's unsafe use of strncpy).

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 fs/exec.c             |   17 +++++++++++++----
 include/linux/sched.h |    3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 85092e3..aa84f66 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -935,17 +935,18 @@ static void flush_old_files(struct files_struct * files)
 	spin_unlock(&files->file_lock);
 }
 
-char *get_task_comm(char *buf, struct task_struct *tsk)
+char *get_task_comm_size(char *buf, size_t len, struct task_struct *tsk)
 {
-	/* buf must be at least sizeof(tsk->comm) in size */
 	task_lock(tsk);
-	strncpy(buf, tsk->comm, sizeof(tsk->comm));
+	strlcpy(buf, tsk->comm, len);
 	task_unlock(tsk);
 	return buf;
 }
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
+	size_t i;
+
 	task_lock(tsk);
 
 	/*
@@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 */
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();
-	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+
+	/* sanitize non-printable characters */
+	for (i = 0; buf[i] && i < (sizeof(tsk->comm) - 1); i++) {
+		if (!isprint(buf[i]))
+			tsk->comm[i] = '?';
+		else
+			tsk->comm[i] = buf[i];
+	}
+
 	task_unlock(tsk);
 	perf_event_comm(tsk);
 }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f118809..0ba69dc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2117,7 +2117,8 @@ extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned lon
 struct task_struct *fork_idle(int);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
+extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
 extern unsigned long wait_task_inactive(struct task_struct *, long match_state);
-- 
1.7.1


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Oleg Nesterov]

On 06/23, Kees Cook wrote:
>
> @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
>  	 */
>  	memset(tsk->comm, 0, TASK_COMM_LEN);
>  	wmb();

Off-topic. I'd wish I could understand this barrier. Since the lockless
reader doesn't do rmb() I don't see how this can help. OTOH, I don't
understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

> -	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> +
> +	/* sanitize non-printable characters */
> +	for (i = 0; buf[i] && i < (sizeof(tsk->comm) - 1); i++) {
> +		if (!isprint(buf[i]))
> +			tsk->comm[i] = '?';
> +		else
> +			tsk->comm[i] = buf[i];
> +	}

Personally I think this makes sense.

> -extern char *get_task_comm(char *to, struct task_struct *tsk);
> +#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
> +extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);

Oh, but this means that get_task_comm(ptr, task) doesn't work?

Oleg.

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Alexey Dobriyan]

On Wed, Jun 23, 2010 at 09:41:45PM +0200, Oleg Nesterov wrote:
> On 06/23, Kees Cook wrote:
 
> > -extern char *get_task_comm(char *to, struct task_struct *tsk);
> > +#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
> > +extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);
> 
> Oh, but this means that get_task_comm(ptr, task) doesn't work?

The number of users is so small, and everyone uses TASK_COMM_LEN,
so maybe nothing should be done or "char buf[TASK_COMM_LEN]"?

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Kees Cook]

On Wed, Jun 23, 2010 at 11:23:35PM +0300, Alexey Dobriyan wrote:
> On Wed, Jun 23, 2010 at 09:41:45PM +0200, Oleg Nesterov wrote:
> > On 06/23, Kees Cook wrote:
>  
> > > -extern char *get_task_comm(char *to, struct task_struct *tsk);
> > > +#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
> > > +extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);
> > 
> > Oh, but this means that get_task_comm(ptr, task) doesn't work?
> 
> The number of users is so small, and everyone uses TASK_COMM_LEN,
> so maybe nothing should be done or "char buf[TASK_COMM_LEN]"?

I couldn't handle the pathological use of strncpy(dest, src, sizeof(src))
that is currently in get_task_comm; that's just asking for trouble.

If someone wants to use a ptr for get_task_comm, they would get to call
get_task_comm_size() instead?

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Andrew Morton]

On Wed, 23 Jun 2010 23:23:35 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Wed, Jun 23, 2010 at 09:41:45PM +0200, Oleg Nesterov wrote:
> > On 06/23, Kees Cook wrote:
>  
> > > -extern char *get_task_comm(char *to, struct task_struct *tsk);
> > > +#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
> > > +extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);
> > 
> > Oh, but this means that get_task_comm(ptr, task) doesn't work?
> 
> The number of users is so small, and everyone uses TASK_COMM_LEN,
> so maybe nothing should be done or "char buf[TASK_COMM_LEN]"?

Yup, it would take an act of wilful daftness to pass anything other
than a TASK_COMM_LEN array into get_task_comm().

I can't say I like the patch much - going in and altering a task's
comm[] behind its back seems fraught with all sorts of problems which I
can't immediately think of.  Do we corrupt (err, "fix")
/proc/pid/cmdline as well?

Surely it would be better to fix the tools which display this info
rather than making the kernel tell fibs.

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Kees Cook]

Hi,

On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote:
> Surely it would be better to fix the tools which display this info
> rather than making the kernel tell fibs.

The strncpy in get_task_comm() is totally wrong -- it's testing the length
of task->comm.  Why should get_task_comm not take a destination buffer
length argument?  At least consider v2 of the patch -- it just fixes the
get_task_comm definition and callers.

But, if not, then patches to the kernel that include
printk(..., get_task_comm(...) ...) shouldn't be considered flawed[1].

-Kees

[1] http://lkml.org/lkml/2010/6/23/132

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Artem Bityutskiy]

On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote:
> On 06/23, Kees Cook wrote:
> >
> > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
> >  	 */
> >  	memset(tsk->comm, 0, TASK_COMM_LEN);
> >  	wmb();
> 
> Off-topic. I'd wish I could understand this barrier. Since the lockless
> reader doesn't do rmb() I don't see how this can help.

This wmb() looks wrong to me as well. To achieve what the comment in
this function says, it should be smp_wmb() and we should have smp_rmb()
in the reading side, AFAIU.

>  OTOH, I don't
> understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

I think the idea was that readers can see incomplete names, but not
messed up names, consisting of old and new ones.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Artem Bityutskiy]

On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote:
> On 06/23, Kees Cook wrote:
> >
> > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
> >  	 */
> >  	memset(tsk->comm, 0, TASK_COMM_LEN);
> >  	wmb();
> 
> Off-topic. I'd wish I could understand this barrier. Since the lockless
> reader doesn't do rmb() I don't see how this can help.

This wmb() looks wrong to me as well. To achieve what the comment in
this function says, it should be smp_wmb() and we should have smp_rmb()
in the reading side, AFAIU.

>  OTOH, I don't
> understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

I think the idea was that readers can see incomplete names, but not
messed up names, consisting of old and new ones.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Alexey Dobriyan]

On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook@canonical.com> wrote:
> On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote:
>> Surely it would be better to fix the tools which display this info
>> rather than making the kernel tell fibs.
>
> The strncpy in get_task_comm() is totally wrong -- it's testing the length
> of task->comm.

It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.

> Why should get_task_comm not take a destination buffer length argument?

If you pass too small, you needlessly truncate output.
If you pass too large, you waste space.

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Oleg Nesterov]

On 06/29, Artem Bityutskiy wrote:
>
> On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote:
> > On 06/23, Kees Cook wrote:
> > >
> > > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
> > >  	 */
> > >  	memset(tsk->comm, 0, TASK_COMM_LEN);
> > >  	wmb();
> >
> > Off-topic. I'd wish I could understand this barrier. Since the lockless
> > reader doesn't do rmb() I don't see how this can help.
>
> This wmb() looks wrong to me as well. To achieve what the comment in
> this function says, it should be smp_wmb() and we should have smp_rmb()
> in the reading side, AFAIU.
>
> >  OTOH, I don't
> > understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.
>
> I think the idea was that readers can see incomplete names, but not
> messed up names, consisting of old and new ones.

OK, agreed, comm[TASK_COMM_LEN-1] == '0' can't help to avoid the
messed names.

But nether can help smp_rmb() in the reading (lockless) side?

Say,

	printk("comm=%s\n", current->comm);

if we add rmb() before printk, it can't make any difference. The lockless
code should do something like

	get_comm_lockless(char *to, char *comm)
	{
		while (*comm++ = *to++)
			rmb();
	}

to ensure it sees the result of

	memset(tsk->comm, 0, TASK_COMM_LEN);
	wmb();
	strcpy(tsk->comm, buf);

in the right order.

otherwise printk("comm=%s\n", current->comm) can read, say, comm[1]
before set_task_comm->memset(), and comm[0] after set_task_comm->strcpy().

So, afaics, set_task_comm()->wmb() buys nothing and should be removed.
The last zero char in task_struct->comm[] is always here, at least this
guarantees that strcpy(char *dest, tsk->comm) is always safe.

(I cc'ed the expert, Paul can correct me)

Oleg.

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Kees Cook]

On Tue, Jun 29, 2010 at 11:45:14AM +0300, Alexey Dobriyan wrote:
> On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook@canonical.com> wrote:
> > On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote:
> >> Surely it would be better to fix the tools which display this info
> >> rather than making the kernel tell fibs.
> >
> > The strncpy in get_task_comm() is totally wrong -- it's testing the length
> > of task->comm.
> 
> It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.
> 
> > Why should get_task_comm not take a destination buffer length argument?
> 
> If you pass too small, you needlessly truncate output.

If you pass too small a buffer, get_task_comm will happily write all over
the caller's stack past the end of the buffer if the contents of task->comm
are large enough:

        strncpy(buf, tsk->comm, sizeof(tsk->comm));

The "n" argument to get_task_comm's use of strncpy is totally wrong --
it needs to be the size of the destination, not the size of the source.
Luckily, everyone using get_task_comm currently uses buffers that are
sizeof(task->comm).

> If you pass too large, you waste space.

Right.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Paul E. McKenney]

On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote:
> On 06/29, Artem Bityutskiy wrote:
> >
> > On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote:
> > > On 06/23, Kees Cook wrote:
> > > >
> > > > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
> > > >  	 */
> > > >  	memset(tsk->comm, 0, TASK_COMM_LEN);
> > > >  	wmb();
> > >
> > > Off-topic. I'd wish I could understand this barrier. Since the lockless
> > > reader doesn't do rmb() I don't see how this can help.
> >
> > This wmb() looks wrong to me as well. To achieve what the comment in
> > this function says, it should be smp_wmb() and we should have smp_rmb()
> > in the reading side, AFAIU.
> >
> > >  OTOH, I don't
> > > understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.
> >
> > I think the idea was that readers can see incomplete names, but not
> > messed up names, consisting of old and new ones.
> 
> OK, agreed, comm[TASK_COMM_LEN-1] == '0' can't help to avoid the
> messed names.
> 
> But nether can help smp_rmb() in the reading (lockless) side?
> 
> Say,
> 
> 	printk("comm=%s\n", current->comm);
> 
> if we add rmb() before printk, it can't make any difference. The lockless
> code should do something like
> 
> 	get_comm_lockless(char *to, char *comm)
> 	{
> 		while (*comm++ = *to++)
> 			rmb();
> 	}
> 
> to ensure it sees the result of
> 
> 	memset(tsk->comm, 0, TASK_COMM_LEN);
> 	wmb();
> 	strcpy(tsk->comm, buf);
> 
> in the right order.
> 
> otherwise printk("comm=%s\n", current->comm) can read, say, comm[1]
> before set_task_comm->memset(), and comm[0] after set_task_comm->strcpy().
> 
> So, afaics, set_task_comm()->wmb() buys nothing and should be removed.
> The last zero char in task_struct->comm[] is always here, at least this
> guarantees that strcpy(char *dest, tsk->comm) is always safe.
> 
> (I cc'ed the expert, Paul can correct me)

First, all of the implementations that I can see do task_lock(tsk), which
should prevent readers from seeing any changes.  So I am guessing that
you guys want to allow readers to get at ->comm without having to acquire
this lock.

So if the reader uses no mutual exclusion, then that reader could be
preempted/interrupted/NMIed/cache-missed/whatever for quite some time.
The task's ->comm field might change once, twice, ten times in the
meantime.  The reader might then to so far as to get one character from
each of the multiple names that happened in the meantime.  So, how to fix?

Here are some approaches that come to mind off-hand:

1.	Use RCU (always my favorite, of course, but in this case...):

	The task structure adds a char* named ->commp that normally
	points to ->comm in the same task structure.  The updater
	changes the name as follows:

	a.	Allocate a chunk of memory to hold the new name.

	b.	Copy the desired string into this newly allocated
		chunk of memory.

	c.	rcu_assign_pointer() ->commp to point to this
		newly allocated chunk of memory.

	d.	synchronize_rcu().

	e.	Now all readers are looking at the new name, so
		copy the new name into ->comm.

	f.	rcu_assign_pointer() ->commp to point to ->comm.

	g.	synchronize_rcu().

	h.	free up the newly assigned chunk of memory.

	Then the reader does:

		rcu_read_lock()
		p = rcu_dereference(tsk->commp);
		do_something_with(p);
		rcu_read_unlock();

	This works, but is a bit complex, and adds not one but two
	grace periods to the updater.  Yes, it is possible to optimize
	the second grace period away in the case of back-to-back
	updaters, but...

2.	Use sequence locks:

	The updater just does the update under the sequence lock.

	The reader does the usual:

		do {
			seq = read_seqbegin(&tsk->commseqlock);
			do_something_with(tsk->comm);
		} while (read_seqretry(&tsk->commseqlock, seq));

	Of course, this can starve readers, which can retaliate
	using something like the following:

		i = 0;
		do {
			seq = read_seqbegin(&tsk->commseqlock);
			do_something_with(tsk->comm);
		} while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3);
		if (i >= 3) {
			write_seqlock(&tsk->commseqlock);
			do_something_with(tsk->comm);
			write_sequnlock(&tsk->commseqlock);
		}

	Readers might also need rcu_read_lock() to keep the task from
	disappearing out from under them:

		rcu_read_lock();
		tsk = ...
		...
		i = 0;
		do {
			seq = read_seqbegin(&tsk->commseqlock);
			do_something_with(tsk->comm);
		} while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3);
		if (i >= 3) {
			write_seqlock(&tsk->commseqlock);
			do_something_with(tsk->comm);
			write_sequnlock(&tsk->commseqlock);
		}
		rcu_read_unlock();

	But I have to defer to you guys on this one.  For example, in the
	case where tsk==current, I do not believe that rcu_read_lock()
	is needed.

3.	Various forms of reader-writer locks -- none of which seem to
	me to work very well in this case.

4.	Your approach here!  ;-)

							Thanx, Paul

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Oleg Nesterov]

On 06/29, Paul E. McKenney wrote:
>
> On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote:
>
> > So, afaics, set_task_comm()->wmb() buys nothing and should be removed.
> > The last zero char in task_struct->comm[] is always here, at least this
> > guarantees that strcpy(char *dest, tsk->comm) is always safe.
> >
> > (I cc'ed the expert, Paul can correct me)
>
> First, all of the implementations that I can see do task_lock(tsk), which
> should prevent readers from seeing any changes.  So I am guessing that
> you guys want to allow readers to get at ->comm without having to acquire
> this lock.

Ah, sorry for confusion.

No, we are not trying to invent the lockless get_task_comm(). I'd say
it is not needed, if we really care about the precise ->comm we can
take task->alloc_lock.

The only problem is that I believe that set_task_comm() wrongly pretends
wmb() can help the lockless reader, it does:

	task_lock(tsk);

	/*
	 * Threads may access current->comm without holding
	 * the task lock, so write the string carefully.
	 * Readers without a lock may see incomplete new
	 * names but are safe from non-terminating string reads.
	 */
	memset(tsk->comm, 0, TASK_COMM_LEN);
	wmb();
	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
	task_unlock(tsk);

but afaics this wmb() buys absolutely nothing if we race with the
reader doing, say,

	printk("my name is %s\n", current->comm);

Afaics, this wmb()

	- can't prevent from printing the mixture of the old/new data

	- is not needed to make strcpy(somewhere, task->comm) safe,
	  the final char is always '0', we never change it.

	- adds the unnecessary confusion

> [... snip a lot of good ideas ...]

Thanks a lot, Paul ;)

Oleg.

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Paul E. McKenney]

On Tue, Jun 29, 2010 at 07:18:46PM +0200, Oleg Nesterov wrote:
> On 06/29, Paul E. McKenney wrote:
> >
> > On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote:
> >
> > > So, afaics, set_task_comm()->wmb() buys nothing and should be removed.
> > > The last zero char in task_struct->comm[] is always here, at least this
> > > guarantees that strcpy(char *dest, tsk->comm) is always safe.
> > >
> > > (I cc'ed the expert, Paul can correct me)
> >
> > First, all of the implementations that I can see do task_lock(tsk), which
> > should prevent readers from seeing any changes.  So I am guessing that
> > you guys want to allow readers to get at ->comm without having to acquire
> > this lock.
> 
> Ah, sorry for confusion.
> 
> No, we are not trying to invent the lockless get_task_comm(). I'd say
> it is not needed, if we really care about the precise ->comm we can
> take task->alloc_lock.
> 
> The only problem is that I believe that set_task_comm() wrongly pretends
> wmb() can help the lockless reader, it does:
> 
> 	task_lock(tsk);
> 
> 	/*
> 	 * Threads may access current->comm without holding
> 	 * the task lock, so write the string carefully.
> 	 * Readers without a lock may see incomplete new
> 	 * names but are safe from non-terminating string reads.
> 	 */
> 	memset(tsk->comm, 0, TASK_COMM_LEN);
> 	wmb();
> 	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> 	task_unlock(tsk);
> 
> but afaics this wmb() buys absolutely nothing if we race with the
> reader doing, say,
> 
> 	printk("my name is %s\n", current->comm);
> 
> Afaics, this wmb()
> 
> 	- can't prevent from printing the mixture of the old/new data
> 
> 	- is not needed to make strcpy(somewhere, task->comm) safe,
> 	  the final char is always '0', we never change it.
> 
> 	- adds the unnecessary confusion

I agree -- I cannot see how this wmb() can help.

> > [... snip a lot of good ideas ...]
> 
> Thanks a lot, Paul ;)

Glad you liked them.  ;-)

							Thanx, Paul

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Andrew Morton]

On Tue, 29 Jun 2010 08:09:52 -0700
Kees Cook <kees.cook@canonical.com> wrote:

> On Tue, Jun 29, 2010 at 11:45:14AM +0300, Alexey Dobriyan wrote:
> > On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook@canonical.com> wrote:
> > > On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote:
> > >> Surely it would be better to fix the tools which display this info
> > >> rather than making the kernel tell fibs.
> > >
> > > The strncpy in get_task_comm() is totally wrong -- it's testing the length
> > > of task->comm.
> > 
> > It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.
> > 
> > > Why should get_task_comm not take a destination buffer length argument?
> > 
> > If you pass too small, you needlessly truncate output.
> 
> If you pass too small a buffer, get_task_comm will happily write all over
> the caller's stack past the end of the buffer if the contents of task->comm
> are large enough:
> 
>         strncpy(buf, tsk->comm, sizeof(tsk->comm));
> 
> The "n" argument to get_task_comm's use of strncpy is totally wrong --
> it needs to be the size of the destination, not the size of the source.
> Luckily, everyone using get_task_comm currently uses buffers that are
> sizeof(task->comm).

It's not "totally wrong" at all.  get_task_comm() *requires* that it be
passed a buffer of at least TASK_COMM_LEN bytes.  sizeof(tsk->comm)
equals TASK_COMM_LEN and always will do so.  We could replace the
sizeof with TASK_COMM_LEN for cosmetic reasons but that's utter
nitpicking.  But then, the comment right there says "buf must be at
least sizeof(tsk->comm) in size".  That's so simple that even a kernel
developer could understand it?

Do we need a runtime check every time to make sure that some developer
didn't misunderstand such a simple thing?  Seems pretty pointless -
there are a zillion such runtime checks we could add.  It'd be better
to do

#define get_task_comm(buf, tsk) {			\
	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
	__get_task_comm(buf, tsk);			\
}

and save the runtime bloat.  But again, what was special about this
particular programmer error?  There are five or six instances of
strcpy(foo, current->comm).  Do we need runtime checks there as well??

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[Kees Cook]

On Tue, Jun 29, 2010 at 11:59:56AM -0700, Andrew Morton wrote:
> On Tue, 29 Jun 2010 08:09:52 -0700
> Kees Cook <kees.cook@canonical.com> wrote:
> 
> > On Tue, Jun 29, 2010 at 11:45:14AM +0300, Alexey Dobriyan wrote:
> > > On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook@canonical.com> wrote:
> > > > On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote:
> > > >> Surely it would be better to fix the tools which display this info
> > > >> rather than making the kernel tell fibs.
> > > >
> > > > The strncpy in get_task_comm() is totally wrong -- it's testing the length
> > > > of task->comm.
> > > 
> > > It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.
> > > 
> > > > Why should get_task_comm not take a destination buffer length argument?
> > > 
> > > If you pass too small, you needlessly truncate output.
> > 
> > If you pass too small a buffer, get_task_comm will happily write all over
> > the caller's stack past the end of the buffer if the contents of task->comm
> > are large enough:
> > 
> >         strncpy(buf, tsk->comm, sizeof(tsk->comm));
> > 
> > The "n" argument to get_task_comm's use of strncpy is totally wrong --
> > it needs to be the size of the destination, not the size of the source.
> > Luckily, everyone using get_task_comm currently uses buffers that are
> > sizeof(task->comm).
> 
> It's not "totally wrong" at all.  get_task_comm() *requires* that it be

Using strncpy with n as the source buffer length is meaningless here
(tsk->comm is always null terminated at TASK_COMM_LEN or earlier).

> passed a buffer of at least TASK_COMM_LEN bytes.  sizeof(tsk->comm)
> equals TASK_COMM_LEN and always will do so.  We could replace the
> sizeof with TASK_COMM_LEN for cosmetic reasons but that's utter
> nitpicking.  But then, the comment right there says "buf must be at
> least sizeof(tsk->comm) in size".  That's so simple that even a kernel
> developer could understand it?

If so, strncpy should just be replaced with strcpy.  You're assuming buf
will always be at least TASK_COMM_LEN.  We know the source buffer size is
TASK_COMM_LEN because it's already defined that way.  There is nothing in
the build or runtime that makes sure that buf is at least TASK_COMM_LEN.

> Do we need a runtime check every time to make sure that some developer
> didn't misunderstand such a simple thing?  Seems pretty pointless -
> there are a zillion such runtime checks we could add.  It'd be better
> to do
> 
> #define get_task_comm(buf, tsk) {			\
> 	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
> 	__get_task_comm(buf, tsk);			\
> }
> 
> and save the runtime bloat.  But again, what was special about this
> particular programmer error?  There are five or six instances of
> strcpy(foo, current->comm).  Do we need runtime checks there as well??

I can't see how it could be a bad thing.  Why not try to do some defensive
programming here?  It's a trivial fix and your define would block this from
ever being a problem.

As I said before, either get_task_comm() is considered sensitive or
it's not.  If it is, I've sent a few patches that might help.  If it's
not, then code should not be criticised for using it.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] sanitize task->comm to avoid leaking escape codes

[john stultz]

On Tue, 2010-06-29 at 07:58 +0300, Artem Bityutskiy wrote:
> On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote:
> > On 06/23, Kees Cook wrote:
> > >
> > > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
> > >  	 */
> > >  	memset(tsk->comm, 0, TASK_COMM_LEN);
> > >  	wmb();
> > 
> > Off-topic. I'd wish I could understand this barrier. Since the lockless
> > reader doesn't do rmb() I don't see how this can help.
> 
> This wmb() looks wrong to me as well. To achieve what the comment in
> this function says, it should be smp_wmb() and we should have smp_rmb()
> in the reading side, AFAIU.
> 
> >  OTOH, I don't
> > understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.
> 
> I think the idea was that readers can see incomplete names, but not
> messed up names, consisting of old and new ones.

Yes, that was the intent, but I do see how it is unnecessary.

So I'm fine with it and the memset being removed.

Thanks for catching this!
-john