[patch 0/4] Syslog permissions, revised

[patch 0/4] Syslog permissions, revised

[Zack Weinberg]

This patchset revises my attempt from last week to allow running klogd
unprivileged without a root shim.  I believe I have addressed all
outstanding objections: in particular, the privilege model enforced by
SELinux is unchanged (you have to have system__syslog_mod to read
/proc/kmsg).  I have also included some nice refactorings (symbolic
constants for sys_syslog opcodes, that sort of thing) and a few
bugfixes (minor and unlikely to affect any live application, but
still).

I hope that this can be considered for 2.6.19; it is low risk in my
opinion and it would be nice to get this functionality into the hands
of the distributors sooner.

zw

[patch 1/4] Add <linux/klog.h>

[Zack Weinberg]

[-- Attachment #1: add_klog_h.diff --]
[-- Type: text/plain, Size: 5846 bytes --]

This patch introduces <linux/klog.h> with symbolic constants for the
various sys_syslog() opcodes, and changes all in-kernel references to
those opcodes to use the constants.  The header is added to the set of
user/kernel interface headers; there is no #ifdef __KERNEL__ block in
it yet, but there will be in subsequent patches, so I put it in as an
'unifdef' header.

zw

Index: linux-2.6/include/linux/Kbuild
===================================================================
--- linux-2.6.orig/include/linux/Kbuild	2006-11-10 13:36:58.000000000 -0800
+++ linux-2.6/include/linux/Kbuild	2006-11-10 13:37:13.000000000 -0800
@@ -253,6 +253,7 @@
 unifdef-y += kernelcapi.h
 unifdef-y += kernel.h
 unifdef-y += keyboard.h
+unifdef-y += klog.h
 unifdef-y += llc.h
 unifdef-y += loop.h
 unifdef-y += lp.h
Index: linux-2.6/include/linux/klog.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/include/linux/klog.h	2006-11-10 14:16:35.000000000 -0800
@@ -0,0 +1,26 @@
+#ifndef _LINUX_KLOG_H
+#define _LINUX_KLOG_H
+
+/*
+ * Constants for the first argument to the syslog() system call
+ * (aka klogctl()).  These numbers are part of the user space ABI!
+ */
+enum {
+	KLOG_CLOSE           =  0, /* close log */
+	KLOG_OPEN            =  1, /* open log */
+	KLOG_READ            =  2, /* read from log (klogd) */
+
+	KLOG_READ_HIST       =  3, /* read history of log messages (dmesg) */
+	KLOG_READ_CLEAR_HIST =  4, /* read and clear history */
+	KLOG_CLEAR_HIST      =  5, /* just clear history */
+
+	KLOG_DISABLE_CONSOLE =  6, /* disable printk to console */
+	KLOG_ENABLE_CONSOLE  =  7, /* enable printk to console */
+	KLOG_SET_CONSOLE_LVL =  8, /* set minimum severity of messages to be
+				    * printed to console */
+
+	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
+	KLOG_GET_SIZE        = 10  /* return size of log buffer */
+};
+
+#endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 13:16:47.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-10 14:16:34.000000000 -0800
@@ -32,6 +32,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/jiffies.h>
+#include <linux/klog.h>
 
 #include <asm/uaccess.h>
 
@@ -165,21 +166,7 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-/*
- * Commands to do_syslog:
- *
- * 	0 -- Close the log.  Currently a NOP.
- * 	1 -- Open the log. Currently a NOP.
- * 	2 -- Read from the log.
- * 	3 -- Read all messages remaining in the ring buffer.
- * 	4 -- Read and clear all messages remaining in the ring buffer
- * 	5 -- Clear ring buffer.
- * 	6 -- Disable printk's to console
- * 	7 -- Enable printk's to console
- *	8 -- Set level of messages printed to console
- *	9 -- Return number of unread characters in the log buffer
- *     10 -- Return size of the log buffer
- */
+/* See linux/klog.h for the command numbers passed as the first argument.  */
 int do_syslog(int type, char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;
@@ -192,11 +179,11 @@
 		return error;
 
 	switch (type) {
-	case 0:		/* Close log */
+	case KLOG_CLOSE:
 		break;
-	case 1:		/* Open log */
+	case KLOG_OPEN:
 		break;
-	case 2:		/* Read from log */
+	case KLOG_READ:
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -227,10 +214,10 @@
 		if (!error)
 			error = i;
 		break;
-	case 4:		/* Read/clear last kernel messages */
+	case KLOG_READ_CLEAR_HIST:
 		do_clear = 1;
 		/* FALL THRU */
-	case 3:		/* Read last kernel messages */
+	case KLOG_READ_HIST:
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -283,16 +270,16 @@
 			}
 		}
 		break;
-	case 5:		/* Clear ring buffer */
+	case KLOG_CLEAR_HIST:
 		logged_chars = 0;
 		break;
-	case 6:		/* Disable logging to console */
+	case KLOG_DISABLE_CONSOLE:
 		console_loglevel = minimum_console_loglevel;
 		break;
-	case 7:		/* Enable logging to console */
+	case KLOG_ENABLE_CONSOLE:
 		console_loglevel = default_console_loglevel;
 		break;
-	case 8:		/* Set level of messages printed to console */
+	case KLOG_SET_CONSOLE_LVL:
 		error = -EINVAL;
 		if (len < 1 || len > 8)
 			goto out;
@@ -301,10 +288,10 @@
 		console_loglevel = len;
 		error = 0;
 		break;
-	case 9:		/* Number of chars in the log buffer */
+	case KLOG_GET_UNREAD:
 		error = log_end - log_start;
 		break;
-	case 10:	/* Size of the log buffer */
+	case KLOG_GET_SIZE:
 		error = log_buf_len;
 		break;
 	default:
Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-10 14:16:41.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-10 14:22:56.000000000 -0800
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/poll.h>
 #include <linux/fs.h>
+#include <linux/klog.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -21,27 +22,28 @@
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(1,NULL,0);
+	return do_syslog(KLOG_OPEN,NULL,0);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(0,NULL,0);
+	(void) do_syslog(KLOG_CLOSE,NULL,0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK) && !do_syslog(9, NULL, 0))
+	if ((file->f_flags & O_NONBLOCK)
+	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
 		return -EAGAIN;
-	return do_syslog(2, buf, count);
+	return do_syslog(KLOG_READ, buf, count);
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &log_wait, wait);
-	if (do_syslog(9, NULL, 0))
+	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
 		return POLLIN | POLLRDNORM;
 	return 0;
 }

--

[patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

[-- Attachment #1: map_perms.diff --]
[-- Type: text/plain, Size: 6250 bytes --]

As suggested by Stephen Smalley: map the various sys_syslog operations
to a smaller set of privilege codes before calling security modules.
This patch changes the security module interface!  There should be no
change in the actual security semantics enforced by dummy, capability,
nor SELinux (with one exception, clearly marked in sys_syslog).

zw


Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-10 13:48:52.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-10 14:08:57.000000000 -0800
@@ -23,4 +23,16 @@
 	KLOG_GET_SIZE        = 10  /* return size of log buffer */
 };
 
+#ifdef __KERNEL__
+
+/*
+ * Constants passed by do_syslog to security_syslog to indicate which
+ * privilege is requested.
+ */
+#define KLOGSEC_READ	  0  /* read current messages (klogd) */
+#define KLOGSEC_READHIST  1  /* read message history (dmesg) */
+#define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
+#define KLOGSEC_CONSOLE   3  /* set console log level */
+
+#endif /* __KERNEL__ */
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 13:44:16.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-10 13:54:53.000000000 -0800
@@ -166,6 +166,12 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
+#define security_syslog_or_fail(type) do {		\
+		int error = security_syslog(type);	\
+		if (error)				\
+			return error;			\
+	} while (0)
+
 /* See linux/klog.h for the command numbers passed as the first argument.  */
 int do_syslog(int type, char __user *buf, int len)
 {
@@ -174,16 +180,15 @@
 	char c;
 	int error = 0;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case KLOG_CLOSE:
+		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_OPEN:
+		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_READ:
+		security_syslog_or_fail(KLOGSEC_READ);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -215,9 +220,11 @@
 			error = i;
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_CLEARHIST);
 		do_clear = 1;
 		/* FALL THRU */
 	case KLOG_READ_HIST:
+		security_syslog_or_fail(KLOGSEC_READHIST);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -271,15 +278,19 @@
 		}
 		break;
 	case KLOG_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_CLEARHIST);
 		logged_chars = 0;
 		break;
 	case KLOG_DISABLE_CONSOLE:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		console_loglevel = minimum_console_loglevel;
 		break;
 	case KLOG_ENABLE_CONSOLE:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		console_loglevel = default_console_loglevel;
 		break;
 	case KLOG_SET_CONSOLE_LVL:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
 			goto out;
@@ -289,9 +300,18 @@
 		error = 0;
 		break;
 	case KLOG_GET_UNREAD:
+		security_syslog_or_fail(KLOGSEC_READ);
 		error = log_end - log_start;
 		break;
 	case KLOG_GET_SIZE:
+		/* This one is allowed if you have _either_
+		   KLOGSEC_READ or KLOGSEC_READHIST.  */
+		error = security_syslog(KLOGSEC_READ);
+		if (error)
+			error = security_syslog(KLOGSEC_READHIST);
+		if (error)
+			break;
+
 		error = log_buf_len;
 		break;
 	default:
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-11-10 13:56:35.000000000 -0800
@@ -23,6 +23,7 @@
 #include <linux/ptrace.h>
 #include <linux/xattr.h>
 #include <linux/hugetlb.h>
+#include <linux/klog.h>
 
 int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
@@ -311,7 +312,7 @@
 
 int cap_syslog (int type)
 {
-	if ((type != 3 && type != 10) && !capable(CAP_SYS_ADMIN))
+	if (type != KLOGSEC_READHIST && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/dummy.c	2006-11-10 13:57:22.000000000 -0800
@@ -28,6 +28,7 @@
 #include <linux/hugetlb.h>
 #include <linux/ptrace.h>
 #include <linux/file.h>
+#include <linux/klog.h>
 
 static int dummy_ptrace (struct task_struct *parent, struct task_struct *child)
 {
@@ -96,11 +97,10 @@
 
 static int dummy_syslog (int type)
 {
-	if ((type != 3 && type != 10) && current->euid)
+	if (type != KLOGSEC_READHIST && current->euid)
 		return -EPERM;
 	return 0;
 }
-
 static int dummy_settime(struct timespec *ts, struct timezone *tz)
 {
 	if (!capable(CAP_SYS_TIME))
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-11-10 14:00:49.000000000 -0800
@@ -71,6 +71,7 @@
 #include <linux/string.h>
 #include <linux/selinux.h>
 #include <linux/mutex.h>
+#include <linux/klog.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -1506,25 +1507,17 @@
 		return rc;
 
 	switch (type) {
-		case 3:         /* Read last kernel messages */
-		case 10:        /* Return size of the log buffer */
-			rc = task_has_system(current, SYSTEM__SYSLOG_READ);
-			break;
-		case 6:         /* Disable logging to console */
-		case 7:         /* Enable logging to console */
-		case 8:		/* Set level of messages printed to console */
-			rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
-			break;
-		case 0:         /* Close log */
-		case 1:         /* Open log */
-		case 2:         /* Read from log */
-		case 4:         /* Read/clear last kernel messages */
-		case 5:         /* Clear ring buffer */
-		default:
-			rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
-			break;
+	case KLOGSEC_READHIST:
+		return task_has_system(current, SYSTEM__SYSLOG_READ);
+
+	case KLOGSEC_CONSOLE:
+		return task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
+
+	case KLOGSEC_READ:
+	case KLOGSEC_CLEARHIST:
+	default:
+		return task_has_system(current, SYSTEM__SYSLOG_MOD);
 	}
-	return rc;
 }
 
 /*

--

[patch 3/4] Refactor do_syslog interface

[Zack Weinberg]

[-- Attachment #1: break_up_do_syslog.diff --]
[-- Type: text/plain, Size: 11066 bytes --]

This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were with one exception: neither
kmsg_close() nor sys_syslog(KLOG_CLOSE, ...) calls security_syslog
at all anymore (close operations should never fail).

Finally, I fixed a couple of minor bugs.  __put_user error handling in
klog_read was slightly off: if __put_user returns an error, that
character should not be consumed from the kernel buffer; if it returns
an error after some characters have already been copied successfully,
the read operation should return the count of already-copied
characters, not the error code.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce that.

zw

Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-10 14:59:38.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-11 21:52:08.000000000 -0800
@@ -9,43 +9,41 @@
 #include <linux/errno.h>
 #include <linux/time.h>
 #include <linux/kernel.h>
-#include <linux/poll.h>
 #include <linux/fs.h>
 #include <linux/klog.h>
+#include <linux/security.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
-extern wait_queue_head_t log_wait;
-
-extern int do_syslog(int type, char __user *bug, int count);
-
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(KLOG_OPEN,NULL,0);
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(KLOG_CLOSE,NULL,0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK)
-	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return -EAGAIN;
-	return do_syslog(KLOG_READ, buf, count);
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	poll_wait(file, &log_wait, wait);
-	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return POLLIN | POLLRDNORM;
-	return 0;
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-10 14:59:46.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-11 21:55:34.000000000 -0800
@@ -17,22 +17,31 @@
 	KLOG_DISABLE_CONSOLE =  6, /* disable printk to console */
 	KLOG_ENABLE_CONSOLE  =  7, /* enable printk to console */
 	KLOG_SET_CONSOLE_LVL =  8, /* set minimum severity of messages to be
-				    * printed to console */
+				      printed to console */
 
 	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
-	KLOG_GET_SIZE        = 10  /* return size of log buffer */
+	KLOG_GET_SIZE        = 10, /* return size of log buffer */
+	KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+				      -- new in 2.6.19 */
 };
 
 #ifdef __KERNEL__
+#include <linux/poll.h>
 
 /*
- * Constants passed by do_syslog to security_syslog to indicate which
- * privilege is requested.
+ * Constants passed to security_syslog to indicate which privilege is
+ * requested.
  */
 #define KLOGSEC_READ	  0  /* read current messages (klogd) */
 #define KLOGSEC_READHIST  1  /* read message history (dmesg) */
 #define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
 #define KLOGSEC_CONSOLE   3  /* set console log level */
 
+/*
+ * Subfunctions of sys_syslog used from fs/proc/kmsg.c.
+ */
+extern int klog_read(char __user *buf, int len, int block);
+extern unsigned int klog_poll(struct file *, poll_table *);
+
 #endif /* __KERNEL__ */
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 14:57:32.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-11 21:54:17.000000000 -0800
@@ -45,7 +45,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int console_printk[4] = {
 	DEFAULT_CONSOLE_LOGLEVEL,	/* console_loglevel */
@@ -166,116 +166,142 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-#define security_syslog_or_fail(type) do {		\
-		int error = security_syslog(type);	\
-		if (error)				\
-			return error;			\
-	} while (0)
+/*
+ * Subfunctions of sys_syslog.  Some are also used from fs/proc/kmsg.c.
+ */
 
-/* See linux/klog.h for the command numbers passed as the first argument.  */
-int do_syslog(int type, char __user *buf, int len)
+int klog_read(char __user *buf, int len, int block)
+{
+	int i, error;
+	char c;
+
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (len == 0)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+	if (!block && log_start - log_end == 0)
+		return -EAGAIN;
+
+	error = wait_event_interruptible(log_wait, (log_start - log_end));
+	if (error)
+		return error;
+
+	i = 0;
+	spin_lock_irq(&logbuf_lock);
+	while (log_start != log_end && i < len) {
+		c = LOG_BUF(log_start);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,buf);
+		if (error)
+			goto out;
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+		log_start++;
+		buf++;
+		i++;
+	}
+	spin_unlock_irq(&logbuf_lock);
+out:
+	if (i > 0)
+		return i;
+	return error;
+}
+
+unsigned int klog_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &log_wait, wait);
+	if (log_end - log_start > 0)
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+static int klog_readhist(char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;
-	int do_clear = 0;
 	char c;
 	int error = 0;
 
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (!len)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+
+	count = len;
+	if (count > log_buf_len)
+		count = log_buf_len;
+	spin_lock_irq(&logbuf_lock);
+	if (count > logged_chars)
+		count = logged_chars;
+	limit = log_end;
+	/*
+	 * __put_user() could sleep, and while we sleep
+	 * printk() could overwrite the messages
+	 * we try to copy to user space. Therefore
+	 * the messages are copied in reverse. <manfreds>
+	 */
+	for (i = 0; i < count && !error; i++) {
+		j = limit-1-i;
+		if (j + log_buf_len < log_end)
+			break;
+		c = LOG_BUF(j);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,&buf[count-1-i]);
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+	}
+	spin_unlock_irq(&logbuf_lock);
+	if (error)
+		return error;
+	error = i;
+	if (i != count) {
+		int offset = count-error;
+		/* buffer overflow during copy, correct user buffer. */
+		for (i = 0; i < error; i++) {
+			if (__get_user(c,&buf[i+offset]) ||
+			    __put_user(c,&buf[i])) {
+				error = -EFAULT;
+				break;
+			}
+			cond_resched();
+		}
+	}
+	return error;
+}
+
+#define security_syslog_or_fail(type) do {	\
+		error = security_syslog(type);	\
+		if (error)			\
+			return error;		\
+	} while (0)
+
+/* See linux/klog.h for the command numbers passed as the first argument.  */
+asmlinkage long sys_syslog(int type, char __user *buf, int len)
+{
+	int error = 0;
 	switch (type) {
 	case KLOG_CLOSE:
-		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_OPEN:
 		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_READ:
+	case KLOG_READ_NONBLOCK:
 		security_syslog_or_fail(KLOGSEC_READ);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		error = wait_event_interruptible(log_wait,
-							(log_start - log_end));
-		if (error)
-			goto out;
-		i = 0;
-		spin_lock_irq(&logbuf_lock);
-		while (!error && (log_start != log_end) && i < len) {
-			c = LOG_BUF(log_start);
-			log_start++;
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,buf);
-			buf++;
-			i++;
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (!error)
-			error = i;
+		error = klog_read(buf, len, type == KLOG_READ);
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_READHIST);
 		security_syslog_or_fail(KLOGSEC_CLEARHIST);
-		do_clear = 1;
-		/* FALL THRU */
+		error = klog_readhist(buf, len);
+		logged_chars = 0;
+		break;
 	case KLOG_READ_HIST:
 		security_syslog_or_fail(KLOGSEC_READHIST);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		count = len;
-		if (count > log_buf_len)
-			count = log_buf_len;
-		spin_lock_irq(&logbuf_lock);
-		if (count > logged_chars)
-			count = logged_chars;
-		if (do_clear)
-			logged_chars = 0;
-		limit = log_end;
-		/*
-		 * __put_user() could sleep, and while we sleep
-		 * printk() could overwrite the messages
-		 * we try to copy to user space. Therefore
-		 * the messages are copied in reverse. <manfreds>
-		 */
-		for (i = 0; i < count && !error; i++) {
-			j = limit-1-i;
-			if (j + log_buf_len < log_end)
-				break;
-			c = LOG_BUF(j);
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,&buf[count-1-i]);
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (error)
-			break;
-		error = i;
-		if (i != count) {
-			int offset = count-error;
-			/* buffer overflow during copy, correct user buffer. */
-			for (i = 0; i < error; i++) {
-				if (__get_user(c,&buf[i+offset]) ||
-				    __put_user(c,&buf[i])) {
-					error = -EFAULT;
-					break;
-				}
-				cond_resched();
-			}
-		}
+		error = klog_readhist(buf, len);
 		break;
 	case KLOG_CLEAR_HIST:
 		security_syslog_or_fail(KLOGSEC_CLEARHIST);
@@ -293,7 +319,7 @@
 		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
-			goto out;
+			break;
 		if (len < minimum_console_loglevel)
 			len = minimum_console_loglevel;
 		console_loglevel = len;
@@ -318,15 +344,9 @@
 		error = -EINVAL;
 		break;
 	}
-out:
 	return error;
 }
 
-asmlinkage long sys_syslog(int type, char __user *buf, int len)
-{
-	return do_syslog(type, buf, len);
-}
-
 /*
  * Call the console drivers on a range of log_buf
  */

--

[patch 4/4] Distinguish /proc/kmsg access from sys_syslog

[Zack Weinberg]

[-- Attachment #1: distinguish_kmsg_security.diff --]
[-- Type: text/plain, Size: 3842 bytes --]

Finally, add a new security class for access to /proc/kmsg, distinct
from the class used for the "read current messages" operations on
sys_syslog.  The dummy and capability modules permit access to
/proc/kmsg to any user (who has somehow acquired an open fd on it);
SELinux is unchanged.  This accomplishes what I was trying to do in
the first place, i.e. enable running klogd unprivileged without a root
shim, in a non-SELinux installation.  (Please remember that the
default DAC permissions for /proc/kmsg restrict it to root, so unless
you chmod it in your installation or modify klogd to open the file and
then drop privs, the effective restrictions are unchanged.)

zw


Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-11 21:57:45.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-11 21:59:01.000000000 -0800
@@ -18,7 +18,7 @@
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return nonseekable_open(inode, file);
@@ -32,7 +32,7 @@
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
@@ -40,7 +40,7 @@
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return klog_poll(file, wait);
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-11 21:57:52.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-11 21:58:40.000000000 -0800
@@ -36,6 +36,8 @@
 #define KLOGSEC_READHIST  1  /* read message history (dmesg) */
 #define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
 #define KLOGSEC_CONSOLE   3  /* set console log level */
+#define KLOGSEC_READ_PROC 4  /* read current messages, but from /proc/kmsg
+				rather than the system call */
 
 /*
  * Subfunctions of sys_syslog used from fs/proc/kmsg.c.
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-11-11 22:02:07.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-11-11 22:02:34.000000000 -0800
@@ -312,7 +312,14 @@
 
 int cap_syslog (int type)
 {
-	if (type != KLOGSEC_READHIST && !capable(CAP_SYS_ADMIN))
+	/*
+	 * Reading history is allowed to any user, and so is reading
+	 * current messages via /proc/kmsg (by default that file is
+	 * only readable by root, but root is allowed to change that,
+	 * or open it and hand the fd to an unprivileged process).
+	 */
+	if (type != KLOGSEC_READHIST && type != KLOGSEC_READ_PROC
+	    && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-11-11 22:02:07.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-11-11 22:04:11.000000000 -0800
@@ -1513,7 +1513,14 @@
 	case KLOGSEC_CONSOLE:
 		return task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
 
+		/*
+		 * N.B. Unlike the default security model, with
+		 * SELinux active you have to have SYSTEM__SYSLOG_MOD
+		 * privilege to read current messages either with the
+		 * system call or from /proc/kmsg.
+		 */
 	case KLOGSEC_READ:
+	case KLOGSEC_READ_PROC:
 	case KLOGSEC_CLEARHIST:
 	default:
 		return task_has_system(current, SYSTEM__SYSLOG_MOD);

--

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Sun, 2006-11-12 at 22:40 -0800, Zack Weinberg wrote:
> plain text document attachment (map_perms.diff)
> As suggested by Stephen Smalley: map the various sys_syslog operations
> to a smaller set of privilege codes before calling security modules.
> This patch changes the security module interface!  There should be no
> change in the actual security semantics enforced by dummy, capability,
> nor SELinux (with one exception, clearly marked in sys_syslog).
> 
> zw
> 
> 
> Index: linux-2.6/include/linux/klog.h
> ===================================================================
> --- linux-2.6.orig/include/linux/klog.h	2006-11-10 13:48:52.000000000 -0800
> +++ linux-2.6/include/linux/klog.h	2006-11-10 14:08:57.000000000 -0800
> @@ -23,4 +23,16 @@
>  	KLOG_GET_SIZE        = 10  /* return size of log buffer */
>  };
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Constants passed by do_syslog to security_syslog to indicate which
> + * privilege is requested.
> + */
> +#define KLOGSEC_READ	  0  /* read current messages (klogd) */
> +#define KLOGSEC_READHIST  1  /* read message history (dmesg) */
> +#define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
> +#define KLOGSEC_CONSOLE   3  /* set console log level */
> +
> +#endif /* __KERNEL__ */

Hi,

you had such a nice userspace/kernel shared header.. and now you mix it
with kernel privates again... can you consider making this a second
header? Eg have a pure userspace clean header and a separate for-kernel
header (which #can include the user header for all I care)... that's
nicer for sure!

Greetings,
   Arjan van de Ven

Re: [patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> you had such a nice userspace/kernel shared header.. and now you mix it
> with kernel privates again... can you consider making this a second
> header?

I thought the point of the "unifdef" thing was that it made a version
of the header with the __KERNEL__ section ripped out, for copying into
/usr/include, so you didn't have to do that ...

zw

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > you had such a nice userspace/kernel shared header.. and now you mix it
> > with kernel privates again... can you consider making this a second
> > header?
> 
> I thought the point of the "unifdef" thing was that it made a version
> of the header with the __KERNEL__ section ripped out, for copying into
> /usr/include, so you didn't have to do that ...

yes it is, however it's mostly for existing stuff/seamless transition.
It's a hack :)
If you can avoid it lets do so; you already have the nice clean header,
so lets not go backwards... you HAVE the clean separation. 

> 
> zw
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > I thought the point of the "unifdef" thing was that it made a version
> > of the header with the __KERNEL__ section ripped out, for copying into
> > /usr/include, so you didn't have to do that ...
>
> yes it is, however it's mostly for existing stuff/seamless transition.
> It's a hack :)
> If you can avoid it lets do so; you already have the nice clean header,
> so lets not go backwards... you HAVE the clean separation.

ok, but I gotta ask that you tell me what to name the internal header,
I can't think of anything that isn't ugly.

zw

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Mon, 2006-11-13 at 09:17 -0800, Zack Weinberg wrote:
> On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > > I thought the point of the "unifdef" thing was that it made a version
> > > of the header with the __KERNEL__ section ripped out, for copying into
> > > /usr/include, so you didn't have to do that ...
> >
> > yes it is, however it's mostly for existing stuff/seamless transition.
> > It's a hack :)
> > If you can avoid it lets do so; you already have the nice clean header,
> > so lets not go backwards... you HAVE the clean separation.
> 
> ok, but I gotta ask that you tell me what to name the internal header,
> I can't think of anything that isn't ugly.

klog.h vs klogd.h ? or klog_api.h for the user one ?

(and yes I suck at names even more than you do ;)

> 
> zw
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [patch 2/4] permission mapping for sys_syslog operations

[Alexey Dobriyan]

On Mon, Nov 13, 2006 at 06:22:04PM +0100, Arjan van de Ven wrote:
> On Mon, 2006-11-13 at 09:17 -0800, Zack Weinberg wrote:
> > On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > > On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > > > I thought the point of the "unifdef" thing was that it made a version
> > > > of the header with the __KERNEL__ section ripped out, for copying into
> > > > /usr/include, so you didn't have to do that ...
> > >
> > > yes it is, however it's mostly for existing stuff/seamless transition.
> > > It's a hack :)
> > > If you can avoid it lets do so; you already have the nice clean header,
> > > so lets not go backwards... you HAVE the clean separation.
> >
> > ok, but I gotta ask that you tell me what to name the internal header,
> > I can't think of anything that isn't ugly.
>
> klog.h vs klogd.h ? or klog_api.h for the user one ?
>
> (and yes I suck at names even more than you do ;)

What about security.h, so SELinux folks won't feel lonely there?
:)

[patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

[-- Attachment #1: map_perms.diff --]
[-- Type: text/plain, Size: 5824 bytes --]

As suggested by Stephen Smalley: map the various sys_syslog operations
to a smaller set of privilege codes before calling security modules.
This patch changes the security module interface!  There should be no
change in the actual security semantics enforced by dummy, capability,
nor SELinux (with one exception, clearly marked in sys_syslog).

Change from previous version of patch: the privilege codes are now
in linux/security.h instead of linux/klog.h, and use the LSM_* naming
convention used for other such constants in that file.

zw


Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-12-13 16:06:22.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-13 16:08:30.000000000 -0800
@@ -164,6 +164,12 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
+#define security_syslog_or_fail(type) do {		\
+		int error = security_syslog(type);	\
+		if (error)				\
+			return error;			\
+	} while (0)
+
 /* See linux/klog.h for the command numbers passed as the first argument.  */
 int do_syslog(int type, char __user *buf, int len)
 {
@@ -172,16 +178,15 @@
 	char c;
 	int error = 0;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case KLOG_CLOSE:
+		security_syslog_or_fail(LSM_KLOG_READ);
 		break;
 	case KLOG_OPEN:
+		security_syslog_or_fail(LSM_KLOG_READ);
 		break;
 	case KLOG_READ:
+		security_syslog_or_fail(LSM_KLOG_READ);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -213,9 +218,11 @@
 			error = i;
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
 		do_clear = 1;
 		/* FALL THRU */
 	case KLOG_READ_HIST:
+		security_syslog_or_fail(LSM_KLOG_READHIST);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -269,15 +276,19 @@
 		}
 		break;
 	case KLOG_CLEAR_HIST:
+		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
 		logged_chars = 0;
 		break;
 	case KLOG_DISABLE_CONSOLE:
+		security_syslog_or_fail(LSM_KLOG_CONSOLE);
 		console_loglevel = minimum_console_loglevel;
 		break;
 	case KLOG_ENABLE_CONSOLE:
+		security_syslog_or_fail(LSM_KLOG_CONSOLE);
 		console_loglevel = default_console_loglevel;
 		break;
 	case KLOG_SET_CONSOLE_LVL:
+		security_syslog_or_fail(LSM_KLOG_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
 			goto out;
@@ -287,9 +298,18 @@
 		error = 0;
 		break;
 	case KLOG_GET_UNREAD:
+		security_syslog_or_fail(LSM_KLOG_READ);
 		error = log_end - log_start;
 		break;
 	case KLOG_GET_SIZE:
+		/* This one is allowed if you have _either_
+		   LSM_KLOG_READ or LSM_KLOG_READHIST.  */
+		error = security_syslog(LSM_KLOG_READ);
+		if (error)
+			error = security_syslog(LSM_KLOG_READHIST);
+		if (error)
+			break;
+
 		error = log_buf_len;
 		break;
 	default:
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-12-13 16:06:22.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-12-13 16:11:13.000000000 -0800
@@ -311,7 +311,7 @@
 
 int cap_syslog (int type)
 {
-	if ((type != 3 && type != 10) && !capable(CAP_SYS_ADMIN))
+	if (type != LSM_KLOG_READHIST && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2006-12-13 16:06:22.000000000 -0800
+++ linux-2.6/security/dummy.c	2006-12-13 16:11:31.000000000 -0800
@@ -96,7 +96,7 @@
 
 static int dummy_syslog (int type)
 {
-	if ((type != 3 && type != 10) && current->euid)
+	if (type != LSM_KLOG_READHIST && current->euid)
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-12-13 16:06:22.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-12-13 16:11:41.000000000 -0800
@@ -1509,25 +1509,17 @@
 		return rc;
 
 	switch (type) {
-		case 3:         /* Read last kernel messages */
-		case 10:        /* Return size of the log buffer */
-			rc = task_has_system(current, SYSTEM__SYSLOG_READ);
-			break;
-		case 6:         /* Disable logging to console */
-		case 7:         /* Enable logging to console */
-		case 8:		/* Set level of messages printed to console */
-			rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
-			break;
-		case 0:         /* Close log */
-		case 1:         /* Open log */
-		case 2:         /* Read from log */
-		case 4:         /* Read/clear last kernel messages */
-		case 5:         /* Clear ring buffer */
-		default:
-			rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
-			break;
+	case LSM_KLOG_READHIST:
+		return task_has_system(current, SYSTEM__SYSLOG_READ);
+
+	case LSM_KLOG_CONSOLE:
+		return task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
+
+	case LSM_KLOG_READ:
+	case LSM_KLOG_CLEARHIST:
+	default:
+		return task_has_system(current, SYSTEM__SYSLOG_MOD);
 	}
-	return rc;
 }
 
 /*
Index: linux-2.6/include/linux/security.h
===================================================================
--- linux-2.6.orig/include/linux/security.h	2006-12-13 16:09:10.000000000 -0800
+++ linux-2.6/include/linux/security.h	2006-12-13 16:10:13.000000000 -0800
@@ -86,6 +86,15 @@
 /* setfsuid or setfsgid, id0 == fsuid or fsgid */
 #define LSM_SETID_FS	8
 
+/*
+ * Values passed to security_syslog to indicate which privilege is
+ * requested.
+ */
+#define LSM_KLOG_READ      0  /* read current messages (klogd) */
+#define LSM_KLOG_READHIST  1  /* read message history (dmesg) */
+#define LSM_KLOG_CLEARHIST 2  /* clear message history (dmesg -c) */
+#define LSM_KLOG_CONSOLE   3  /* set console log level */
+
 /* forward declares to avoid warnings */
 struct nfsctl_arg;
 struct sched_param;

--

Re: [patch 2/4] permission mapping for sys_syslog operations

[Randy Dunlap]

On Thu, 14 Dec 2006 16:16:41 -0800 Zack Weinberg wrote:

> As suggested by Stephen Smalley: map the various sys_syslog operations
> to a smaller set of privilege codes before calling security modules.
> This patch changes the security module interface!  There should be no
> change in the actual security semantics enforced by dummy, capability,
> nor SELinux (with one exception, clearly marked in sys_syslog).
> 
> Change from previous version of patch: the privilege codes are now
> in linux/security.h instead of linux/klog.h, and use the LSM_* naming
> convention used for other such constants in that file.
> 
> 
> Index: linux-2.6/kernel/printk.c
> ===================================================================
> --- linux-2.6.orig/kernel/printk.c	2006-12-13 16:06:22.000000000 -0800
> +++ linux-2.6/kernel/printk.c	2006-12-13 16:08:30.000000000 -0800
> @@ -164,6 +164,12 @@
>  
>  __setup("log_buf_len=", log_buf_len_setup);
>  
> +#define security_syslog_or_fail(type) do {		\
> +		int error = security_syslog(type);	\
> +		if (error)				\
> +			return error;			\
> +	} while (0)
> +

>From Documentation/CodingStyle:

Things to avoid when using macros:

1) macros that affect control flow: ...


>  /* See linux/klog.h for the command numbers passed as the first argument.  */
>  int do_syslog(int type, char __user *buf, int len)
>  {
> @@ -172,16 +178,15 @@
>  	char c;
>  	int error = 0;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case KLOG_CLOSE:
> +		security_syslog_or_fail(LSM_KLOG_READ);
>  		break;
>  	case KLOG_OPEN:
> +		security_syslog_or_fail(LSM_KLOG_READ);
>  		break;
>  	case KLOG_READ:
> +		security_syslog_or_fail(LSM_KLOG_READ);
>  		error = -EINVAL;
>  		if (!buf || len < 0)
>  			goto out;
> @@ -213,9 +218,11 @@
>  			error = i;
>  		break;
>  	case KLOG_READ_CLEAR_HIST:
> +		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
>  		do_clear = 1;
>  		/* FALL THRU */
>  	case KLOG_READ_HIST:
> +		security_syslog_or_fail(LSM_KLOG_READHIST);
>  		error = -EINVAL;
>  		if (!buf || len < 0)
>  			goto out;
> @@ -269,15 +276,19 @@
>  		}
>  		break;
>  	case KLOG_CLEAR_HIST:
> +		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
>  		logged_chars = 0;
>  		break;
>  	case KLOG_DISABLE_CONSOLE:
> +		security_syslog_or_fail(LSM_KLOG_CONSOLE);
>  		console_loglevel = minimum_console_loglevel;
>  		break;
>  	case KLOG_ENABLE_CONSOLE:
> +		security_syslog_or_fail(LSM_KLOG_CONSOLE);
>  		console_loglevel = default_console_loglevel;
>  		break;
>  	case KLOG_SET_CONSOLE_LVL:
> +		security_syslog_or_fail(LSM_KLOG_CONSOLE);
>  		error = -EINVAL;
>  		if (len < 1 || len > 8)
>  			goto out;
> @@ -287,9 +298,18 @@
>  		error = 0;
>  		break;
>  	case KLOG_GET_UNREAD:
> +		security_syslog_or_fail(LSM_KLOG_READ);
>  		error = log_end - log_start;
>  		break;


---
~Randy

Re: [patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

On 12/14/06, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > +#define security_syslog_or_fail(type) do {           \
> > +             int error = security_syslog(type);      \
> > +             if (error)                              \
> > +                     return error;                   \
> > +     } while (0)
> > +
>
> From Documentation/CodingStyle:
>
> Things to avoid when using macros:
>
> 1) macros that affect control flow: ...

It says "avoid", not "never use".  If you can think of another way to
code this function that won't completely obscure the actual operations
with the security checks, I will be happy to change it.

zw

Re: [patch 2/4] permission mapping for sys_syslog operations

[Randy Dunlap]

On Thu, 14 Dec 2006 17:21:25 -0800 Zack Weinberg wrote:

> On 12/14/06, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > > +#define security_syslog_or_fail(type) do {           \
> > > +             int error = security_syslog(type);      \
> > > +             if (error)                              \
> > > +                     return error;                   \
> > > +     } while (0)
> > > +
> >
> > From Documentation/CodingStyle:
> >
> > Things to avoid when using macros:
> >
> > 1) macros that affect control flow: ...
> 
> It says "avoid", not "never use".  If you can think of another way to
> code this function that won't completely obscure the actual operations
> with the security checks, I will be happy to change it.

The usual answer is to just open-code it.  You may dislike that,
but hiding control flow in macros is held as really bad by
more than just me.

---
~Randy

[patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

[-- Attachment #1: map_perms.diff --]
[-- Type: text/plain, Size: 6597 bytes --]

As suggested by Stephen Smalley: map the various sys_syslog operations
to a smaller set of privilege codes before calling security modules.
This patch changes the security module interface!  There should be no
change in the actual security semantics enforced by dummy, capability,
nor SELinux (with one exception, clearly marked in sys_syslog).

The privilege codes are now in linux/security.h instead of
linux/klog.h, and use the LSM_* naming convention used for other such
constants in that file.

I'm now using a static table to map operations to privilege codes.
This makes it very easy to see the mapping as a whole, and prevents
security operations from cluttering up the body of do_syslog, but does
necessitate some checks to ensure that when new KLOG_* operations are
added, people don't forget to add entries to the table.  If people are
not fans of the idiom I used, I will see about getting a feature added
to gcc to make it less ugly.

Note that after this patch, close() on /proc/kmsg and
sys_syslog(KLOG_CLOSE) do not do security checks; they always succeed.
(IMO close should never fail.) This was always in this patch series,
but used to be in a different stage.

zw

Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-12-24 11:43:14.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-24 11:43:16.000000000 -0800
@@ -164,6 +164,39 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
+/*
+ * This table maps KLOG_* operation codes to LSM_KLOG_* privilege classes.
+ * "unsigned char" is used just to keep it small.
+ */
+
+static const unsigned char klog_privclass[] = {
+	[KLOG_CLOSE]           = 0,  /* close always succeeds */
+	[KLOG_OPEN]            = LSM_KLOG_READ,
+	[KLOG_READ]            = LSM_KLOG_READ,
+	[KLOG_GET_UNREAD]      = LSM_KLOG_READ,
+
+	[KLOG_READ_HIST]       = LSM_KLOG_READHIST,
+	[KLOG_GET_SIZE]        = LSM_KLOG_READHIST,
+	[KLOG_READ_CLEAR_HIST] = LSM_KLOG_READHIST|LSM_KLOG_CLEARHIST,
+	[KLOG_CLEAR_HIST]      = LSM_KLOG_CLEARHIST,
+
+	[KLOG_DISABLE_CONSOLE] = LSM_KLOG_CONSOLE,
+	[KLOG_ENABLE_CONSOLE]  = LSM_KLOG_CONSOLE,
+	[KLOG_SET_CONSOLE_LVL] = LSM_KLOG_CONSOLE,
+};
+
+/* It is essential that there be an entry in the above table for every
+ * KLOG_* code.  The following, er, odd declarations cause compilation
+ * of this file to fail if that is not true.  They do not correspond
+ * to real data objects.
+ */
+extern char klog_privclass_is_missing_an_entry[
+	ARRAY_SIZE(klog_privclass) < KLOG_OPCODE_MAX ? -1 : 1
+];
+extern char klog_privclass_has_too_many_entries[
+	ARRAY_SIZE(klog_privclass) > KLOG_OPCODE_MAX ? -1 : 1
+];
+
 /**
  * do_syslog - operate on the log of kernel messages
  * @type: operation to perform
@@ -185,9 +218,11 @@
 	unsigned long i, j, limit, count;
 	int do_clear = 0;
 	char c;
-	int error = 0;
+	int error;
 
-	error = security_syslog(type);
+	if (type < 0 || type >= KLOG_OPCODE_MAX)
+		return -EINVAL;
+	error = security_syslog(klog_privclass[type]);
 	if (error)
 		return error;
 
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-12-23 08:55:16.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-12-24 11:43:16.000000000 -0800
@@ -311,7 +311,7 @@
 
 int cap_syslog (int type)
 {
-	if ((type != 3 && type != 10) && !capable(CAP_SYS_ADMIN))
+	if ((type & ~LSM_KLOG_READHIST) && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2006-12-23 08:55:16.000000000 -0800
+++ linux-2.6/security/dummy.c	2006-12-24 11:43:16.000000000 -0800
@@ -96,7 +96,7 @@
 
 static int dummy_syslog (int type)
 {
-	if ((type != 3 && type != 10) && current->euid)
+	if ((type & ~LSM_KLOG_READHIST) && current->euid)
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-12-23 08:55:16.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-12-24 11:43:16.000000000 -0800
@@ -1504,29 +1504,27 @@
 {
 	int rc;
 
+	/* if there are any codes we don't know about, there's a bug */
+	BUG_ON(type & ~(LSM_KLOG_READ|LSM_KLOG_READHIST
+			|LSM_KLOG_CLEARHIST|LSM_KLOG_CONSOLE));
+
 	rc = secondary_ops->syslog(type);
 	if (rc)
 		return rc;
 
-	switch (type) {
-		case 3:         /* Read last kernel messages */
-		case 10:        /* Return size of the log buffer */
-			rc = task_has_system(current, SYSTEM__SYSLOG_READ);
-			break;
-		case 6:         /* Disable logging to console */
-		case 7:         /* Enable logging to console */
-		case 8:		/* Set level of messages printed to console */
-			rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
-			break;
-		case 0:         /* Close log */
-		case 1:         /* Open log */
-		case 2:         /* Read from log */
-		case 4:         /* Read/clear last kernel messages */
-		case 5:         /* Clear ring buffer */
-		default:
-			rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
-			break;
-	}
+	if (type & (LSM_KLOG_READ|LSM_KLOG_CLEARHIST))
+		rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
+	if (rc)
+		return rc;
+
+	if (type & LSM_KLOG_READHIST)
+		rc = task_has_system(current, SYSTEM__SYSLOG_READ);
+	if (rc)
+		return rc;
+
+	if (type & LSM_KLOG_CONSOLE)
+		rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
+
 	return rc;
 }
 
Index: linux-2.6/include/linux/security.h
===================================================================
--- linux-2.6.orig/include/linux/security.h	2006-12-23 08:55:16.000000000 -0800
+++ linux-2.6/include/linux/security.h	2006-12-24 11:43:16.000000000 -0800
@@ -86,6 +86,18 @@
 /* setfsuid or setfsgid, id0 == fsuid or fsgid */
 #define LSM_SETID_FS	8
 
+/*
+ * Values passed to security_syslog to indicate which privilege class
+ * is requested.  requested.  Note these are bitmasks - callers may
+ * request more than one of the privilege classes at once (this is
+ * currently only used for READHIST|CLEARHIST) or no privilege at all
+ * (which should always succeed).
+ */
+#define LSM_KLOG_READ      1  /* read current messages (klogd) */
+#define LSM_KLOG_READHIST  2  /* read message history (dmesg) */
+#define LSM_KLOG_CLEARHIST 4  /* clear message history (dmesg -c) */
+#define LSM_KLOG_CONSOLE   8  /* set or query console log level */
+
 /* forward declares to avoid warnings */
 struct nfsctl_arg;
 struct sched_param;

--