On Wed, Apr 24, 2013 at 01:58:35PM -0400, Josh Boyer wrote: > On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote: > > On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keesc...@chromium.org> wrote: > > > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwbo...@redhat.com> 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 > > >> > > >> 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> > > > > > > Thanks! > > > > > > Acked-by: Kees Cook <keesc...@chromium.org> > > > > If that's the version currently in Fedora, we just cannot do this. > > https://bugzilla.redhat.com/show_bug.cgi?id=952655 > > > > /dev/kmsg is supposed, and was added, to be a sane alternative to > > syslog(). It is already used in dmesg(1) which is now broken with this > > patch. > > > > The access rules for /dev/kmsg should follow the access rules of > > syslog(), and not be any stricter. > > I haven't tested it yet, but I think something like this should work > while still honoring dmesg_restrict. I'll test it out while the rest > of you debate things.
Yeah, that seems to work. So, comments or Reviewed-by/Acked-by on it would be welcome. josh > From: Josh Boyer <jwbo...@redhat.com> > Date: Tue, 9 Apr 2013 11:08:13 -0400 > Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg > > 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 > > 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> > --- > v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make > devkmsg_read honor dmesg_restrict > > kernel/printk.c | 91 > +++++++++++++++++++++++++++++---------------------------- > 1 file changed, 47 insertions(+), 44 deletions(-) > > diff --git a/kernel/printk.c b/kernel/printk.c > index abbdd9e..2d7be05 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_CALL); > + 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_READ_ALL, > 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/
