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

[Kees Cook]

On Wed, Feb 27, 2013 at 2:19 PM, Josh Boyer <jwboyer@xxxxxxxxxx> 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
--
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/