[PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[Vasily Averin]

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

Signed-off-by: Vasily Averin <vvs@openvz.org>
---
 kernel/printk/printk.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c099b08..bff0169 100644
--- 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;
 
 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
-			return 0;
+			goto ok;
 		/*
 		 * For historical reasons, accept CAP_SYS_ADMIN too, with
 		 * a warning.
@@ -498,10 +498,11 @@ int check_syslog_permissions(int type, bool from_file)
 				     "CAP_SYS_ADMIN but no CAP_SYSLOG "
 				     "(deprecated).\n",
 				 current->comm, task_pid_nr(current));
-			return 0;
+			goto ok;
 		}
 		return -EPERM;
 	}
+ok:
 	return security_syslog(type);
 }
 
@@ -1263,10 +1264,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;
-- 
1.9.1

Re: [PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[Andrew Morton]

On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <vvs@odin.com> 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.

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

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

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

Re: [PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[Vasily Averin]

On 15.05.2015 01:01, Andrew Morton wrote:
> On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <vvs@odin.com> 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.

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.

>> --- 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;
> 
> This seems wrong - we should only call security_syslog() for opens?

Are you sure?
I saw Linus comment in thread related to old patch, 
and I agree that usual kernel checks can be skipped.

However I believe security hooks should be called anyway,
in general case they can have own rules about access, logging
or additional things that should be called before execution of requested operation.

Am I wrong?

Re: [PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[Vasily Averin]

On 15.05.2015 10:41, Vasily Averin wrote:
> On 15.05.2015 01:01, Andrew Morton wrote:
>> On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <vvs@odin.com> 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.
> 
> For example, if dmesg_restrict is not set and SYSLOG_ACTION_OPEN is requested.

no, SYSLOG_ACTION_OPEN does not fit.

syslog_action_restricted() should return 0,
so it should be SYSLOG_ACTION_READ_ALL or SYSLOG_ACTION_SIZE_BUFFER commands
and from_file should be set to SYSLOG_FROM_READER.

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

Re: [PATCH] kernel/printk/printk.c: check_syslog_permissions() cleanup

[Vasily Averin]

On 15.05.2015 01:01, Andrew Morton wrote:
> On Sun, 10 May 2015 09:35:53 +0300 Vasily Averin <vvs@odin.com> wrote:
>> --- 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;
> 
> This seems wrong - we should only call security_syslog() for opens?

No I think my version is correct.
Currently security hook _is_ called in this situation:
from_file is set in /proc/kmsg handlers only, 
they use do_syslog() and call security_syslog() directly
after check_syslog_permissions() call.

My patch forces check_syslog_permissions() to call security hook in all cases,
that allows to remove extra security_syslog() call from do_syslog().

I've changed patch comment and going to resend new patch version soon.

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

[Vasily Averin]

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.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 kernel/printk/printk.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c099b08..bff0169 100644
--- 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;
 
 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
-			return 0;
+			goto ok;
 		/*
 		 * For historical reasons, accept CAP_SYS_ADMIN too, with
 		 * a warning.
@@ -498,10 +498,11 @@ int check_syslog_permissions(int type, bool from_file)
 				     "CAP_SYS_ADMIN but no CAP_SYSLOG "
 				     "(deprecated).\n",
 				 current->comm, task_pid_nr(current));
-			return 0;
+			goto ok;
 		}
 		return -EPERM;
 	}
+ok:
 	return security_syslog(type);
 }
 
@@ -1263,10 +1264,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;
-- 
1.9.1

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

[Andrew Morton]

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?

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

[Vasily Averin]

On 28.05.2015 02:43, Andrew Morton wrote:
> 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?

> Also, that from_file handling makes me cry.
...
> One possible fix would be something like this, plus various
> fixups/audit:

I've prepared such patch and send it in separate letter.

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

IMHO 'type' and SYSLOG_ACTION_* was inherited from sys_syslog definition.

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

[Andrew Morton]

On Sat, 30 May 2015 16:51:34 +0300 Vasily Averin <vvs@virtuozzo.com> wrote:

> On 28.05.2015 02:43, Andrew Morton wrote:
> > 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?

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

[Vasily Averin]

On 02.06.2015 00:23, Andrew Morton wrote:
> On Sat, 30 May 2015 16:51:34 +0300 Vasily Averin <vvs@virtuozzo.com> wrote:
> 
>> On 28.05.2015 02:43, Andrew Morton wrote:
>>> 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?

But it isn't new behaviour.
Previously security_syslog() was called from do_syslog(), 
now it will be called from check_syslog_permissions()

from_file = true == SYSLOG_FROM_PROC is set in kmsg_open/release/read/pool()
only. These functions use do_syslog() that had called security_syslog() 
right after return from check_syslog_permissions().

sys_syslog() calls this security hook for any action and does it long time ago.

The only place where behaviour is changed, where hook was _NOT_called is
check_syslog_permissions(SYSLOG_ACTION_READ_ALL) calls from devkmsg_open()
and pstore_check_syslog_permissions().
But they does it only if dmesg_restrict is set, that looks wrong for me,
because dmesg_restict should add restrictions but do not remove existing ones.

So I do not see any new problems here.

[PATCH] check_syslog_permissions() cleanup

[Vasily Averin]

Patch fixes drawbacks in heck_syslog_permissions() noticed by AKPM:
"from_file handling makes me cry.

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

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 include/linux/syslog.h |  6 +++---
 kernel/printk/printk.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/syslog.h b/include/linux/syslog.h
index 4b7b875..c3a7f0c 100644
--- a/include/linux/syslog.h
+++ b/include/linux/syslog.h
@@ -47,12 +47,12 @@
 #define SYSLOG_FROM_READER           0
 #define SYSLOG_FROM_PROC             1
 
-int do_syslog(int type, char __user *buf, int count, bool from_file);
+int do_syslog(int type, char __user *buf, int count, int source);
 
 #ifdef CONFIG_PRINTK
-int check_syslog_permissions(int type, bool from_file);
+int check_syslog_permissions(int type, int source);
 #else
-static inline int check_syslog_permissions(int type, bool from_file)
+static inline int check_syslog_permissions(int type, int source)
 {
 	return 0;
 }
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bff0169..5ed9d6d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -477,13 +477,13 @@ static int syslog_action_restricted(int type)
 	       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)) {
@@ -1254,13 +1254,13 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 	return len;
 }
 
-int do_syslog(int type, char __user *buf, int len, bool from_file)
+int do_syslog(int type, char __user *buf, int len, int source)
 {
 	bool clear = false;
 	static int saved_console_loglevel = LOGLEVEL_DEFAULT;
 	int error;
 
-	error = check_syslog_permissions(type, from_file);
+	error = check_syslog_permissions(type, source);
 	if (error)
 		goto out;
 
@@ -1343,7 +1343,7 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 			syslog_prev = 0;
 			syslog_partial = 0;
 		}
-		if (from_file) {
+		if (source == SYSLOG_FROM_PROC) {
 			/*
 			 * Short-cut for poll(/"proc/kmsg") which simply checks
 			 * for pending data, not the size; return the count of
-- 
1.9.1

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

[Kees Cook]

On Wed, May 27, 2015 at 4:43 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> 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?

No, it's just been historically ugly code. I tried to improve it
(before it was just using hard coded numbers) by adding the #defines
for the actions.

The fundamental issue is that the syslog syscall interface has the
ability to perform handle-less actions. We need to security-check all
the syscall actions, but only the "open" on (at the time) /proc/kmsg.
Adding devkmsg then gave us a third entry point, and it got even
uglier.

So, the semantics for the security check depends on what was
historically called "type" (but is better known by its SYSLOG_ACTION_*
values). Linus wants security checking done on open where possible, so
for files, the checking must be done against an implicit
"SYSLOG_ACTION_OPEN". For the syscall, each action is individually
checked.

With that in mind, what can we do to make this sensible, and cover
/proc/kmsg, /dev/kmsg, and the syscall interface?

-Kees

-- 
Kees Cook
Chrome OS Security