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