On Tue, Apr 09, 2013 at 11:48:20AM -0400, Josh Boyer wrote: > The dmesg_restrict sysctl currently covers the syslog method for access > dmesg, however /dev/kmsg isn't covered by the same protections. Most > people haven't noticed because util-linux dmesg(1) defaults to using the > syslog method for access in older versions. With util-linux dmesg(1) > defaults to reading directly from /dev/kmsg. > > Fix this by reworking all of the access methods to use the > check_syslog_permissions function and adding checks to devkmsg_open and > devkmsg_read. > > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
So this does fix that bug. But then it introduced this one: https://bugzilla.redhat.com/show_bug.cgi?id=952655 Basically, dmesg(1) now always falls back to using the syslog interface instead of /dev/kmsg because nothing is granting CAP_SYSLOG for normal users. That was somewhat intentional based on the feedback from Kees and Eric, but it does present a problem. If we want to keep the existing open behavior for /dev/kmsg, and still honor dmesg_restrict, we basically need it to fail in devkmsg_read. With the current functions we have, that won't work so we'll either need to hack that up or just have devkmsg_read call syslog_action_restricted instead. josh > Reported-by: Christian Kujau <li...@nerdbynature.de> > CC: sta...@vger.kernel.org > Signed-off-by: Eric Paris <epa...@redhat.com> > Signed-off-by: Josh Boyer <jwbo...@redhat.com> > --- > > v2: Rework patch based on code from Eric Paris, add check in devkmsg_read as > suggested by Kees Cook. > > kernel/printk.c | 91 > +++++++++++++++++++++++++++++---------------------------- > 1 file changed, 47 insertions(+), 44 deletions(-) > > diff --git a/kernel/printk.c b/kernel/printk.c > index abbdd9e..5541095 100644 > --- a/kernel/printk.c > +++ b/kernel/printk.c > @@ -368,6 +368,46 @@ static void log_store(int facility, int level, > log_next_seq++; > } > > +#ifdef CONFIG_SECURITY_DMESG_RESTRICT > +int dmesg_restrict = 1; > +#else > +int dmesg_restrict; > +#endif > + > +static int syslog_action_restricted(int type) > +{ > + if (dmesg_restrict) > + return 1; > + /* Unless restricted, we allow "read all" and "get buffer size" for > everybody */ > + return type != SYSLOG_ACTION_READ_ALL && type != > SYSLOG_ACTION_SIZE_BUFFER; > +} > + > +static int check_syslog_permissions(int type, bool from_file) > +{ > + /* > + * 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) > + goto ok; > + > + if (syslog_action_restricted(type)) { > + if (capable(CAP_SYSLOG)) > + 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)); > + goto ok; > + } > + return -EPERM; > + } > +ok: > + return security_syslog(type); > +} > + > /* /dev/kmsg - userspace message inject/listen interface */ > struct devkmsg_user { > u64 seq; > @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char > __user *buf, > char cont = '-'; > size_t len; > ssize_t ret; > + int err; > > if (!user) > return -EBADF; > > + err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, > + SYSLOG_FROM_FILE); > + if (err) > + return err; > + > ret = mutex_lock_interruptible(&user->lock); > if (ret) > return ret; > @@ -624,7 +670,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; > > @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level) > } > #endif > > -#ifdef CONFIG_SECURITY_DMESG_RESTRICT > -int dmesg_restrict = 1; > -#else > -int dmesg_restrict; > -#endif > - > -static int syslog_action_restricted(int type) > -{ > - if (dmesg_restrict) > - return 1; > - /* Unless restricted, we allow "read all" and "get buffer size" for > everybody */ > - return type != SYSLOG_ACTION_READ_ALL && type != > SYSLOG_ACTION_SIZE_BUFFER; > -} > - > -static int check_syslog_permissions(int type, bool from_file) > -{ > - /* > - * 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) > - return 0; > - > - if (syslog_action_restricted(type)) { > - if (capable(CAP_SYSLOG)) > - return 0; > - /* 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; > - } > - return -EPERM; > - } > - return 0; > -} > - > #if defined(CONFIG_PRINTK_TIME) > static bool printk_time = 1; > #else > @@ -1131,10 +1138,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.8.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
