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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 24, 2013 at 01:35:17PM -0700, Kees Cook wrote:
> On Wed, Apr 24, 2013 at 10:58 AM, Josh Boyer <jwboyer@xxxxxxxxxx> 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 <keescook@xxxxxxxxxxxx> wrote:
> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@xxxxxxxxxx> 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 <lists@xxxxxxxxxxxxxxx>
> >> >> CC: stable@xxxxxxxxxxxxxxx
> >> >> Signed-off-by: Eric Paris <eparis@xxxxxxxxxx>
> >> >> Signed-off-by: Josh Boyer <jwboyer@xxxxxxxxxx>
> >> >
> >> > Thanks!
> >> >
> >> > Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
> >>
> >> 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.
> >
> > josh
> >
> > From: Josh Boyer <jwboyer@xxxxxxxxxx>
> > 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 <lists@xxxxxxxxxxxxxxx>
> > CC: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Eric Paris <eparis@xxxxxxxxxx>
> > Signed-off-by: Josh Boyer <jwboyer@xxxxxxxxxx>
> > ---
> >  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;
> 
> So, the problem here is the expectation of privileges. The /proc/kmsg
> usage pattern was:
> 
> open /proc/kmsg with CAP_SYSLOG
> drop CAP_SYSLOG
> read /proc/kmsg forever

This doesn't change the /proc interface at all.

> If we change the FILE vs CALL and OPEN vs READ stuff here, we're lying
> to the API about what's happening. If we use this patch, then we can't
> use /dev/kmsg in the same way (i.e. running without privileges).

Uh... Yes we can.  I tested it as a normal user.  It works just fine
running without privs and without CAP_SYSLOG, like it did before there
was a patch at all.  It also honors dmesg_restrict in devkmsg_read.
I'm confused why you think this doesn't work?


> That said, I much prefer doing the privilege test at read time since
> that means passing a file descriptor to another process doesn't mean
> the new process can just continue reading. If we're going to be
> defining the new behavior for /dev/kmsg, then I think we should
> explicitly drop the fall-back to CAP_SYS_ADMIN. Perhaps introduce a

I think Karel and Kay's entire point was /dev/kmsg shouldn't be getting
new behavior.  Aside from honoring dmesg_restrict, they see any behavior
change as a regression.

> "legacy_privs" option to check_syslog_permissions() and leave it set
> for do_syslog and cleared for devkmsg_*. This means we can run a
> /dev/kmsg reader with _only_ CAP_SYSLOG. (A while back, the original
> intent that reworked the permissions here was to avoid needing
> CAP_SYS_ADMIN.)

Right, but /dev/kmsg didn't require CAP_SYSLOG before any version of
this patch existed.  That's the bug Karel and Kay are reporting.  As
far as I can see, that should be just fine for reading unless
dmesg_restrict is set.

> And then maybe we need to rename the from_file to be from_proc (and
> similarly SYSLOG_FROM_PROC) too?

Well, that can be done regardless of what falls out from above and would
probably make things clearer.

josh
--
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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]