Message-ID: <CAGXu5jJweZVLhz0pP6=Woud=aqAKgNmG-7T5WKYdmWR4gxcw9w@mail.gmail.com>
Date:	Wed, 24 Apr 2013 14:36:39 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Josh Boyer <jwboyer@...hat.com>
Cc:	Kay Sievers <kay@...y.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eric Paris <eparis@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christian Kujau <lists@...dbynature.de>,
	"# 3.4.x" <stable@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, kzak@...hat.com
Subject: Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

On Wed, Apr 24, 2013 at 2:21 PM, Josh Boyer <jwboyer@...hat.com> wrote:
> 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@...hat.com> 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@...omium.org> wrote:
>> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@...hat.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 <lists@...dbynature.de>
>> >> >> CC: stable@...r.kernel.org
>> >> >> Signed-off-by: Eric Paris <eparis@...hat.com>
>> >> >> Signed-off-by: Josh Boyer <jwboyer@...hat.com>
>> >> >
>> >> > Thanks!
>> >> >
>> >> > Acked-by: Kees Cook <keescook@...omium.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.
>> >
>> > josh
>> >
>> > From: Josh Boyer <jwboyer@...hat.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 <lists@...dbynature.de>
>> > CC: stable@...r.kernel.org
>> > Signed-off-by: Eric Paris <eparis@...hat.com>
>> > Signed-off-by: Josh Boyer <jwboyer@...hat.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;
>>
>> 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.

Right, I meant that Kay's assertion that /proc/kmsg is "legacy" means
he expects syslog daemons to switch to using /dev/kmsg.

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

I don't think I was clear. There are two use-cases, as I see it:

- normal user running dmesg(1)
- system daemon pulling kernel syslog and putting into userspace (e.g.
/var/log/kern.log).

In the dmesg(1) case, we're fine. It was using syscalls, now it can
use /dev/kmsg since open isn't checked, just the read action. That's
all cool by me.

In the daemon case, it's nice to be able to drop privileges after
setting up resources. The past was open /proc/kmsg with CAP_SYS_ADMIN,
then drop CAP_SYS_ADMIN and keep reading. Then later CAP_SYS_LOG was
introduced. So if a daemon switched from /proc/kmsg to /dev/kmsg they
wouldn't be able to drop the capability. But, it's much saner to carry
CAP_SYS_LOG than CAP_SYS_ADMIN on a long-running daemon.

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

Is there an intention to use /dev/kmsg for the syslog management daemon?

>> "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.

Right, your patch fixes things fine. I was pointing out how the
semantics for switching from /proc/kmsg to /dev/kmsg are different,
and why we might want to consider changing the requirements here in an
intentional and clear manner.

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

Yeah, cool.

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists