linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] sanitize task->comm to avoid leaking escape codes
@ 2010-06-24 19:05 Kees Cook
  2010-06-24 23:56 ` KOSAKI Motohiro
  2010-06-29  9:36 ` Alan Cox
  0 siblings, 2 replies; 13+ messages in thread
From: Kees Cook @ 2010-06-24 19:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Greg Kroah-Hartman, Andrew Morton, Tejun Heo, Veaceslav Falico,
	Alexander Viro, Oleg Nesterov, KOSAKI Motohiro, Neil Horman,
	Roland McGrath, Ingo Molnar, Peter Zijlstra, Hidetoshi Seto,
	Stefani Seibold, Thomas Gleixner, Eric Paris, James Morris,
	Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn, Steve Grubb,
	Christoph Hellwig, linux-fsdevel

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).

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
v2:
 - don't use a helper #define, just fix the arguments and callers
 - add missing ctype.h include that got lost during testing.
---
 drivers/char/tty_audit.c |    2 +-
 fs/exec.c                |   18 ++++++++++++++----
 fs/proc/array.c          |    4 ++--
 include/linux/sched.h    |    2 +-
 kernel/auditsc.c         |    2 +-
 kernel/capability.c      |    4 ++--
 kernel/fork.c            |    2 +-
 kernel/sys.c             |    2 +-
 8 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
index 1b8ee59..b061fe8 100644
--- a/drivers/char/tty_audit.c
+++ b/drivers/char/tty_audit.c
@@ -75,7 +75,7 @@ static void tty_audit_log(const char *description, struct task_struct *tsk,
 				 "major=%d minor=%d comm=", description,
 				 tsk->pid, uid, loginuid, sessionid,
 				 major, minor);
-		get_task_comm(name, tsk);
+		get_task_comm(name, sizeof(name), tsk);
 		audit_log_untrustedstring(ab, name);
 		audit_log_format(ab, " data=");
 		audit_log_n_hex(ab, data, size);
diff --git a/fs/exec.c b/fs/exec.c
index e19de6a..e0e42b7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -55,6 +55,7 @@
 #include <linux/fsnotify.h>
 #include <linux/fs_struct.h>
 #include <linux/pipe_fs_i.h>
+#include <linux/ctype.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -934,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(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);
 
 	/*
@@ -955,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/fs/proc/array.c b/fs/proc/array.c
index 9b58d38..cdf55c9 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -93,7 +93,7 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
 	char *name;
 	char tcomm[sizeof(p->comm)];
 
-	get_task_comm(tcomm, p);
+	get_task_comm(tcomm, sizeof(tcomm), p);
 
 	seq_printf(m, "Name:\t");
 	end = m->buf + m->size;
@@ -393,7 +393,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		}
 	}
 
-	get_task_comm(tcomm, task);
+	get_task_comm(tcomm, sizeof(tcomm), task);
 
 	sigemptyset(&sigign);
 	sigemptyset(&sigcatch);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f118809..dbd538b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2117,7 +2117,7 @@ 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);
+extern char *get_task_comm(char *to, size_t len, struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
 extern unsigned long wait_task_inactive(struct task_struct *, long match_state);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3828ad5..ab8a134 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -956,7 +956,7 @@ static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk
 
 	/* tsk == current */
 
-	get_task_comm(name, tsk);
+	get_task_comm(name, sizeof(name), tsk);
 	audit_log_format(ab, " comm=");
 	audit_log_untrustedstring(ab, name);
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 2f05303..022fc03 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -51,7 +51,7 @@ static void warn_legacy_capability_use(void)
 
 		printk(KERN_INFO "warning: `%s' uses 32-bit capabilities"
 		       " (legacy support in use)\n",
-		       get_task_comm(name, current));
+		       get_task_comm(name, sizeof(name), current));
 		warned = 1;
 	}
 }
@@ -81,7 +81,7 @@ static void warn_deprecated_v2(void)
 
 		printk(KERN_INFO "warning: `%s' uses deprecated v2"
 		       " capabilities in a way that may be insecure.\n",
-		       get_task_comm(name, current));
+		       get_task_comm(name, sizeof(name), current));
 		warned = 1;
 	}
 }
diff --git a/kernel/fork.c b/kernel/fork.c
index b6cce14..8c23470 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1403,7 +1403,7 @@ long do_fork(unsigned long clone_flags,
 			count--;
 			printk(KERN_INFO "fork(): process `%s' used deprecated "
 					"clone flags 0x%lx\n",
-				get_task_comm(comm, current),
+				get_task_comm(comm, sizeof(comm), current),
 				clone_flags & CLONE_STOPPED);
 		}
 	}
diff --git a/kernel/sys.c b/kernel/sys.c
index e83ddbb..b1a215b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1535,7 +1535,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			set_task_comm(me, comm);
 			return 0;
 		case PR_GET_NAME:
-			get_task_comm(comm, me);
+			get_task_comm(comm, sizeof(comm), me);
 			if (copy_to_user((char __user *)arg2, comm,
 					 sizeof(comm)))
 				return -EFAULT;
-- 
1.7.1


-- 
Kees Cook
Ubuntu Security Team

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-24 19:05 [PATCH v2] sanitize task->comm to avoid leaking escape codes Kees Cook
@ 2010-06-24 23:56 ` KOSAKI Motohiro
  2010-06-28 17:48   ` Stefani Seibold
  2010-06-29  9:36 ` Alan Cox
  1 sibling, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-06-24 23:56 UTC (permalink / raw)
  To: Kees Cook
  Cc: kosaki.motohiro, linux-kernel, Greg Kroah-Hartman, Andrew Morton,
	Tejun Heo, Veaceslav Falico, Alexander Viro, Oleg Nesterov,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Stefani Seibold, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Steve Grubb, Christoph Hellwig, linux-fsdevel

> 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).
> 
> Signed-off-by: Kees Cook <kees.cook@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.




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-24 23:56 ` KOSAKI Motohiro
@ 2010-06-28 17:48   ` Stefani Seibold
  2010-06-28 18:04     ` Kees Cook
  2010-06-29  3:05     ` KOSAKI Motohiro
  0 siblings, 2 replies; 13+ messages in thread
From: Stefani Seibold @ 2010-06-28 17:48 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Kees Cook, linux-kernel, Greg Kroah-Hartman, Andrew Morton,
	Tejun Heo, Veaceslav Falico, Alexander Viro, Oleg Nesterov,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Thomas Gleixner, Eric Paris, James Morris,
	Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn, Steve Grubb,
	Christoph Hellwig, linux-fsdevel

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).
> > 
> > Signed-off-by: Kees Cook <kees.cook@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.

Stefani



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-28 17:48   ` Stefani Seibold
@ 2010-06-28 18:04     ` Kees Cook
  2010-06-29  3:05     ` KOSAKI Motohiro
  1 sibling, 0 replies; 13+ messages in thread
From: Kees Cook @ 2010-06-28 18:04 UTC (permalink / raw)
  To: Stefani Seibold
  Cc: KOSAKI Motohiro, linux-kernel, Greg Kroah-Hartman, Andrew Morton,
	Tejun Heo, Veaceslav Falico, Alexander Viro, Oleg Nesterov,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Thomas Gleixner, Eric Paris, James Morris,
	Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn, Steve Grubb,
	Christoph Hellwig, linux-fsdevel

On Mon, Jun 28, 2010 at 07:48:38PM +0200, Stefani Seibold 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).
> > > 
> > > Signed-off-by: Kees Cook <kees.cook@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.

I think sanitizing inputs is more effective than sanitizing outputs.
If ->comm is safe internally, then we don't have to filter it going out
on printk, audit, /proc output, etc.  There is a limited number of places
where a process has control over an arbitrary string in kernel structures,
so the places where they are set should be fixed instead of fixing every
possible usage of it on output.

I wouldn't mind sanitizing printk also, but it's tangential to sanitizing
task->comm when it is set.

-Kees

-- 
Kees Cook
Ubuntu Security Team

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-28 17:48   ` Stefani Seibold
  2010-06-28 18:04     ` Kees Cook
@ 2010-06-29  3:05     ` KOSAKI Motohiro
  2010-06-29 12:58       ` Steve Grubb
  1 sibling, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-06-29  3:05 UTC (permalink / raw)
  To: Stefani Seibold
  Cc: kosaki.motohiro, Kees Cook, linux-kernel, Greg Kroah-Hartman,
	Andrew Morton, Tejun Heo, Veaceslav Falico, Alexander Viro,
	Oleg Nesterov, Neil Horman, Roland McGrath, Ingo Molnar,
	Peter Zijlstra, Hidetoshi Seto, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Steve Grubb, Christoph Hellwig, linux-fsdevel

> 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).
> > > 
> > > Signed-off-by: Kees Cook <kees.cook@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.




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-24 19:05 [PATCH v2] sanitize task->comm to avoid leaking escape codes Kees Cook
  2010-06-24 23:56 ` KOSAKI Motohiro
@ 2010-06-29  9:36 ` Alan Cox
  2010-06-29 14:51   ` Kees Cook
  2010-06-30  0:31   ` KOSAKI Motohiro
  1 sibling, 2 replies; 13+ messages in thread
From: Alan Cox @ 2010-06-29  9:36 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Greg Kroah-Hartman, Andrew Morton, Tejun Heo,
	Veaceslav Falico, Alexander Viro, Oleg Nesterov, KOSAKI Motohiro,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Stefani Seibold, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Steve Grubb, Christoph Hellwig, linux-fsdevel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-29  3:05     ` KOSAKI Motohiro
@ 2010-06-29 12:58       ` Steve Grubb
  2010-06-30  0:16         ` KOSAKI Motohiro
  0 siblings, 1 reply; 13+ messages in thread
From: Steve Grubb @ 2010-06-29 12:58 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Stefani Seibold, Kees Cook, linux-kernel, Greg Kroah-Hartman,
	Andrew Morton, Tejun Heo, Veaceslav Falico, Alexander Viro,
	Oleg Nesterov, Neil Horman, Roland McGrath, Ingo Molnar,
	Peter Zijlstra, Hidetoshi Seto, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Christoph Hellwig, linux-fsdevel

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@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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-29  9:36 ` Alan Cox
@ 2010-06-29 14:51   ` Kees Cook
  2010-06-30  9:13     ` Alan Cox
  2010-06-30  0:31   ` KOSAKI Motohiro
  1 sibling, 1 reply; 13+ messages in thread
From: Kees Cook @ 2010-06-29 14:51 UTC (permalink / raw)
  To: Alan Cox
  Cc: linux-kernel, Greg Kroah-Hartman, Andrew Morton, Tejun Heo,
	Veaceslav Falico, Alexander Viro, Oleg Nesterov, KOSAKI Motohiro,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Stefani Seibold, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Steve Grubb, Christoph Hellwig, linux-fsdevel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-29 12:58       ` Steve Grubb
@ 2010-06-30  0:16         ` KOSAKI Motohiro
  2010-06-30  0:22           ` Steve Grubb
  0 siblings, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-06-30  0:16 UTC (permalink / raw)
  To: Steve Grubb
  Cc: kosaki.motohiro, Stefani Seibold, Kees Cook, linux-kernel,
	Greg Kroah-Hartman, Andrew Morton, Tejun Heo, Veaceslav Falico,
	Alexander Viro, Oleg Nesterov, Neil Horman, Roland McGrath,
	Ingo Molnar, Peter Zijlstra, Hidetoshi Seto, Thomas Gleixner,
	Eric Paris, James Morris, Andrew G. Morgan, Dhaval Giani,
	Serge E. Hallyn, Christoph Hellwig, linux-fsdevel

> 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.

Probably this mail is offtopic. I think audit is unrelated with this discusstion. because when 
forensic, admins shouldn't believe task->comm at all. because 1) no path information, 
perhaps "ls" might mean "/home/attackers-dir/evil-script/ls" 2) easily obscured by 
prctl(PR_SET_NAME).

That said, audit have to logged following two point if task name is necessary.
1) exec
2) prctl(PRT_SET_NAME)

Thought ?




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-30  0:16         ` KOSAKI Motohiro
@ 2010-06-30  0:22           ` Steve Grubb
  2010-06-30  0:28             ` KOSAKI Motohiro
  0 siblings, 1 reply; 13+ messages in thread
From: Steve Grubb @ 2010-06-30  0:22 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Stefani Seibold, Kees Cook, linux-kernel, Greg Kroah-Hartman,
	Andrew Morton, Tejun Heo, Veaceslav Falico, Alexander Viro,
	Oleg Nesterov, Neil Horman, Roland McGrath, Ingo Molnar,
	Peter Zijlstra, Hidetoshi Seto, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Christoph Hellwig, linux-fsdevel

On Tuesday, June 29, 2010 08:16:08 pm KOSAKI Motohiro wrote:
> > 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.
> 
> Probably this mail is offtopic. I think audit is unrelated with this
> discusstion. because when  forensic, admins shouldn't believe task->comm
> at all. because 1) no path information, perhaps "ls" might mean
> "/home/attackers-dir/evil-script/ls" 2) easily obscured by
> prctl(PR_SET_NAME).

No, its on-topic and we want that information unchanged.


> That said, audit have to logged following two point if task name is
> necessary. 1) exec
> 2) prctl(PRT_SET_NAME)
> 
> Thought ?

The audit system is capable of grabbing that information, too.

-Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-30  0:22           ` Steve Grubb
@ 2010-06-30  0:28             ` KOSAKI Motohiro
  0 siblings, 0 replies; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-06-30  0:28 UTC (permalink / raw)
  To: Steve Grubb
  Cc: kosaki.motohiro, Stefani Seibold, Kees Cook, linux-kernel,
	Greg Kroah-Hartman, Andrew Morton, Tejun Heo, Veaceslav Falico,
	Alexander Viro, Oleg Nesterov, Neil Horman, Roland McGrath,
	Ingo Molnar, Peter Zijlstra, Hidetoshi Seto, Thomas Gleixner,
	Eric Paris, James Morris, Andrew G. Morgan, Dhaval Giani,
	Serge E. Hallyn, Christoph Hellwig, linux-fsdevel

> On Tuesday, June 29, 2010 08:16:08 pm KOSAKI Motohiro wrote:
> > > 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.
> > 
> > Probably this mail is offtopic. I think audit is unrelated with this
> > discusstion. because when  forensic, admins shouldn't believe task->comm
> > at all. because 1) no path information, perhaps "ls" might mean
> > "/home/attackers-dir/evil-script/ls" 2) easily obscured by
> > prctl(PR_SET_NAME).
> 
> No, its on-topic and we want that information unchanged.

Why?
I think I've described why admins should't see task->comm during forensic. Do you
disagree this? or Do you have another viewpoint?

Can you help us clarify your point?

> > That said, audit have to logged following two point if task name is
> > necessary. 1) exec
> > 2) prctl(PRT_SET_NAME)
> > 
> > Thought ?
> 
> The audit system is capable of grabbing that information, too.

ok. thanks good information :)



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-29  9:36 ` Alan Cox
  2010-06-29 14:51   ` Kees Cook
@ 2010-06-30  0:31   ` KOSAKI Motohiro
  1 sibling, 0 replies; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-06-30  0:31 UTC (permalink / raw)
  To: Alan Cox
  Cc: kosaki.motohiro, Kees Cook, linux-kernel, Greg Kroah-Hartman,
	Andrew Morton, Tejun Heo, Veaceslav Falico, Alexander Viro,
	Oleg Nesterov, Neil Horman, Roland McGrath, Ingo Molnar,
	Peter Zijlstra, Hidetoshi Seto, Stefani Seibold, Thomas Gleixner,
	Eric Paris, James Morris, Andrew G. Morgan, Dhaval Giani,
	Serge E. Hallyn, Steve Grubb, Christoph Hellwig, linux-fsdevel

> > 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.

Ah, I recall one use case. In past, some IBM folks talked about they
want to map Java thread name to prctl(PR_SET_NAME). In such case, 
utf-8 name is very common.

So, I agree with you.

Thanks.




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] sanitize task->comm to avoid leaking escape codes
  2010-06-29 14:51   ` Kees Cook
@ 2010-06-30  9:13     ` Alan Cox
  0 siblings, 0 replies; 13+ messages in thread
From: Alan Cox @ 2010-06-30  9:13 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Greg Kroah-Hartman, Andrew Morton, Tejun Heo,
	Veaceslav Falico, Alexander Viro, Oleg Nesterov, KOSAKI Motohiro,
	Neil Horman, Roland McGrath, Ingo Molnar, Peter Zijlstra,
	Hidetoshi Seto, Stefani Seibold, Thomas Gleixner, Eric Paris,
	James Morris, Andrew G. Morgan, Dhaval Giani, Serge E. Hallyn,
	Steve Grubb, Christoph Hellwig, linux-fsdevel

> 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).

Lots of things are theoretically problematic and kernel would take a week
to boot if we covered them all 8)

Having a 
	struct task_name {
		char [propersize];
	}

would produce the same code as far as I can tell and so typechecking
though - so as you say it can be done sanely.

Alan

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2010-06-30  9:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-24 19:05 [PATCH v2] sanitize task->comm to avoid leaking escape codes Kees Cook
2010-06-24 23:56 ` KOSAKI Motohiro
2010-06-28 17:48   ` Stefani Seibold
2010-06-28 18:04     ` Kees Cook
2010-06-29  3:05     ` KOSAKI Motohiro
2010-06-29 12:58       ` Steve Grubb
2010-06-30  0:16         ` KOSAKI Motohiro
2010-06-30  0:22           ` Steve Grubb
2010-06-30  0:28             ` KOSAKI Motohiro
2010-06-29  9:36 ` Alan Cox
2010-06-29 14:51   ` Kees Cook
2010-06-30  9:13     ` Alan Cox
2010-06-30  0:31   ` KOSAKI Motohiro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).