| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 24 Apr 2013 17:21:12 -0400
From: Josh Boyer <jwboyer@...hat.com>
To: Kees Cook <keescook@...omium.org>
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 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.
> 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 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