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

[Kees Cook]

On Wed, May 27, 2015 at 4:43 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sun, 24 May 2015 19:18:40 +0300 Vasily Averin <vvs@xxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/