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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html