From: Alexey Dobriyan on
On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook(a)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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on
> 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.

If an administrator has a vulnerable terminal emulator they have other
problems.

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

This is a regression for tools that correctly handle unmutilated data.

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

The kernel "isprint" isn't adequate for this. comm is set by the shell
based on argv[0] usually which means that in normal situations it is a
UTF-8 string.

Please do any filtering you must in the yama security module where it
only affects that. One way to approach it without losing data within the
module might be to use HTML style encoding within Yama so your own tools
can undo the 'sanitizing' rather than losing information ?

Ideally you want to the dev/inode pair of the thing being executed
printed as well - that will give real information for security purposes,
while the ->comm data is much more convenient for general debugging and
investigation than having to keep looking them up.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steve Grubb on
On Monday, June 28, 2010 11:05:56 pm KOSAKI Motohiro wrote:
> > Am Freitag, den 25.06.2010, 08:56 +0900 schrieb KOSAKI Motohiro:
> > > > 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
> > > > is more obvious when misused by callers (presently nothing was
> > > > incorrectly calling get_task_comm's unsafe use of strncpy).

For the audit system, we want the real, unsanitized task->comm. We record it
in a special format to the audit logs such that unprintable characters are
included. We want it exactly this way for certification purposes as well as
forensic evidence if someone was playing games. If you do sanitize it for
other areas of the kernel, please give us a way to get the unsanitized text.

-Steve


> > > > Signed-off-by: Kees Cook <kees.cook(a)canonical.com>
> > >
> > > I've reviewed this patch briefly, Here is my personal concern...
> > >
> > > On Linux, non-printable leaking is fundamental, only fixing task->comm
> > > doesn't solve syslog exploit issue. Probably all /proc/kmsg user should
> > > have escaping non-pritables code.
> > >
> > > However, task->comm is one of most easy injection data of kernel,
> > > because we have prctl(PR_SET_NAME), attacker don't need root
> > > privilege. So, conservative assumption seems guard from crappy fault.
> > > Plus, this patch is very small and our small TASK_COMM_LEN lead that
> > > we don't need big performance concern.
> > >
> > > So, I don't find demerit in this proposal. but I'm not security
> > > specialist, it's only personal thinking.
> >
> > Agree. I think a escaped printk should be a more generic solution.
>
> Is this possible? printk() doesn't know userland locale. how do it escape
> correctly? When we only concern task->comm, assuming ascii-only string is
> enough practical. but printk generic logic should allow non-ascii, I
> think.
>
> I think userland reader process only know correct escaping way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on
On Tue, Jun 29, 2010 at 10:36:50AM +0100, Alan Cox wrote:
> > 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.
>
> If an administrator has a vulnerable terminal emulator they have other
> problems.

Totally agreed.

> Please do any filtering you must in the yama security module where it
> only affects that. One way to approach it without losing data within the
> module might be to use HTML style encoding within Yama so your own tools
> can undo the 'sanitizing' rather than losing information ?

I'm not interested in sanitizing this in Yama. The use of task->comm via
printk was seen as a flaw. I didn't agree (see above about terminal),
and suggested that if it was a flaw, it was a flaw with printk or
task->comm itself. Since "fixing" both of those have been vetoed,
I have no more interest in the filtering.

What I do have interest in is fixing get_task_comm's use of buffers, which
is theoretically problematic in some future where someone accidentally
calls it with a buffer smaller than sizeof(task->comm).

I'll send a patch that only fixes that and leaves out the filtering.

-Kees

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/