kernel/printk/printk.c: check_syslog_permissions() cleanup

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() is not checked inside check_syslog_permissions() if dmesg_restrict is set, or it can be called twice in do_syslog().

kernel/printk/printk.c +1 -1

 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
-			return 0;
+			goto ok;
 		/*
 		 * For historical reasons, accept CAP_SYS_ADMIN too, with
 		 * a warning.
 
...

Quoted 6 lines · On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <[email protected]> wrote:

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() is not checked inside check_syslog_permissions() if dmesg_restrict is set, or it can be called twice in do_syslog().

I'm not seeing how security_syslog() is called twice from do_syslog(). Put more details in the changelog, please.

kernel/printk/printk.c int check_syslog_permissions(int type, bool from_file) · line 487

Quoted 8 lines

kernel/printk/printk.c +1 -1

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

This seems wrong - we should only call security_syslog() for opens?

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <[email protected]> wrote:

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() is not checked inside check_syslog_permissions() if dmesg_restrict is set, or it can be called twice in do_syslog().

Quoted 2 lines

I'm not seeing how security_syslog() is called twice from do_syslog(). Put more details in the changelog, please.

For example, if dmesg_restrict is not set and SYSLOG_ACTION_OPEN is requested.

In this case do_syslog() calls check_syslog_permissions() where security_syslog() is called first time and approves the operation, then do_syslog() itself calls security_syslog() 2nd time.

kernel/printk/printk.c int check_syslog_permissions(int type, bool from_file) · line 487

Quoted 8 lines · On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <[email protected]> wrote:

kernel/printk/printk.c +1 -1

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

This seems wrong - we should only call security_syslog() for opens?

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.

...

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

Also, that from_file handling makes me cry.

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

And type' should be renamed to action' for heavens sake.

Quoted 2 lines · Andrew Morton

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

Could you please clarify this question?

Linux kernel have reasonable default security policy and it's great. And at the same time kernel allows to override default behaviour and set custom security policy.
For example, to prohibit work on Saturday.
QA can use it for random failures generation.
Why not?

Quoted 11 lines · On Sat, 30 May 2015 16:51:34 +0300 Vasily Averin <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

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

Could you please clarify this question?

Linux kernel have reasonable default security policy and it's great. And at the same time kernel allows to override default behaviour and set custom security policy.
For example, to prohibit work on Saturday.
QA can use it for random failures generation.
Why not?

This change:

: --- a/kernel/printk/printk.c~security_syslog-should-be-called-once-only : +++ a/kernel/printk/printk.c
: @@ -496,11 +496,11 @@ int check_syslog_permissions(int type, b

:  	 * already done the capabilities checks at open time.
:  	 */
:  	if (from_file && type != SYSLOG_ACTION_OPEN)

: - return 0;
: + goto ok;
:
: ...
:

:  		}
:  		return -EPERM;
:  	}

: +ok:
: return security_syslog(type);
: }

Means that we will now call security_syslog() for SYSLOG_ACTION_CLOSE, SYSLOG_ACTION_READ, SYSLOG_ACTION_READ_ALL, etc.

That's new behaviour and it may be wrong. Why should check_syslog_permissions() call security_syslog() for anything other than SYSLOG_ACTION_OPEN?

Quoted 22 lines · On Sun, 24 May 2015 19:18:40 +0300 Vasily Averin <[email protected]> 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.

...

kernel/printk/printk.c int check_syslog_permissions(int type, bool from_file) · line 487

Quoted 5 lines

kernel/printk/printk.c +1 -1

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

kernel/printk/printk.c renamed from kernel/printk/printk.c~security_syslog-should-be-called-once-only-fix +2 -2

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