[PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

To fix /dev/kmsg, let's compare the existing interfaces and what they allow:

- /proc/kmsg allows:
 - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
   single-reader interface (SYSLOG_ACTION_READ).
 - everything, after an open.

- syslog syscall allows:
 - anything, if CAP_SYSLOG.
 - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
 - nothing else (EPERM).

The use-cases were:
- dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
- sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
  destructive SYSLOG_ACTION_READs.

AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
clear the ring buffer.

Based on the comments in devkmsg_llseek, it sounds like actions besides
reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
so we have a strict subset of the non-destructive syslog syscall actions.

To this end, move the check as Josh had done, but also rename the constants
to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
after a capabilities-constrained SYSLOG_ACTION_OPEN check.

- /dev/kmsg allows:
 - open if CAP_SYSLOG or dmesg_restrict==0
 - reading/polling, after open

Signed-off-by: Kees Cook <keescook@chromium.org>
Reported-by: Christian Kujau <lists@nerdbynature.de>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: stable@vger.kernel.org
---
 fs/proc/kmsg.c         |   10 +++---
 include/linux/syslog.h |    4 +--
 kernel/printk.c        |   91 ++++++++++++++++++++++++++----------------------
 3 files changed, 57 insertions(+), 48 deletions(-)

diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
index bd4b5a7..bdfabda 100644
--- a/fs/proc/kmsg.c
+++ b/fs/proc/kmsg.c
@@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
+	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
+	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
 	return 0;
 }
 
@@ -34,15 +34,15 @@ 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(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+	    !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
 		return -EAGAIN;
-	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
+	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &log_wait, wait);
-	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
 		return POLLIN | POLLRDNORM;
 	return 0;
 }
diff --git a/include/linux/syslog.h b/include/linux/syslog.h
index 3891139..98a3153 100644
--- a/include/linux/syslog.h
+++ b/include/linux/syslog.h
@@ -44,8 +44,8 @@
 /* Return size of the log buffer */
 #define SYSLOG_ACTION_SIZE_BUFFER   10
 
-#define SYSLOG_FROM_CALL 0
-#define SYSLOG_FROM_FILE 1
+#define SYSLOG_FROM_READER           0
+#define SYSLOG_FROM_PROC             1
 
 int do_syslog(int type, char __user *buf, int count, bool from_file);
 
diff --git a/kernel/printk.c b/kernel/printk.c
index abbdd9e..53b5c5e 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -368,6 +368,53 @@ static void log_store(int facility, int level,
 	log_next_seq++;
 }
 
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+int dmesg_restrict = 1;
+#else
+int dmesg_restrict;
+#endif
+
+static int syslog_action_restricted(int type)
+{
+	if (dmesg_restrict)
+		return 1;
+	/*
+	 * Unless restricted, we allow "read all" and "get buffer size"
+	 * for everybody.
+	 */
+	return type != SYSLOG_ACTION_READ_ALL &&
+	       type != SYSLOG_ACTION_SIZE_BUFFER;
+}
+
+static int check_syslog_permissions(int type, bool from_file)
+{
+	/*
+	 * If this is from /proc/kmsg and we've already opened it, then we've
+	 * already done the capabilities checks at open time.
+	 */
+	if (from_file && type != SYSLOG_ACTION_OPEN)
+		return 0;
+
+	if (syslog_action_restricted(type)) {
+		if (capable(CAP_SYSLOG))
+			return 0;
+		/*
+		 * For historical reasons, accept CAP_SYS_ADMIN too, with
+		 * a warning.
+		 */
+		if (capable(CAP_SYS_ADMIN)) {
+			printk_once(KERN_WARNING "%s (%d): "
+				 "Attempt to access syslog with CAP_SYS_ADMIN "
+				 "but no CAP_SYSLOG (deprecated).\n",
+				 current->comm, task_pid_nr(current));
+			return 0;
+		}
+		return -EPERM;
+	}
+	return security_syslog(type);
+}
+
+
 /* /dev/kmsg - userspace message inject/listen interface */
 struct devkmsg_user {
 	u64 seq;
@@ -624,7 +671,8 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
 
-	err = security_syslog(SYSLOG_ACTION_READ_ALL);
+	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
+				       SYSLOG_FROM_READER);
 	if (err)
 		return err;
 
@@ -817,45 +865,6 @@ static inline void boot_delay_msec(int level)
 }
 #endif
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
-
-static int syslog_action_restricted(int type)
-{
-	if (dmesg_restrict)
-		return 1;
-	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
-	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
-}
-
-static int check_syslog_permissions(int type, bool from_file)
-{
-	/*
-	 * If this is from /proc/kmsg and we've already opened it, then we've
-	 * already done the capabilities checks at open time.
-	 */
-	if (from_file && type != SYSLOG_ACTION_OPEN)
-		return 0;
-
-	if (syslog_action_restricted(type)) {
-		if (capable(CAP_SYSLOG))
-			return 0;
-		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
-		if (capable(CAP_SYS_ADMIN)) {
-			printk_once(KERN_WARNING "%s (%d): "
-				 "Attempt to access syslog with CAP_SYS_ADMIN "
-				 "but no CAP_SYSLOG (deprecated).\n",
-				 current->comm, task_pid_nr(current));
-			return 0;
-		}
-		return -EPERM;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_PRINTK_TIME)
 static bool printk_time = 1;
 #else
@@ -1253,7 +1262,7 @@ out:
 
 SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
 {
-	return do_syslog(type, buf, len, SYSLOG_FROM_CALL);
+	return do_syslog(type, buf, len, SYSLOG_FROM_READER);
 }
 
 /*
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
> 
> - /proc/kmsg allows:
>  - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>    single-reader interface (SYSLOG_ACTION_READ).
>  - everything, after an open.
> 
> - syslog syscall allows:
>  - anything, if CAP_SYSLOG.
>  - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
>  - nothing else (EPERM).
> 
> The use-cases were:
> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
>   destructive SYSLOG_ACTION_READs.
> 
> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
> clear the ring buffer.
> 
> Based on the comments in devkmsg_llseek, it sounds like actions besides
> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
> so we have a strict subset of the non-destructive syslog syscall actions.
> 
> To this end, move the check as Josh had done, but also rename the constants
> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
> 
> - /dev/kmsg allows:
>  - open if CAP_SYSLOG or dmesg_restrict==0
>  - reading/polling, after open
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> Cc: Josh Boyer <jwboyer@redhat.com>
> Cc: Kay Sievers <kay@vrfy.org>
> Cc: stable@vger.kernel.org

So staring at this for a while, I think it looks correct.  It's
basically the same thing as the v3 I sent out, with the constant rename
and no check in devkmsg_read, right?

I'll try and get it tested here locally tomorrow.

josh

> ---
>  fs/proc/kmsg.c         |   10 +++---
>  include/linux/syslog.h |    4 +--
>  kernel/printk.c        |   91 ++++++++++++++++++++++++++----------------------
>  3 files changed, 57 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
> index bd4b5a7..bdfabda 100644
> --- a/fs/proc/kmsg.c
> +++ b/fs/proc/kmsg.c
> @@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
>  
>  static int kmsg_open(struct inode * inode, struct file * file)
>  {
> -	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
> +	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
>  }
>  
>  static int kmsg_release(struct inode * inode, struct file * file)
>  {
> -	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
> +	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
>  	return 0;
>  }
>  
> @@ -34,15 +34,15 @@ 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(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> +	    !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
>  		return -EAGAIN;
> -	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
> +	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
>  }
>  
>  static unsigned int kmsg_poll(struct file *file, poll_table *wait)
>  {
>  	poll_wait(file, &log_wait, wait);
> -	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> +	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
>  		return POLLIN | POLLRDNORM;
>  	return 0;
>  }
> diff --git a/include/linux/syslog.h b/include/linux/syslog.h
> index 3891139..98a3153 100644
> --- a/include/linux/syslog.h
> +++ b/include/linux/syslog.h
> @@ -44,8 +44,8 @@
>  /* Return size of the log buffer */
>  #define SYSLOG_ACTION_SIZE_BUFFER   10
>  
> -#define SYSLOG_FROM_CALL 0
> -#define SYSLOG_FROM_FILE 1
> +#define SYSLOG_FROM_READER           0
> +#define SYSLOG_FROM_PROC             1
>  
>  int do_syslog(int type, char __user *buf, int count, bool from_file);
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..53b5c5e 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,53 @@ static void log_store(int facility, int level,
>  	log_next_seq++;
>  }
>  
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +int dmesg_restrict = 1;
> +#else
> +int dmesg_restrict;
> +#endif
> +
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/*
> +	 * Unless restricted, we allow "read all" and "get buffer size"
> +	 * for everybody.
> +	 */
> +	return type != SYSLOG_ACTION_READ_ALL &&
> +	       type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * If this is from /proc/kmsg and we've already opened it, then we've
> +	 * already done the capabilities checks at open time.
> +	 */
> +	if (from_file && type != SYSLOG_ACTION_OPEN)
> +		return 0;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			return 0;
> +		/*
> +		 * For historical reasons, accept CAP_SYS_ADMIN too, with
> +		 * a warning.
> +		 */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			printk_once(KERN_WARNING "%s (%d): "
> +				 "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n",
> +				 current->comm, task_pid_nr(current));
> +			return 0;
> +		}
> +		return -EPERM;
> +	}
> +	return security_syslog(type);
> +}
> +
> +
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
>  	u64 seq;
> @@ -624,7 +671,8 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +				       SYSLOG_FROM_READER);
>  	if (err)
>  		return err;
>  
> @@ -817,45 +865,6 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> -
> -static int syslog_action_restricted(int type)
> -{
> -	if (dmesg_restrict)
> -		return 1;
> -	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> -	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> -}
> -
> -static int check_syslog_permissions(int type, bool from_file)
> -{
> -	/*
> -	 * If this is from /proc/kmsg and we've already opened it, then we've
> -	 * already done the capabilities checks at open time.
> -	 */
> -	if (from_file && type != SYSLOG_ACTION_OPEN)
> -		return 0;
> -
> -	if (syslog_action_restricted(type)) {
> -		if (capable(CAP_SYSLOG))
> -			return 0;
> -		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> -		if (capable(CAP_SYS_ADMIN)) {
> -			printk_once(KERN_WARNING "%s (%d): "
> -				 "Attempt to access syslog with CAP_SYS_ADMIN "
> -				 "but no CAP_SYSLOG (deprecated).\n",
> -				 current->comm, task_pid_nr(current));
> -			return 0;
> -		}
> -		return -EPERM;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_PRINTK_TIME)
>  static bool printk_time = 1;
>  #else
> @@ -1253,7 +1262,7 @@ out:
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
>  {
> -	return do_syslog(type, buf, len, SYSLOG_FROM_CALL);
> +	return do_syslog(type, buf, len, SYSLOG_FROM_READER);
>  }
>  
>  /*
> -- 
> 1.7.9.5
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Tue, Apr 30, 2013 at 11:35 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
>> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
>>
>> - /proc/kmsg allows:
>>  - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>>    single-reader interface (SYSLOG_ACTION_READ).
>>  - everything, after an open.
>>
>> - syslog syscall allows:
>>  - anything, if CAP_SYSLOG.
>>  - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
>>  - nothing else (EPERM).
>>
>> The use-cases were:
>> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
>> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
>>   destructive SYSLOG_ACTION_READs.
>>
>> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
>> clear the ring buffer.
>>
>> Based on the comments in devkmsg_llseek, it sounds like actions besides
>> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
>> so we have a strict subset of the non-destructive syslog syscall actions.
>>
>> To this end, move the check as Josh had done, but also rename the constants
>> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
>> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
>> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
>> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
>>
>> - /dev/kmsg allows:
>>  - open if CAP_SYSLOG or dmesg_restrict==0
>>  - reading/polling, after open
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Reported-by: Christian Kujau <lists@nerdbynature.de>
>> Cc: Josh Boyer <jwboyer@redhat.com>
>> Cc: Kay Sievers <kay@vrfy.org>
>> Cc: stable@vger.kernel.org
>
> So staring at this for a while, I think it looks correct.  It's
> basically the same thing as the v3 I sent out, with the constant rename
> and no check in devkmsg_read, right?

Yeah, I just explicitly clarified the use-cases and reasoning. :)

> I'll try and get it tested here locally tomorrow.

Great; thanks!

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
> 
> - /proc/kmsg allows:
>  - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>    single-reader interface (SYSLOG_ACTION_READ).
>  - everything, after an open.
> 
> - syslog syscall allows:
>  - anything, if CAP_SYSLOG.
>  - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
>  - nothing else (EPERM).
> 
> The use-cases were:
> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
>   destructive SYSLOG_ACTION_READs.
> 
> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
> clear the ring buffer.
> 
> Based on the comments in devkmsg_llseek, it sounds like actions besides
> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
> so we have a strict subset of the non-destructive syslog syscall actions.
> 
> To this end, move the check as Josh had done, but also rename the constants
> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
> 
> - /dev/kmsg allows:
>  - open if CAP_SYSLOG or dmesg_restrict==0
>  - reading/polling, after open
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> Cc: Josh Boyer <jwboyer@redhat.com>
> Cc: Kay Sievers <kay@vrfy.org>
> Cc: stable@vger.kernel.org

OK, this seems to work as expected.  Non-privileged users can open
/dev/kmsg for reading and read dmesg if dmesg_restrict is not set.  If
set, dmesg(1) will try both /dev/kmsg and the syslog call and get EPERM
back.  Running as root still works.

Tested-by: Josh Boyer <jwboyer@redhat.com>

> ---
>  fs/proc/kmsg.c         |   10 +++---
>  include/linux/syslog.h |    4 +--
>  kernel/printk.c        |   91 ++++++++++++++++++++++++++----------------------
>  3 files changed, 57 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
> index bd4b5a7..bdfabda 100644
> --- a/fs/proc/kmsg.c
> +++ b/fs/proc/kmsg.c
> @@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
>  
>  static int kmsg_open(struct inode * inode, struct file * file)
>  {
> -	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
> +	return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
>  }
>  
>  static int kmsg_release(struct inode * inode, struct file * file)
>  {
> -	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
> +	(void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
>  	return 0;
>  }
>  
> @@ -34,15 +34,15 @@ 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(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> +	    !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
>  		return -EAGAIN;
> -	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
> +	return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
>  }
>  
>  static unsigned int kmsg_poll(struct file *file, poll_table *wait)
>  {
>  	poll_wait(file, &log_wait, wait);
> -	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> +	if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
>  		return POLLIN | POLLRDNORM;
>  	return 0;
>  }
> diff --git a/include/linux/syslog.h b/include/linux/syslog.h
> index 3891139..98a3153 100644
> --- a/include/linux/syslog.h
> +++ b/include/linux/syslog.h
> @@ -44,8 +44,8 @@
>  /* Return size of the log buffer */
>  #define SYSLOG_ACTION_SIZE_BUFFER   10
>  
> -#define SYSLOG_FROM_CALL 0
> -#define SYSLOG_FROM_FILE 1
> +#define SYSLOG_FROM_READER           0
> +#define SYSLOG_FROM_PROC             1
>  
>  int do_syslog(int type, char __user *buf, int count, bool from_file);
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..53b5c5e 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,53 @@ static void log_store(int facility, int level,
>  	log_next_seq++;
>  }
>  
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +int dmesg_restrict = 1;
> +#else
> +int dmesg_restrict;
> +#endif
> +
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/*
> +	 * Unless restricted, we allow "read all" and "get buffer size"
> +	 * for everybody.
> +	 */
> +	return type != SYSLOG_ACTION_READ_ALL &&
> +	       type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * If this is from /proc/kmsg and we've already opened it, then we've
> +	 * already done the capabilities checks at open time.
> +	 */
> +	if (from_file && type != SYSLOG_ACTION_OPEN)
> +		return 0;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			return 0;
> +		/*
> +		 * For historical reasons, accept CAP_SYS_ADMIN too, with
> +		 * a warning.
> +		 */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			printk_once(KERN_WARNING "%s (%d): "
> +				 "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n",
> +				 current->comm, task_pid_nr(current));
> +			return 0;
> +		}
> +		return -EPERM;
> +	}
> +	return security_syslog(type);
> +}
> +
> +
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
>  	u64 seq;
> @@ -624,7 +671,8 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +				       SYSLOG_FROM_READER);
>  	if (err)
>  		return err;
>  
> @@ -817,45 +865,6 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> -
> -static int syslog_action_restricted(int type)
> -{
> -	if (dmesg_restrict)
> -		return 1;
> -	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> -	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> -}
> -
> -static int check_syslog_permissions(int type, bool from_file)
> -{
> -	/*
> -	 * If this is from /proc/kmsg and we've already opened it, then we've
> -	 * already done the capabilities checks at open time.
> -	 */
> -	if (from_file && type != SYSLOG_ACTION_OPEN)
> -		return 0;
> -
> -	if (syslog_action_restricted(type)) {
> -		if (capable(CAP_SYSLOG))
> -			return 0;
> -		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> -		if (capable(CAP_SYS_ADMIN)) {
> -			printk_once(KERN_WARNING "%s (%d): "
> -				 "Attempt to access syslog with CAP_SYS_ADMIN "
> -				 "but no CAP_SYSLOG (deprecated).\n",
> -				 current->comm, task_pid_nr(current));
> -			return 0;
> -		}
> -		return -EPERM;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_PRINTK_TIME)
>  static bool printk_time = 1;
>  #else
> @@ -1253,7 +1262,7 @@ out:
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
>  {
> -	return do_syslog(type, buf, len, SYSLOG_FROM_CALL);
> +	return do_syslog(type, buf, len, SYSLOG_FROM_READER);
>  }
>  
>  /*
> -- 
> 1.7.9.5
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Andrew Morton]

On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook <keescook@chromium.org> wrote:

> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
> 
> - /proc/kmsg allows:
>  - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>    single-reader interface (SYSLOG_ACTION_READ).
>  - everything, after an open.
> 
> - syslog syscall allows:
>  - anything, if CAP_SYSLOG.
>  - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
>  - nothing else (EPERM).
> 
> The use-cases were:
> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
>   destructive SYSLOG_ACTION_READs.
> 
> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
> clear the ring buffer.
> 
> Based on the comments in devkmsg_llseek, it sounds like actions besides
> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
> so we have a strict subset of the non-destructive syslog syscall actions.
> 
> To this end, move the check as Josh had done, but also rename the constants
> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
> 
> - /dev/kmsg allows:
>  - open if CAP_SYSLOG or dmesg_restrict==0
>  - reading/polling, after open

hm, that changelog is waaaay down in the weeds and anyone who hasn't
been following this with a microscope won't have a clue.

I went into an earlier patch and dug out this:

: The dmesg_restrict sysctl currently covers the syslog method for access
: dmesg, however /dev/kmsg isn't covered by the same protections.  Most
: people haven't noticed because util-linux dmesg(1) defaults to using the
: syslog method for access in older versions.  With util-linux dmesg(1)
: defaults to reading directly from /dev/kmsg.
: 
: Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192

Which is still accurate and relevant, yes?

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, May 8, 2013 at 2:22 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
>>
>> - /proc/kmsg allows:
>>  - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>>    single-reader interface (SYSLOG_ACTION_READ).
>>  - everything, after an open.
>>
>> - syslog syscall allows:
>>  - anything, if CAP_SYSLOG.
>>  - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
>>  - nothing else (EPERM).
>>
>> The use-cases were:
>> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
>> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
>>   destructive SYSLOG_ACTION_READs.
>>
>> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
>> clear the ring buffer.
>>
>> Based on the comments in devkmsg_llseek, it sounds like actions besides
>> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
>> so we have a strict subset of the non-destructive syslog syscall actions.
>>
>> To this end, move the check as Josh had done, but also rename the constants
>> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
>> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
>> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
>> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
>>
>> - /dev/kmsg allows:
>>  - open if CAP_SYSLOG or dmesg_restrict==0
>>  - reading/polling, after open
>
> hm, that changelog is waaaay down in the weeds and anyone who hasn't
> been following this with a microscope won't have a clue.
>
> I went into an earlier patch and dug out this:
>
> : The dmesg_restrict sysctl currently covers the syslog method for access
> : dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> : people haven't noticed because util-linux dmesg(1) defaults to using the
> : syslog method for access in older versions.  With util-linux dmesg(1)
> : defaults to reading directly from /dev/kmsg.
> :
> : Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192
>
> Which is still accurate and relevant, yes?

Yeah, that'll be fine. I kind of like having the longer rationale in
the commit message for future reference (i.e. destructive vs
non-destructive, etc), but I'd rather see the code fixed. :)

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg

[Steven Rostedt]

On Wed, 2013-05-08 at 14:26 -0700, Kees Cook wrote:

> Yeah, that'll be fine. I kind of like having the longer rationale in
> the commit message for future reference (i.e. destructive vs
> non-destructive, etc), but I'd rather see the code fixed. :)

There's no reason not to have both, is there?

-- Steve

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Mon, Apr 08, 2013 at 02:34:38PM -0700, Kees Cook wrote:
> On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> > ----- Original Message -----
> >> From: "Kees Cook" <keescook@chromium.org>
> >> To: "Josh Boyer" <jwboyer@redhat.com>
> >> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
> >> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
> >> "LKML" <linux-kernel@vger.kernel.org>
> >> Sent: Monday, April 1, 2013 7:51:57 PM
> >> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> >>
> >> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> >> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >> >>
> >> >> poke.  Nothing got applied.  I'll drop
> >> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> >> effect ;)
> >> >
> >> > Oh dear.
> >> >
> >> > Eric, were you going to cleanup your suggestion and send it out?
> >>
> >> Ping? What state is this in?
> >
> > If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
> > original patch, but I'd rather get this fixed everywhere.
> 
> Pinging again here. Any news on this?

I am now almost as delinquent as Eric.  I owe you one beer of your
choice.  You can add one beer to that tally every day until I send out a
cleaned up patch.  Maybe that will motivate me to send one.

More seriously, I was on PTO at the end of last week and this fell off
my plate.  I will clean it up and test it tomorrow, with the aim of
actually sending something tomorrow afternoon.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> ----- Original Message -----
>> From: "Kees Cook" <keescook@chromium.org>
>> To: "Josh Boyer" <jwboyer@redhat.com>
>> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
>> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
>> "LKML" <linux-kernel@vger.kernel.org>
>> Sent: Monday, April 1, 2013 7:51:57 PM
>> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>>
>> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>> >>
>> >> poke.  Nothing got applied.  I'll drop
>> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> >> effect ;)
>> >
>> > Oh dear.
>> >
>> > Eric, were you going to cleanup your suggestion and send it out?
>>
>> Ping? What state is this in?
>
> If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
> original patch, but I'd rather get this fixed everywhere.

Pinging again here. Any news on this?

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

----- Original Message -----
> From: "Kees Cook" <keescook@chromium.org>
> To: "Josh Boyer" <jwboyer@redhat.com>
> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
> "LKML" <linux-kernel@vger.kernel.org>
> Sent: Monday, April 1, 2013 7:51:57 PM
> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> 
> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >>
> >> poke.  Nothing got applied.  I'll drop
> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> effect ;)
> >
> > Oh dear.
> >
> > Eric, were you going to cleanup your suggestion and send it out?
> 
> Ping? What state is this in?

If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
original patch, but I'd rather get this fixed everywhere.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>>
>> poke.  Nothing got applied.  I'll drop
>> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> effect ;)
>
> Oh dear.
>
> Eric, were you going to cleanup your suggestion and send it out?

Ping? What state is this in?

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> 
> poke.  Nothing got applied.  I'll drop
> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> effect ;) 

Oh dear.

Eric, were you going to cleanup your suggestion and send it out?

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Andrew Morton]

poke.  Nothing got applied.  I'll drop
kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
effect ;) 


From: Josh Boyer <jwboyer@redhat.com>
Subject: kmsg: honor dmesg_restrict sysctl on /dev/kmsg

Originally, the addition of dmesg_restrict covered both the syslog
method of accessing dmesg, as well as /dev/kmsg itself.  This was done
indirectly by security_syslog calling cap_syslog before doing any LSM
checks.

However, commit 12b3052c3ee ("capabilities/syslog: open code cap_syslog
logic to fix build failure") moved the code around and pushed the checks
into the caller itself.  That seems to have inadvertently dropped the
checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
because util-linux dmesg(1) defaults to using the syslog method for access
in older versions.  With util-linux 2.22 and a kernel newer than 3.5,
dmesg(1) defaults to reading directly from /dev/kmsg.

Fix this by making an explicit check in the devkmsg_open function.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
Reported-by: Christian Kujau <lists@nerdbynature.de>
Cc: Eric Paris <eparis@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/printk.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg kernel/printk.c
--- a/kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg
+++ a/kernel/printk.c
@@ -620,6 +620,9 @@ static int devkmsg_open(struct inode *in
 	struct devkmsg_user *user;
 	int err;
 
+	if (dmesg_restrict && !capable(CAP_SYSLOG))
+		return -EACCES;
+
 	/* write-only does not need any file context */
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
_

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Feb 27, 2013 at 2:19 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
>> Fine Fine, I'll get off my lazy butt and look at this.
>
> Shock!
>
>> Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
>> and the syscall both use do_syslog() which calls
>> check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
>> security_syslog(), which we all agree needs fixed.
>>
>> > > Also, the LSM hooks aren't doing any capability checks at all that I can
>> > > see, which may or may not be a bug in and of itself but I have no idea.
>> > > I was hoping Eric would speak up about that.
>>
>> I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
>> to have security_* sometimes the right thing to do and sometimes
>> capable() is the right thing to do.  It is pervasive in the kernel that
>> you have either/or, but I can't think of anywhere that functions are
>> expected to do BOTH.  So yeah, that needs fixed.
>
> OK.
>
>>
>> > Eric explicitly removed the cap check since it was cluttering things
>> > the way it was originally written. I do think security_syslog() should
>> > pass through check_syslog_permissions(), though. Then this wouldn't
>> > have happened. That might actually be the right way to clean this up,
>> > but I'd like to see Eric's thoughts first.
>>
>> How about something like this?
>
> I think this looks pretty good.  Much clearer overall and the
> consolidation is nice.  I'll try to get it tested soon.
>
> josh
>
>>
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 7c69b3e..ced2cac 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>>       if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>>               return 0;
>>
>> -     err = security_syslog(SYSLOG_ACTION_READ_ALL);
>> +     err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);

Yes, this looks correct with the consolidation below. Nice!

>>       if (err)
>>               return err;
>>
>> @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool from_file)
>>        * already done the capabilities checks at open time.
>>        */
>>       if (from_file && type != SYSLOG_ACTION_OPEN)
>> -             return 0;
>> +             goto ok;
>>
>>       if (syslog_action_restricted(type)) {
>>               if (capable(CAP_SYSLOG))
>> -                     return 0;
>> +                     goto ok;
>>               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>>               if (capable(CAP_SYS_ADMIN)) {
>>                       printk_once(KERN_WARNING "%s (%d): "
>>                                "Attempt to access syslog with CAP_SYS_ADMIN "
>>                                "but no CAP_SYSLOG (deprecated).\n",
>>                                current->comm, task_pid_nr(current));
>> -                     return 0;
>> +                     goto ok;
>>               }
>>               return -EPERM;
>>       }
>> -     return 0;
>> +ok:
>> +     return security_syslog(type);
>>  }
>>
>>  #if defined(CONFIG_PRINTK_TIME)
>> @@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>>       if (error)
>>               goto out;
>>
>> -     error = security_syslog(type);
>> -     if (error)
>> -             return error;
>> -
>>       switch (type) {
>>       case SYSLOG_ACTION_CLOSE:       /* Close log */
>>               break;
>>
>>

I think for completeness, we need to add a
check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE)
call to devkmsg_read().

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
> Fine Fine, I'll get off my lazy butt and look at this.

Shock!

> Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
> and the syscall both use do_syslog() which calls
> check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
> security_syslog(), which we all agree needs fixed.
> 
> > > Also, the LSM hooks aren't doing any capability checks at all that I can
> > > see, which may or may not be a bug in and of itself but I have no idea.
> > > I was hoping Eric would speak up about that.
> 
> I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
> to have security_* sometimes the right thing to do and sometimes
> capable() is the right thing to do.  It is pervasive in the kernel that
> you have either/or, but I can't think of anywhere that functions are
> expected to do BOTH.  So yeah, that needs fixed.

OK.

> 
> > Eric explicitly removed the cap check since it was cluttering things
> > the way it was originally written. I do think security_syslog() should
> > pass through check_syslog_permissions(), though. Then this wouldn't
> > have happened. That might actually be the right way to clean this up,
> > but I'd like to see Eric's thoughts first.
> 
> How about something like this?

I think this looks pretty good.  Much clearer overall and the
consolidation is nice.  I'll try to get it tested soon.

josh

> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 7c69b3e..ced2cac 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
>  	if (err)
>  		return err;
>  
> @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool from_file)
>  	 * already done the capabilities checks at open time.
>  	 */
>  	if (from_file && type != SYSLOG_ACTION_OPEN)
> -		return 0;
> +		goto ok;
>  
>  	if (syslog_action_restricted(type)) {
>  		if (capable(CAP_SYSLOG))
> -			return 0;
> +			goto ok;
>  		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>  		if (capable(CAP_SYS_ADMIN)) {
>  			printk_once(KERN_WARNING "%s (%d): "
>  				 "Attempt to access syslog with CAP_SYS_ADMIN "
>  				 "but no CAP_SYSLOG (deprecated).\n",
>  				 current->comm, task_pid_nr(current));
> -			return 0;
> +			goto ok;
>  		}
>  		return -EPERM;
>  	}
> -	return 0;
> +ok:
> +	return security_syslog(type);
>  }
>  
>  #if defined(CONFIG_PRINTK_TIME)
> @@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	if (error)
>  		goto out;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case SYSLOG_ACTION_CLOSE:	/* Close log */
>  		break;
> 
> 

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Eric Paris]

Fine Fine, I'll get off my lazy butt and look at this.

On Wed, 2013-02-27 at 10:14 -0800, Kees Cook wrote:
> On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> > On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
> >> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> >> > Originally, the addition of dmesg_restrict covered both the syslog
> >> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> >> > indirectly by security_syslog calling cap_syslog before doing any LSM
> >> > checks.
> >> >
> >> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> >> > logic to fix build failure) moved the code around and pushed the checks
> >> > into the caller itself.  That seems to have inadvertently dropped the
> >> > checks for dmesg_restrict on /dev/kmsg.

Not sure this is correct.  There was no devkmsg_open() in commit
12b3052c3ee.  That was added in e11fea92e.  Looks like before that
commit the devkmsg code was even worse.  It didn't call security_syslog
or capable().  Uggh.

>   Most people haven't noticed
> >> > because util-linux dmesg(1) defaults to using the syslog method for
> >> > access in older versions.  With util-linux 2.22 and a kernel newer than
> >> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> >> >
> >> > Fix this by making an explicit check in the devkmsg_open function.
> >> >
> >> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> >> >
> >> > Reported-by: Christian Kujau <lists@nerdbynature.de>
> >> > CC: stable@vger.kernel.org
> >> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> >> > ---
> >> >  kernel/printk.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/kernel/printk.c b/kernel/printk.c
> >> > index f24633a..398ef9a 100644
> >> > --- a/kernel/printk.c
> >> > +++ b/kernel/printk.c
> >> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> >> >     struct devkmsg_user *user;
> >> >     int err;
> >> >
> >> > +   if (dmesg_restrict && !capable(CAP_SYSLOG))
> >> > +           return -EACCES;
> >> > +
> >>
> >> I think this should use check_syslog_permissions() instead, as done for
> >> /proc/kmsg and the syslog syscall.
> >>
> >>       err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
> >
> > Did you mean SYSLOG_ACTION_OPEN?
> 
> Oops, yes, typo.
> 
> > I didn't code it that way because the comment in that function about the
> > capability checks already being done seem pretty off to me.  I could
> > have just misread the /proc code though.  I can resend with the change
> > you suggest if everyone thinks that's a better way.
> 
> Yeah, the comment is meaningful if you examine how /proc/kmsg works,
> which was basically as a wrapper to the syslog syscall. The issue was
> that we had to catch (and potentially block) the "open" action on
> /proc/kmsg vs blocking the the syslog read action. In this case, we've
> got another file-based interface, so we should use OPEN and FROM_FILE
> to block the open. (Though it could be argued that we only want to
> block the open if it's reading, which is exactly what Kay was trying
> to do originally it looks like.)

Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
and the syscall both use do_syslog() which calls
check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
security_syslog(), which we all agree needs fixed.

> > Also, the LSM hooks aren't doing any capability checks at all that I can
> > see, which may or may not be a bug in and of itself but I have no idea.
> > I was hoping Eric would speak up about that.

I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
to have security_* sometimes the right thing to do and sometimes
capable() is the right thing to do.  It is pervasive in the kernel that
you have either/or, but I can't think of anywhere that functions are
expected to do BOTH.  So yeah, that needs fixed.

> Eric explicitly removed the cap check since it was cluttering things
> the way it was originally written. I do think security_syslog() should
> pass through check_syslog_permissions(), though. Then this wouldn't
> have happened. That might actually be the right way to clean this up,
> but I'd like to see Eric's thoughts first.

How about something like this?

diff --git a/kernel/printk.c b/kernel/printk.c
index 7c69b3e..ced2cac 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
 
-	err = security_syslog(SYSLOG_ACTION_READ_ALL);
+	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
 	if (err)
 		return err;
 
@@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool from_file)
 	 * already done the capabilities checks at open time.
 	 */
 	if (from_file && type != SYSLOG_ACTION_OPEN)
-		return 0;
+		goto ok;
 
 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
-			return 0;
+			goto ok;
 		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
 		if (capable(CAP_SYS_ADMIN)) {
 			printk_once(KERN_WARNING "%s (%d): "
 				 "Attempt to access syslog with CAP_SYS_ADMIN "
 				 "but no CAP_SYSLOG (deprecated).\n",
 				 current->comm, task_pid_nr(current));
-			return 0;
+			goto ok;
 		}
 		return -EPERM;
 	}
-	return 0;
+ok:
+	return security_syslog(type);
 }
 
 #if defined(CONFIG_PRINTK_TIME)
@@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	if (error)
 		goto out;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case SYSLOG_ACTION_CLOSE:	/* Close log */
 		break;

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
>> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
>> > Originally, the addition of dmesg_restrict covered both the syslog
>> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
>> > indirectly by security_syslog calling cap_syslog before doing any LSM
>> > checks.
>> >
>> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
>> > logic to fix build failure) moved the code around and pushed the checks
>> > into the caller itself.  That seems to have inadvertently dropped the
>> > checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
>> > because util-linux dmesg(1) defaults to using the syslog method for
>> > access in older versions.  With util-linux 2.22 and a kernel newer than
>> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
>> >
>> > Fix this by making an explicit check in the devkmsg_open function.
>> >
>> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>> >
>> > Reported-by: Christian Kujau <lists@nerdbynature.de>
>> > CC: stable@vger.kernel.org
>> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>> > ---
>> >  kernel/printk.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/kernel/printk.c b/kernel/printk.c
>> > index f24633a..398ef9a 100644
>> > --- a/kernel/printk.c
>> > +++ b/kernel/printk.c
>> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>> >     struct devkmsg_user *user;
>> >     int err;
>> >
>> > +   if (dmesg_restrict && !capable(CAP_SYSLOG))
>> > +           return -EACCES;
>> > +
>>
>> I think this should use check_syslog_permissions() instead, as done for
>> /proc/kmsg and the syslog syscall.
>>
>>       err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
>
> Did you mean SYSLOG_ACTION_OPEN?

Oops, yes, typo.

> I didn't code it that way because the comment in that function about the
> capability checks already being done seem pretty off to me.  I could
> have just misread the /proc code though.  I can resend with the change
> you suggest if everyone thinks that's a better way.

Yeah, the comment is meaningful if you examine how /proc/kmsg works,
which was basically as a wrapper to the syslog syscall. The issue was
that we had to catch (and potentially block) the "open" action on
/proc/kmsg vs blocking the the syslog read action. In this case, we've
got another file-based interface, so we should use OPEN and FROM_FILE
to block the open. (Though it could be argued that we only want to
block the open if it's reading, which is exactly what Kay was trying
to do originally it looks like.)

> Also, the LSM hooks aren't doing any capability checks at all that I can
> see, which may or may not be a bug in and of itself but I have no idea.
> I was hoping Eric would speak up about that.

Eric explicitly removed the cap check since it was cluttering things
the way it was originally written. I do think security_syslog() should
pass through check_syslog_permissions(), though. Then this wouldn't
have happened. That might actually be the right way to clean this up,
but I'd like to see Eric's thoughts first.

>>       if (err)
>>               return err;
>>
>> And going forward we should probably think about dropping the CAP_SYS_ADMIN
>> backward-compat code in check_syslog_permissions.
>
> Sure, but that's a separate commit.

Yup.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 10:05:47AM -0800, Kees Cook wrote:
> Hi,
> 
> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> > Originally, the addition of dmesg_restrict covered both the syslog
> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> > indirectly by security_syslog calling cap_syslog before doing any LSM
> > checks.
> 
> Actually, are the security_syslog() checks in /dev/kmsg correct? There is
> only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't
> it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to
> devkmsg_read? (And should we add one for write?)

You ask wonderful questions!  I have no idea.  Either way, it needs to
still somehow check for capable(CAP_SYSLOG) which security_syslog
doesn't do.  So either what I have already, or your suggestion of using
the existing function with SYSLOG_ACTION_OPEN in devkmsg_open.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

Hi,

On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> Originally, the addition of dmesg_restrict covered both the syslog
> method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> indirectly by security_syslog calling cap_syslog before doing any LSM
> checks.

Actually, are the security_syslog() checks in /dev/kmsg correct? There is
only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't
it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to
devkmsg_read? (And should we add one for write?)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> > Originally, the addition of dmesg_restrict covered both the syslog
> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> > indirectly by security_syslog calling cap_syslog before doing any LSM
> > checks.
> > 
> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> > logic to fix build failure) moved the code around and pushed the checks
> > into the caller itself.  That seems to have inadvertently dropped the
> > checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
> > because util-linux dmesg(1) defaults to using the syslog method for
> > access in older versions.  With util-linux 2.22 and a kernel newer than
> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> > 
> > Fix this by making an explicit check in the devkmsg_open function.
> > 
> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> > 
> > Reported-by: Christian Kujau <lists@nerdbynature.de>
> > CC: stable@vger.kernel.org
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > ---
> >  kernel/printk.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/printk.c b/kernel/printk.c
> > index f24633a..398ef9a 100644
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> >  	struct devkmsg_user *user;
> >  	int err;
> >  
> > +	if (dmesg_restrict && !capable(CAP_SYSLOG))
> > +		return -EACCES;
> > +
> 
> I think this should use check_syslog_permissions() instead, as done for
> /proc/kmsg and the syslog syscall.
> 
> 	err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);

Did you mean SYSLOG_ACTION_OPEN?

I didn't code it that way because the comment in that function about the
capability checks already being done seem pretty off to me.  I could
have just misread the /proc code though.  I can resend with the change
you suggest if everyone thinks that's a better way.

Also, the LSM hooks aren't doing any capability checks at all that I can
see, which may or may not be a bug in and of itself but I have no idea.
I was hoping Eric would speak up about that.

> 	if (err)
> 		return err;
> 
> And going forward we should probably think about dropping the CAP_SYS_ADMIN
> backward-compat code in check_syslog_permissions.

Sure, but that's a separate commit.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> Originally, the addition of dmesg_restrict covered both the syslog
> method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> indirectly by security_syslog calling cap_syslog before doing any LSM
> checks.
> 
> However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> logic to fix build failure) moved the code around and pushed the checks
> into the caller itself.  That seems to have inadvertently dropped the
> checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
> because util-linux dmesg(1) defaults to using the syslog method for
> access in older versions.  With util-linux 2.22 and a kernel newer than
> 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> 
> Fix this by making an explicit check in the devkmsg_open function.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> 
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
>  kernel/printk.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index f24633a..398ef9a 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	struct devkmsg_user *user;
>  	int err;
>  
> +	if (dmesg_restrict && !capable(CAP_SYSLOG))
> +		return -EACCES;
> +

I think this should use check_syslog_permissions() instead, as done for
/proc/kmsg and the syslog syscall.

	err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
	if (err)
		return err;

And going forward we should probably think about dropping the CAP_SYS_ADMIN
backward-compat code in check_syslog_permissions.

>  	/* write-only does not need any file context */
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

Originally, the addition of dmesg_restrict covered both the syslog
method of accessing dmesg, as well as /dev/kmsg itself.  This was done
indirectly by security_syslog calling cap_syslog before doing any LSM
checks.

However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
logic to fix build failure) moved the code around and pushed the checks
into the caller itself.  That seems to have inadvertently dropped the
checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
because util-linux dmesg(1) defaults to using the syslog method for
access in older versions.  With util-linux 2.22 and a kernel newer than
3.5, dmesg(1) defaults to reading directly from /dev/kmsg.

Fix this by making an explicit check in the devkmsg_open function.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Reported-by: Christian Kujau <lists@nerdbynature.de>
CC: stable@vger.kernel.org
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 kernel/printk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/printk.c b/kernel/printk.c
index f24633a..398ef9a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	struct devkmsg_user *user;
 	int err;
 
+	if (dmesg_restrict && !capable(CAP_SYSLOG))
+		return -EACCES;
+
 	/* write-only does not need any file context */
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
-- 
1.8.1.2