Re: [PATCH v2] security_syslog() should be called once only

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 24 May 2015 19:18:40 +0300 Vasily Averin <vvs@virtuozzo.com> wrote:

> v2: subject changed, patch comment modified
> 
> Fixes: 637241a900cb ("kmsg: honor dmesg_restrict sysctl on /dev/kmsg")
> 
> Final version of patch 637241a900cb ("kmsg: honor dmesg_restrict sysctl
> on /dev/kmsg") lost few hooks, as result security_syslog() are processed
> incorrectly:
> - open of /dev/kmsg checks syslog access permissions by using
> check_syslog_permissions() where security_syslog() is not called
> if dmesg_restrict is set.
> - syslog syscall and /proc/kmsg calls do_syslog()
> where security_syslog can be executed twice
> (inside check_syslog_permissions() and then directly in do_syslog())
> 
> With this patch security_syslog() is called once only in all syslog-related
> operations regardless of dmesg_restrict value.
> 
> ...
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -484,11 +484,11 @@ 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;

So we run security_syslog() for actions other than open() (of kmsg). 
Why?


Also, that from_file handling makes me cry.

#define SYSLOG_FROM_READER           0
#define SYSLOG_FROM_PROC             1

That's not a boolean - it's an enumerated value with two values
currently defined.

But the code in check_syslog_permissions() treats it as a boolean and
also hardwires the knowledge that SYSLOG_FROM_PROC == 1 (or == `true`).

And the name is wrong: it should be called from_proc to match
SYSLOG_FROM_PROC.

One possible fix would be something like this, plus various
fixups/audit:

--- a/kernel/printk/printk.c~security_syslog-should-be-called-once-only-fix
+++ a/kernel/printk/printk.c
@@ -489,13 +489,13 @@ static int syslog_action_restricted(int
 	       type != SYSLOG_ACTION_SIZE_BUFFER;
 }
 
-int check_syslog_permissions(int type, bool from_file)
+int check_syslog_permissions(int type, int source)
 {
 	/*
 	 * 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)
+	if (source == SYSLOG_FROM_PROC && type != SYSLOG_ACTION_OPEN)
 		goto ok;
 
 	if (syslog_action_restricted(type)) {
_


And `type' should be renamed to `action' for heavens sake.  Kees, were
you drunk?