2013-03-22 21:54:52

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg


poke. Nothing got applied. I'll drop
kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
effect ;)


From: Josh Boyer <[email protected]>
Subject: kmsg: honor dmesg_restrict sysctl on /dev/kmsg

Originally, the addition of dmesg_restrict covered both the syslog
method of accessing dmesg, as well as /dev/kmsg itself. This was done
indirectly by security_syslog calling cap_syslog before doing any LSM
checks.

However, commit 12b3052c3ee ("capabilities/syslog: open code cap_syslog
logic to fix build failure") moved the code around and pushed the checks
into the caller itself. That seems to have inadvertently dropped the
checks for dmesg_restrict on /dev/kmsg. Most people haven't noticed
because util-linux dmesg(1) defaults to using the syslog method for access
in older versions. With util-linux 2.22 and a kernel newer than 3.5,
dmesg(1) defaults to reading directly from /dev/kmsg.

Fix this by making an explicit check in the devkmsg_open function.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Signed-off-by: Josh Boyer <[email protected]>
Reported-by: Christian Kujau <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: James Morris <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

kernel/printk.c | 3 +++
1 file changed, 3 insertions(+)

diff -puN kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg kernel/printk.c
--- a/kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg
+++ a/kernel/printk.c
@@ -620,6 +620,9 @@ static int devkmsg_open(struct inode *in
struct devkmsg_user *user;
int err;

+ if (dmesg_restrict && !capable(CAP_SYSLOG))
+ return -EACCES;
+
/* write-only does not need any file context */
if ((file->f_flags & O_ACCMODE) == O_WRONLY)
return 0;
_


2013-03-22 22:14:55

by Josh Boyer

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>
> poke. Nothing got applied. I'll drop
> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> effect ;)

Oh dear.

Eric, were you going to cleanup your suggestion and send it out?

josh

2013-04-01 23:52:00

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <[email protected]> wrote:
> On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>>
>> poke. Nothing got applied. I'll drop
>> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> effect ;)
>
> Oh dear.
>
> Eric, were you going to cleanup your suggestion and send it out?

Ping? What state is this in?

-Kees

--
Kees Cook
Chrome OS Security

2013-04-02 01:06:02

by Josh Boyer

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

----- Original Message -----
> From: "Kees Cook" <[email protected]>
> To: "Josh Boyer" <[email protected]>
> Cc: "Andrew Morton" <[email protected]>, "Eric Paris" <[email protected]>, "Linus Torvalds"
> <[email protected]>, "Christian Kujau" <[email protected]>, "# 3.4.x" <[email protected]>,
> "LKML" <[email protected]>
> Sent: Monday, April 1, 2013 7:51:57 PM
> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>
> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <[email protected]> wrote:
> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >>
> >> poke. Nothing got applied. I'll drop
> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> effect ;)
> >
> > Oh dear.
> >
> > Eric, were you going to cleanup your suggestion and send it out?
>
> Ping? What state is this in?

If Eric doesn't send a patch tomorrow, I will. Fedora is carrying my
original patch, but I'd rather get this fixed everywhere.

josh

2013-04-08 21:34:40

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <[email protected]> wrote:
> ----- Original Message -----
>> From: "Kees Cook" <[email protected]>
>> To: "Josh Boyer" <[email protected]>
>> Cc: "Andrew Morton" <[email protected]>, "Eric Paris" <[email protected]>, "Linus Torvalds"
>> <[email protected]>, "Christian Kujau" <[email protected]>, "# 3.4.x" <[email protected]>,
>> "LKML" <[email protected]>
>> Sent: Monday, April 1, 2013 7:51:57 PM
>> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>>
>> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <[email protected]> wrote:
>> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>> >>
>> >> poke. Nothing got applied. I'll drop
>> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> >> effect ;)
>> >
>> > Oh dear.
>> >
>> > Eric, were you going to cleanup your suggestion and send it out?
>>
>> Ping? What state is this in?
>
> If Eric doesn't send a patch tomorrow, I will. Fedora is carrying my
> original patch, but I'd rather get this fixed everywhere.

Pinging again here. Any news on this?

-Kees

--
Kees Cook
Chrome OS Security

2013-04-09 00:51:04

by Josh Boyer

[permalink] [raw]
Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

On Mon, Apr 08, 2013 at 02:34:38PM -0700, Kees Cook wrote:
> On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <[email protected]> wrote:
> > ----- Original Message -----
> >> From: "Kees Cook" <[email protected]>
> >> To: "Josh Boyer" <[email protected]>
> >> Cc: "Andrew Morton" <[email protected]>, "Eric Paris" <[email protected]>, "Linus Torvalds"
> >> <[email protected]>, "Christian Kujau" <[email protected]>, "# 3.4.x" <[email protected]>,
> >> "LKML" <[email protected]>
> >> Sent: Monday, April 1, 2013 7:51:57 PM
> >> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> >>
> >> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <[email protected]> wrote:
> >> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >> >>
> >> >> poke. Nothing got applied. I'll drop
> >> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> >> effect ;)
> >> >
> >> > Oh dear.
> >> >
> >> > Eric, were you going to cleanup your suggestion and send it out?
> >>
> >> Ping? What state is this in?
> >
> > If Eric doesn't send a patch tomorrow, I will. Fedora is carrying my
> > original patch, but I'd rather get this fixed everywhere.
>
> Pinging again here. Any news on this?

I am now almost as delinquent as Eric. I owe you one beer of your
choice. You can add one beer to that tally every day until I send out a
cleaned up patch. Maybe that will motivate me to send one.

More seriously, I was on PTO at the end of last week and this fell off
my plate. I will clean it up and test it tomorrow, with the aim of
actually sending something tomorrow afternoon.

josh

2013-04-09 15:48:31

by Josh Boyer

[permalink] [raw]
Subject: [PATCH v2] 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 <[email protected]>
CC: [email protected]
Signed-off-by: Eric Paris <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
---

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

2013-04-09 16:33:11

by Kees Cook

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

On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
> CC: [email protected]
> Signed-off-by: Eric Paris <[email protected]>
> Signed-off-by: Josh Boyer <[email protected]>

Thanks!

Acked-by: Kees Cook <[email protected]>

--
Kees Cook
Chrome OS Security

2013-04-24 17:43:42

by Josh Boyer

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

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 <[email protected]>
> CC: [email protected]
> Signed-off-by: Eric Paris <[email protected]>
> Signed-off-by: Josh Boyer <[email protected]>
> ---
>
> 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
>

2013-04-24 17:44:57

by Kay Sievers

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

On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <[email protected]> wrote:
> On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
>> CC: [email protected]
>> Signed-off-by: Eric Paris <[email protected]>
>> Signed-off-by: Josh Boyer <[email protected]>
>
> Thanks!
>
> Acked-by: Kees Cook <[email protected]>

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.

Thanks,
Kay

2013-04-24 17:58:47

by Josh Boyer

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

On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
> On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <[email protected]> wrote:
> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
> >> CC: [email protected]
> >> Signed-off-by: Eric Paris <[email protected]>
> >> Signed-off-by: Josh Boyer <[email protected]>
> >
> > Thanks!
> >
> > Acked-by: Kees Cook <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
CC: [email protected]
Signed-off-by: Eric Paris <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
---
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

2013-04-24 19:50:43

by Josh Boyer

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

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 <[email protected]> wrote:
> > > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
> > >> CC: [email protected]
> > >> Signed-off-by: Eric Paris <[email protected]>
> > >> Signed-off-by: Josh Boyer <[email protected]>
> > >
> > > Thanks!
> > >
> > > Acked-by: Kees Cook <[email protected]>
> >
> > 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 <[email protected]>
> 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 <[email protected]>
> CC: [email protected]
> Signed-off-by: Eric Paris <[email protected]>
> Signed-off-by: Josh Boyer <[email protected]>
> ---
> 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
>

2013-04-24 20:35:21

by Kees Cook

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

On Wed, Apr 24, 2013 at 10:58 AM, Josh Boyer <[email protected]> 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 <[email protected]> wrote:
>> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
>> >> CC: [email protected]
>> >> Signed-off-by: Eric Paris <[email protected]>
>> >> Signed-off-by: Josh Boyer <[email protected]>
>> >
>> > Thanks!
>> >
>> > Acked-by: Kees Cook <[email protected]>
>>
>> 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 <[email protected]>
> 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 <[email protected]>
> CC: [email protected]
> Signed-off-by: Eric Paris <[email protected]>
> Signed-off-by: Josh Boyer <[email protected]>
> ---
> 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

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

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

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

-Kees

--
Kees Cook
Chrome OS Security

2013-04-24 21:21:24

by Josh Boyer

[permalink] [raw]
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 <[email protected]> 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 <[email protected]> wrote:
> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
> >> >> CC: [email protected]
> >> >> Signed-off-by: Eric Paris <[email protected]>
> >> >> Signed-off-by: Josh Boyer <[email protected]>
> >> >
> >> > Thanks!
> >> >
> >> > Acked-by: Kees Cook <[email protected]>
> >>
> >> 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 <[email protected]>
> > 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 <[email protected]>
> > CC: [email protected]
> > Signed-off-by: Eric Paris <[email protected]>
> > Signed-off-by: Josh Boyer <[email protected]>
> > ---
> > 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

2013-04-24 21:30:56

by Linus Torvalds

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

On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <[email protected]> wrote:
>
> 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.

Bullshit.

That's exactly the wrong kind of thinking. If you had privileges to
open something, and you pass it off, it's *your* choice.

In contrast, the "anybody can open, but some people can read/write"
has several times resulted in real security issues. Notably the whole
"open something, then fool a suid program to write its error message
to it".

This whole discussion has been f*cking moronic. The "security"
arguments have been utter shite with clearly no thinking behind it,
the feature is total crap (people need dmesg to do basic bug
reporting), and I'm seriously considering just getting rid of this
idiotic dmesg_restrict thing entirely. Your comment is the very
epitome of bad security thinking.

Linus

2013-04-24 21:36:42

by Kees Cook

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

On Wed, Apr 24, 2013 at 2:21 PM, Josh Boyer <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <[email protected]> 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 <[email protected]>
>> >> >> CC: [email protected]
>> >> >> Signed-off-by: Eric Paris <[email protected]>
>> >> >> Signed-off-by: Josh Boyer <[email protected]>
>> >> >
>> >> > Thanks!
>> >> >
>> >> > Acked-by: Kees Cook <[email protected]>
>> >>
>> >> 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 <[email protected]>
>> > 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 <[email protected]>
>> > CC: [email protected]
>> > Signed-off-by: Eric Paris <[email protected]>
>> > Signed-off-by: Josh Boyer <[email protected]>
>> > ---
>> > 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

2013-04-24 21:41:50

by Kees Cook

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

On Wed, Apr 24, 2013 at 2:30 PM, Linus Torvalds
<[email protected]> wrote:
> On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <[email protected]> wrote:
>>
>> 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.
>
> Bullshit.
>
> That's exactly the wrong kind of thinking. If you had privileges to
> open something, and you pass it off, it's *your* choice.

Yes, this is what I was pointing out originally. The semantics of
/proc/kmsg do exactly that: check at open time, which is much cleaner.

Solving the permissions checking delta between the syslog via syscall
and syslog via /proc/kmsg was the original intent of the code so that
capabilities could be dropped after open. And when /dev/kmsg came
along, it didn't follow either convention. I just want to see the
behavior standardized.

-Kees

--
Kees Cook
Chrome OS Security

2013-04-24 21:51:14

by Josh Boyer

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

On Wed, Apr 24, 2013 at 02:36:39PM -0700, Kees Cook wrote:
> >>
> >> 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).

Ah.

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

OK.

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

I have no idea on this front. I'll let Kay speak to that. On my
currently running Fedora 18 system, I actually have systemd-journald
using /dev/kmsg, and rsyslog using /proc/kmsg. Why I have both, I have
no friggin idea.

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

Maybe? I mean, systemd-journald seems to be using it for something.
Kay?

josh

2013-04-24 22:01:21

by Josh Boyer

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

On Wed, Apr 24, 2013 at 02:30:53PM -0700, Linus Torvalds wrote:
> On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <[email protected]> wrote:
> >
> > 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.
>
> Bullshit.
>
> That's exactly the wrong kind of thinking. If you had privileges to
> open something, and you pass it off, it's *your* choice.
>
> In contrast, the "anybody can open, but some people can read/write"
> has several times resulted in real security issues. Notably the whole
> "open something, then fool a suid program to write its error message
> to it".
>
> This whole discussion has been f*cking moronic. The "security"
> arguments have been utter shite with clearly no thinking behind it,
> the feature is total crap (people need dmesg to do basic bug
> reporting), and I'm seriously considering just getting rid of this
> idiotic dmesg_restrict thing entirely. Your comment is the very
> epitome of bad security thinking.

I was just trying to get the 3 interfaces all honoring the same thing.

Let this be a lesson to you all: I am the harbinger of security
features removal. If you see me sending patches, run away or I might
accidentally cross the streams and make your feature undergo total
protonic reversal.

Now if only I could use this power for good, like somehow getting Linus
to remove capabilities entirely...

josh

2013-04-24 23:52:46

by Kay Sievers

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

On Wed, Apr 24, 2013 at 11:51 PM, Josh Boyer <[email protected]> wrote:

>> 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.
>
> I have no idea on this front. I'll let Kay speak to that.

The original code checks once at open() only, which would allow to do
do all that privilege dropping. It is how I would expect it to work,
instead of checking the permissions at every read().

> On my
> currently running Fedora 18 system, I actually have systemd-journald
> using /dev/kmsg

That's the recent structured logging stuff.

> and rsyslog using /proc/kmsg.

That's the old plain text syslog daemon stuff.

> Why I have both, I have no friggin idea.

Nobody removed the old syslog dameon by default from the distro. If
you don't want or need the plain text files in /var/log/ anymore, just
uninstall it and use journalctl(1) to see the system logs from then
on.

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

Not that I know.

> Maybe? I mean, systemd-journald seems to be using it for something.
> Kay?

I doubt that old syslog implementations will be ported to a new kernel
interface. They work just fine the way they are, and the structured
data that is additionally put out on the new interface, they cannot
really store away anyway in their plain text files, so they do not
gain anything really.

What we can probably expect though, is that in the future the default
systems will not install any old syslog daemon, which uses that
interface anymore.

Kay