Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Wed, 2013-05-08 at 14:26 -0700, Kees Cook wrote: > Yeah, that'll be fine. I kind of like having the longer rationale in > the commit message for future reference (i.e. destructive vs > non-destructive, etc), but I'd rather see the code fixed. :) There's no reason not to have both, is there? -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Wed, May 8, 2013 at 2:22 PM, Andrew Morton wrote: > On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook wrote: > >> To fix /dev/kmsg, let's compare the existing interfaces and what they allow: >> >> - /proc/kmsg allows: >> - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive >>single-reader interface (SYSLOG_ACTION_READ). >> - everything, after an open. >> >> - syslog syscall allows: >> - anything, if CAP_SYSLOG. >> - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if >> dmesg_restrict==0. >> - nothing else (EPERM). >> >> The use-cases were: >> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. >> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the >> destructive SYSLOG_ACTION_READs. >> >> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't >> clear the ring buffer. >> >> Based on the comments in devkmsg_llseek, it sounds like actions besides >> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), >> so we have a strict subset of the non-destructive syslog syscall actions. >> >> To this end, move the check as Josh had done, but also rename the constants >> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and >> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows >> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions >> after a capabilities-constrained SYSLOG_ACTION_OPEN check. >> >> - /dev/kmsg allows: >> - open if CAP_SYSLOG or dmesg_restrict==0 >> - reading/polling, after open > > hm, that changelog is wy down in the weeds and anyone who hasn't > been following this with a microscope won't have a clue. > > I went into an earlier patch and dug out this: > > : 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. > : > : Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192 > > Which is still accurate and relevant, yes? Yeah, that'll be fine. I kind of like having the longer rationale in the commit message for future reference (i.e. destructive vs non-destructive, etc), but I'd rather see the code fixed. :) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook wrote: > To fix /dev/kmsg, let's compare the existing interfaces and what they allow: > > - /proc/kmsg allows: > - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive >single-reader interface (SYSLOG_ACTION_READ). > - everything, after an open. > > - syslog syscall allows: > - anything, if CAP_SYSLOG. > - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0. > - nothing else (EPERM). > > The use-cases were: > - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. > - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the > destructive SYSLOG_ACTION_READs. > > AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't > clear the ring buffer. > > Based on the comments in devkmsg_llseek, it sounds like actions besides > reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), > so we have a strict subset of the non-destructive syslog syscall actions. > > To this end, move the check as Josh had done, but also rename the constants > to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and > SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows > non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions > after a capabilities-constrained SYSLOG_ACTION_OPEN check. > > - /dev/kmsg allows: > - open if CAP_SYSLOG or dmesg_restrict==0 > - reading/polling, after open hm, that changelog is wy down in the weeds and anyone who hasn't been following this with a microscope won't have a clue. I went into an earlier patch and dug out this: : 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. : : Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192 Which is still accurate and relevant, yes? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook keesc...@chromium.org wrote: To fix /dev/kmsg, let's compare the existing interfaces and what they allow: - /proc/kmsg allows: - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive single-reader interface (SYSLOG_ACTION_READ). - everything, after an open. - syslog syscall allows: - anything, if CAP_SYSLOG. - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0. - nothing else (EPERM). The use-cases were: - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the destructive SYSLOG_ACTION_READs. AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't clear the ring buffer. Based on the comments in devkmsg_llseek, it sounds like actions besides reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), so we have a strict subset of the non-destructive syslog syscall actions. To this end, move the check as Josh had done, but also rename the constants to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions after a capabilities-constrained SYSLOG_ACTION_OPEN check. - /dev/kmsg allows: - open if CAP_SYSLOG or dmesg_restrict==0 - reading/polling, after open hm, that changelog is wy down in the weeds and anyone who hasn't been following this with a microscope won't have a clue. I went into an earlier patch and dug out this: : 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. : : Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192 Which is still accurate and relevant, yes? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Wed, May 8, 2013 at 2:22 PM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 30 Apr 2013 10:25:41 -0700 Kees Cook keesc...@chromium.org wrote: To fix /dev/kmsg, let's compare the existing interfaces and what they allow: - /proc/kmsg allows: - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive single-reader interface (SYSLOG_ACTION_READ). - everything, after an open. - syslog syscall allows: - anything, if CAP_SYSLOG. - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0. - nothing else (EPERM). The use-cases were: - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the destructive SYSLOG_ACTION_READs. AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't clear the ring buffer. Based on the comments in devkmsg_llseek, it sounds like actions besides reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), so we have a strict subset of the non-destructive syslog syscall actions. To this end, move the check as Josh had done, but also rename the constants to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions after a capabilities-constrained SYSLOG_ACTION_OPEN check. - /dev/kmsg allows: - open if CAP_SYSLOG or dmesg_restrict==0 - reading/polling, after open hm, that changelog is wy down in the weeds and anyone who hasn't been following this with a microscope won't have a clue. I went into an earlier patch and dug out this: : 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. : : Addresses https://bugzilla.redhat.com/show_bug.cgi?id=903192 Which is still accurate and relevant, yes? Yeah, that'll be fine. I kind of like having the longer rationale in the commit message for future reference (i.e. destructive vs non-destructive, etc), but I'd rather see the code fixed. :) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Wed, 2013-05-08 at 14:26 -0700, Kees Cook wrote: Yeah, that'll be fine. I kind of like having the longer rationale in the commit message for future reference (i.e. destructive vs non-destructive, etc), but I'd rather see the code fixed. :) There's no reason not to have both, is there? -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
>
> - /proc/kmsg allows:
> - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>single-reader interface (SYSLOG_ACTION_READ).
> - everything, after an open.
>
> - syslog syscall allows:
> - anything, if CAP_SYSLOG.
> - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
> - nothing else (EPERM).
>
> The use-cases were:
> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
> destructive SYSLOG_ACTION_READs.
>
> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
> clear the ring buffer.
>
> Based on the comments in devkmsg_llseek, it sounds like actions besides
> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
> so we have a strict subset of the non-destructive syslog syscall actions.
>
> To this end, move the check as Josh had done, but also rename the constants
> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
>
> - /dev/kmsg allows:
> - open if CAP_SYSLOG or dmesg_restrict==0
> - reading/polling, after open
>
> Signed-off-by: Kees Cook
> Reported-by: Christian Kujau
> Cc: Josh Boyer
> Cc: Kay Sievers
> Cc: sta...@vger.kernel.org
OK, this seems to work as expected. Non-privileged users can open
/dev/kmsg for reading and read dmesg if dmesg_restrict is not set. If
set, dmesg(1) will try both /dev/kmsg and the syslog call and get EPERM
back. Running as root still works.
Tested-by: Josh Boyer
> ---
> fs/proc/kmsg.c | 10 +++---
> include/linux/syslog.h |4 +--
> kernel/printk.c| 91
> ++--
> 3 files changed, 57 insertions(+), 48 deletions(-)
>
> diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
> index bd4b5a7..bdfabda 100644
> --- a/fs/proc/kmsg.c
> +++ b/fs/proc/kmsg.c
> @@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
>
> static int kmsg_open(struct inode * inode, struct file * file)
> {
> - return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
> + return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
> }
>
> static int kmsg_release(struct inode * inode, struct file * file)
> {
> - (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
> + (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
> return 0;
> }
>
> @@ -34,15 +34,15 @@ static ssize_t kmsg_read(struct file *file, char __user
> *buf,
>size_t count, loff_t *ppos)
> {
> if ((file->f_flags & O_NONBLOCK) &&
> - !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> + !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
> return -EAGAIN;
> - return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
> + return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
> }
>
> static unsigned int kmsg_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, _wait, wait);
> - if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> + if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
> return POLLIN | POLLRDNORM;
> return 0;
> }
> diff --git a/include/linux/syslog.h b/include/linux/syslog.h
> index 3891139..98a3153 100644
> --- a/include/linux/syslog.h
> +++ b/include/linux/syslog.h
> @@ -44,8 +44,8 @@
> /* Return size of the log buffer */
> #define SYSLOG_ACTION_SIZE_BUFFER 10
>
> -#define SYSLOG_FROM_CALL 0
> -#define SYSLOG_FROM_FILE 1
> +#define SYSLOG_FROM_READER 0
> +#define SYSLOG_FROM_PROC 1
>
> int do_syslog(int type, char __user *buf, int count, bool from_file);
>
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..53b5c5e 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,53 @@ 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
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
- /proc/kmsg allows:
- open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
single-reader interface (SYSLOG_ACTION_READ).
- everything, after an open.
- syslog syscall allows:
- anything, if CAP_SYSLOG.
- SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
- nothing else (EPERM).
The use-cases were:
- dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
- sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
destructive SYSLOG_ACTION_READs.
AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
clear the ring buffer.
Based on the comments in devkmsg_llseek, it sounds like actions besides
reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
so we have a strict subset of the non-destructive syslog syscall actions.
To this end, move the check as Josh had done, but also rename the constants
to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
after a capabilities-constrained SYSLOG_ACTION_OPEN check.
- /dev/kmsg allows:
- open if CAP_SYSLOG or dmesg_restrict==0
- reading/polling, after open
Signed-off-by: Kees Cook keesc...@chromium.org
Reported-by: Christian Kujau li...@nerdbynature.de
Cc: Josh Boyer jwbo...@redhat.com
Cc: Kay Sievers k...@vrfy.org
Cc: sta...@vger.kernel.org
OK, this seems to work as expected. Non-privileged users can open
/dev/kmsg for reading and read dmesg if dmesg_restrict is not set. If
set, dmesg(1) will try both /dev/kmsg and the syslog call and get EPERM
back. Running as root still works.
Tested-by: Josh Boyer jwbo...@redhat.com
---
fs/proc/kmsg.c | 10 +++---
include/linux/syslog.h |4 +--
kernel/printk.c| 91
++--
3 files changed, 57 insertions(+), 48 deletions(-)
diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
index bd4b5a7..bdfabda 100644
--- a/fs/proc/kmsg.c
+++ b/fs/proc/kmsg.c
@@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
static int kmsg_open(struct inode * inode, struct file * file)
{
- return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
+ return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
}
static int kmsg_release(struct inode * inode, struct file * file)
{
- (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
+ (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
return 0;
}
@@ -34,15 +34,15 @@ static ssize_t kmsg_read(struct file *file, char __user
*buf,
size_t count, loff_t *ppos)
{
if ((file-f_flags O_NONBLOCK)
- !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+ !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
return -EAGAIN;
- return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
+ return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
}
static unsigned int kmsg_poll(struct file *file, poll_table *wait)
{
poll_wait(file, log_wait, wait);
- if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+ if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
return POLLIN | POLLRDNORM;
return 0;
}
diff --git a/include/linux/syslog.h b/include/linux/syslog.h
index 3891139..98a3153 100644
--- a/include/linux/syslog.h
+++ b/include/linux/syslog.h
@@ -44,8 +44,8 @@
/* Return size of the log buffer */
#define SYSLOG_ACTION_SIZE_BUFFER 10
-#define SYSLOG_FROM_CALL 0
-#define SYSLOG_FROM_FILE 1
+#define SYSLOG_FROM_READER 0
+#define SYSLOG_FROM_PROC 1
int do_syslog(int type, char __user *buf, int count, bool from_file);
diff --git a/kernel/printk.c b/kernel/printk.c
index abbdd9e..53b5c5e 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -368,6 +368,53 @@ 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.
+
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 11:35 AM, Josh Boyer wrote: > On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote: >> To fix /dev/kmsg, let's compare the existing interfaces and what they allow: >> >> - /proc/kmsg allows: >> - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive >>single-reader interface (SYSLOG_ACTION_READ). >> - everything, after an open. >> >> - syslog syscall allows: >> - anything, if CAP_SYSLOG. >> - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if >> dmesg_restrict==0. >> - nothing else (EPERM). >> >> The use-cases were: >> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. >> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the >> destructive SYSLOG_ACTION_READs. >> >> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't >> clear the ring buffer. >> >> Based on the comments in devkmsg_llseek, it sounds like actions besides >> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), >> so we have a strict subset of the non-destructive syslog syscall actions. >> >> To this end, move the check as Josh had done, but also rename the constants >> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and >> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows >> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions >> after a capabilities-constrained SYSLOG_ACTION_OPEN check. >> >> - /dev/kmsg allows: >> - open if CAP_SYSLOG or dmesg_restrict==0 >> - reading/polling, after open >> >> Signed-off-by: Kees Cook >> Reported-by: Christian Kujau >> Cc: Josh Boyer >> Cc: Kay Sievers >> Cc: sta...@vger.kernel.org > > So staring at this for a while, I think it looks correct. It's > basically the same thing as the v3 I sent out, with the constant rename > and no check in devkmsg_read, right? Yeah, I just explicitly clarified the use-cases and reasoning. :) > I'll try and get it tested here locally tomorrow. Great; thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
> To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
>
> - /proc/kmsg allows:
> - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
>single-reader interface (SYSLOG_ACTION_READ).
> - everything, after an open.
>
> - syslog syscall allows:
> - anything, if CAP_SYSLOG.
> - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
> - nothing else (EPERM).
>
> The use-cases were:
> - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
> - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
> destructive SYSLOG_ACTION_READs.
>
> AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
> clear the ring buffer.
>
> Based on the comments in devkmsg_llseek, it sounds like actions besides
> reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
> so we have a strict subset of the non-destructive syslog syscall actions.
>
> To this end, move the check as Josh had done, but also rename the constants
> to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
> SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
> non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
> after a capabilities-constrained SYSLOG_ACTION_OPEN check.
>
> - /dev/kmsg allows:
> - open if CAP_SYSLOG or dmesg_restrict==0
> - reading/polling, after open
>
> Signed-off-by: Kees Cook
> Reported-by: Christian Kujau
> Cc: Josh Boyer
> Cc: Kay Sievers
> Cc: sta...@vger.kernel.org
So staring at this for a while, I think it looks correct. It's
basically the same thing as the v3 I sent out, with the constant rename
and no check in devkmsg_read, right?
I'll try and get it tested here locally tomorrow.
josh
> ---
> fs/proc/kmsg.c | 10 +++---
> include/linux/syslog.h |4 +--
> kernel/printk.c| 91
> ++--
> 3 files changed, 57 insertions(+), 48 deletions(-)
>
> diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
> index bd4b5a7..bdfabda 100644
> --- a/fs/proc/kmsg.c
> +++ b/fs/proc/kmsg.c
> @@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
>
> static int kmsg_open(struct inode * inode, struct file * file)
> {
> - return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
> + return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
> }
>
> static int kmsg_release(struct inode * inode, struct file * file)
> {
> - (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
> + (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
> return 0;
> }
>
> @@ -34,15 +34,15 @@ static ssize_t kmsg_read(struct file *file, char __user
> *buf,
>size_t count, loff_t *ppos)
> {
> if ((file->f_flags & O_NONBLOCK) &&
> - !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> + !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
> return -EAGAIN;
> - return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
> + return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
> }
>
> static unsigned int kmsg_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, _wait, wait);
> - if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
> + if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
> return POLLIN | POLLRDNORM;
> return 0;
> }
> diff --git a/include/linux/syslog.h b/include/linux/syslog.h
> index 3891139..98a3153 100644
> --- a/include/linux/syslog.h
> +++ b/include/linux/syslog.h
> @@ -44,8 +44,8 @@
> /* Return size of the log buffer */
> #define SYSLOG_ACTION_SIZE_BUFFER 10
>
> -#define SYSLOG_FROM_CALL 0
> -#define SYSLOG_FROM_FILE 1
> +#define SYSLOG_FROM_READER 0
> +#define SYSLOG_FROM_PROC 1
>
> int do_syslog(int type, char __user *buf, int count, bool from_file);
>
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..53b5c5e 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,53 @@ 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
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote:
To fix /dev/kmsg, let's compare the existing interfaces and what they allow:
- /proc/kmsg allows:
- open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive
single-reader interface (SYSLOG_ACTION_READ).
- everything, after an open.
- syslog syscall allows:
- anything, if CAP_SYSLOG.
- SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0.
- nothing else (EPERM).
The use-cases were:
- dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs.
- sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the
destructive SYSLOG_ACTION_READs.
AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't
clear the ring buffer.
Based on the comments in devkmsg_llseek, it sounds like actions besides
reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR),
so we have a strict subset of the non-destructive syslog syscall actions.
To this end, move the check as Josh had done, but also rename the constants
to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and
SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows
non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions
after a capabilities-constrained SYSLOG_ACTION_OPEN check.
- /dev/kmsg allows:
- open if CAP_SYSLOG or dmesg_restrict==0
- reading/polling, after open
Signed-off-by: Kees Cook keesc...@chromium.org
Reported-by: Christian Kujau li...@nerdbynature.de
Cc: Josh Boyer jwbo...@redhat.com
Cc: Kay Sievers k...@vrfy.org
Cc: sta...@vger.kernel.org
So staring at this for a while, I think it looks correct. It's
basically the same thing as the v3 I sent out, with the constant rename
and no check in devkmsg_read, right?
I'll try and get it tested here locally tomorrow.
josh
---
fs/proc/kmsg.c | 10 +++---
include/linux/syslog.h |4 +--
kernel/printk.c| 91
++--
3 files changed, 57 insertions(+), 48 deletions(-)
diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
index bd4b5a7..bdfabda 100644
--- a/fs/proc/kmsg.c
+++ b/fs/proc/kmsg.c
@@ -21,12 +21,12 @@ extern wait_queue_head_t log_wait;
static int kmsg_open(struct inode * inode, struct file * file)
{
- return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_FILE);
+ return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, SYSLOG_FROM_PROC);
}
static int kmsg_release(struct inode * inode, struct file * file)
{
- (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_FILE);
+ (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, SYSLOG_FROM_PROC);
return 0;
}
@@ -34,15 +34,15 @@ static ssize_t kmsg_read(struct file *file, char __user
*buf,
size_t count, loff_t *ppos)
{
if ((file-f_flags O_NONBLOCK)
- !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+ !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
return -EAGAIN;
- return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_FILE);
+ return do_syslog(SYSLOG_ACTION_READ, buf, count, SYSLOG_FROM_PROC);
}
static unsigned int kmsg_poll(struct file *file, poll_table *wait)
{
poll_wait(file, log_wait, wait);
- if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_FILE))
+ if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, SYSLOG_FROM_PROC))
return POLLIN | POLLRDNORM;
return 0;
}
diff --git a/include/linux/syslog.h b/include/linux/syslog.h
index 3891139..98a3153 100644
--- a/include/linux/syslog.h
+++ b/include/linux/syslog.h
@@ -44,8 +44,8 @@
/* Return size of the log buffer */
#define SYSLOG_ACTION_SIZE_BUFFER 10
-#define SYSLOG_FROM_CALL 0
-#define SYSLOG_FROM_FILE 1
+#define SYSLOG_FROM_READER 0
+#define SYSLOG_FROM_PROC 1
int do_syslog(int type, char __user *buf, int count, bool from_file);
diff --git a/kernel/printk.c b/kernel/printk.c
index abbdd9e..53b5c5e 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -368,6 +368,53 @@ 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)
+
Re: [PATCH] kmsg: honor dmesg_restrict sysctl on /dev/kmsg
On Tue, Apr 30, 2013 at 11:35 AM, Josh Boyer jwbo...@redhat.com wrote: On Tue, Apr 30, 2013 at 10:25:41AM -0700, Kees Cook wrote: To fix /dev/kmsg, let's compare the existing interfaces and what they allow: - /proc/kmsg allows: - open (SYSLOG_ACTION_OPEN) if CAP_SYSLOG since it uses a destructive single-reader interface (SYSLOG_ACTION_READ). - everything, after an open. - syslog syscall allows: - anything, if CAP_SYSLOG. - SYSLOG_ACTION_READ_ALL and SYSLOG_ACTION_SIZE_BUFFER, if dmesg_restrict==0. - nothing else (EPERM). The use-cases were: - dmesg(1) needs to do non-destructive SYSLOG_ACTION_READ_ALLs. - sysklog(1) needs to open /proc/kmsg, drop privs, and still issue the destructive SYSLOG_ACTION_READs. AIUI, dmesg(1) is moving to /dev/kmsg, and systemd-journald doesn't clear the ring buffer. Based on the comments in devkmsg_llseek, it sounds like actions besides reading aren't going to be supported by /dev/kmsg (i.e. SYSLOG_ACTION_CLEAR), so we have a strict subset of the non-destructive syslog syscall actions. To this end, move the check as Josh had done, but also rename the constants to reflect their new uses (SYSLOG_FROM_CALL becomes SYSLOG_FROM_READER, and SYSLOG_FROM_FILE becomes SYSLOG_FROM_PROC). SYSLOG_FROM_READER allows non-destructive actions, and SYSLOG_FROM_PROC allows destructive actions after a capabilities-constrained SYSLOG_ACTION_OPEN check. - /dev/kmsg allows: - open if CAP_SYSLOG or dmesg_restrict==0 - reading/polling, after open Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Christian Kujau li...@nerdbynature.de Cc: Josh Boyer jwbo...@redhat.com Cc: Kay Sievers k...@vrfy.org Cc: sta...@vger.kernel.org So staring at this for a while, I think it looks correct. It's basically the same thing as the v3 I sent out, with the constant rename and no check in devkmsg_read, right? Yeah, I just explicitly clarified the use-cases and reasoning. :) I'll try and get it tested here locally tomorrow. Great; thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 wrote: > > - Original Message - > >> From: "Kees Cook" > >> To: "Josh Boyer" > >> Cc: "Andrew Morton" , "Eric Paris" > >> , "Linus Torvalds" > >> , "Christian Kujau" > >> , "# 3.4.x" , > >> "LKML" > >> 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer wrote: > - Original Message - >> From: "Kees Cook" >> To: "Josh Boyer" >> Cc: "Andrew Morton" , "Eric Paris" >> , "Linus Torvalds" >> , "Christian Kujau" , >> "# 3.4.x" , >> "LKML" >> 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer jwbo...@redhat.com wrote: - Original Message - From: Kees Cook keesc...@chromium.org To: Josh Boyer jwbo...@redhat.com Cc: Andrew Morton a...@linux-foundation.org, Eric Paris epa...@redhat.com, Linus Torvalds torva...@linux-foundation.org, Christian Kujau li...@nerdbynature.de, # 3.4.x sta...@vger.kernel.org, LKML linux-kernel@vger.kernel.org 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 jwbo...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 jwbo...@redhat.com wrote: - Original Message - From: Kees Cook keesc...@chromium.org To: Josh Boyer jwbo...@redhat.com Cc: Andrew Morton a...@linux-foundation.org, Eric Paris epa...@redhat.com, Linus Torvalds torva...@linux-foundation.org, Christian Kujau li...@nerdbynature.de, # 3.4.x sta...@vger.kernel.org, LKML linux-kernel@vger.kernel.org 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 jwbo...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
- Original Message - > From: "Kees Cook" > To: "Josh Boyer" > Cc: "Andrew Morton" , "Eric Paris" > , "Linus Torvalds" > , "Christian Kujau" , > "# 3.4.x" , > "LKML" > 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer jwbo...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
- Original Message - From: Kees Cook keesc...@chromium.org To: Josh Boyer jwbo...@redhat.com Cc: Andrew Morton a...@linux-foundation.org, Eric Paris epa...@redhat.com, Linus Torvalds torva...@linux-foundation.org, Christian Kujau li...@nerdbynature.de, # 3.4.x sta...@vger.kernel.org, LKML linux-kernel@vger.kernel.org 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 jwbo...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
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
Reported-by: Christian Kujau
Cc: Eric Paris
Cc: James Morris
Cc:
Signed-off-by: Andrew Morton
---
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;
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 jwbo...@redhat.com 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 jwbo...@redhat.com Reported-by: Christian Kujau li...@nerdbynature.de Cc: Eric Paris epa...@redhat.com Cc: James Morris jmor...@namei.org Cc: sta...@vger.kernel.org Signed-off-by: Andrew Morton a...@linux-foundation.org --- 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; _ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 2:19 PM, Josh Boyer wrote:
> On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
>> Fine Fine, I'll get off my lazy butt and look at this.
>
> Shock!
>
>> Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg
>> and the syscall both use do_syslog() which calls
>> check_syslog_permissions() and security_syslog(). /dev/kmsg only calls
>> security_syslog(), which we all agree needs fixed.
>>
>> > > Also, the LSM hooks aren't doing any capability checks at all that I can
>> > > see, which may or may not be a bug in and of itself but I have no idea.
>> > > I was hoping Eric would speak up about that.
>>
>> I wouldn't call it a bug. But it sure is a pretty shitty design pattern
>> to have security_* sometimes the right thing to do and sometimes
>> capable() is the right thing to do. It is pervasive in the kernel that
>> you have either/or, but I can't think of anywhere that functions are
>> expected to do BOTH. So yeah, that needs fixed.
>
> OK.
>
>>
>> > Eric explicitly removed the cap check since it was cluttering things
>> > the way it was originally written. I do think security_syslog() should
>> > pass through check_syslog_permissions(), though. Then this wouldn't
>> > have happened. That might actually be the right way to clean this up,
>> > but I'd like to see Eric's thoughts first.
>>
>> How about something like this?
>
> I think this looks pretty good. Much clearer overall and the
> consolidation is nice. I'll try to get it tested soon.
>
> josh
>
>>
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 7c69b3e..ced2cac 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -626,7 +626,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);
Yes, this looks correct with the consolidation below. Nice!
>> if (err)
>> return err;
>>
>> @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool
>> from_file)
>>* already done the capabilities checks at open time.
>>*/
>> if (from_file && type != SYSLOG_ACTION_OPEN)
>> - return 0;
>> + goto ok;
>>
>> if (syslog_action_restricted(type)) {
>> if (capable(CAP_SYSLOG))
>> - return 0;
>> + 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));
>> - return 0;
>> + goto ok;
>> }
>> return -EPERM;
>> }
>> - return 0;
>> +ok:
>> + return security_syslog(type);
>> }
>>
>> #if defined(CONFIG_PRINTK_TIME)
>> @@ -1133,10 +1134,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;
>>
>>
I think for completeness, we need to add a
check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE)
call to devkmsg_read().
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
> Fine Fine, I'll get off my lazy butt and look at this.
Shock!
> Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg
> and the syscall both use do_syslog() which calls
> check_syslog_permissions() and security_syslog(). /dev/kmsg only calls
> security_syslog(), which we all agree needs fixed.
>
> > > Also, the LSM hooks aren't doing any capability checks at all that I can
> > > see, which may or may not be a bug in and of itself but I have no idea.
> > > I was hoping Eric would speak up about that.
>
> I wouldn't call it a bug. But it sure is a pretty shitty design pattern
> to have security_* sometimes the right thing to do and sometimes
> capable() is the right thing to do. It is pervasive in the kernel that
> you have either/or, but I can't think of anywhere that functions are
> expected to do BOTH. So yeah, that needs fixed.
OK.
>
> > Eric explicitly removed the cap check since it was cluttering things
> > the way it was originally written. I do think security_syslog() should
> > pass through check_syslog_permissions(), though. Then this wouldn't
> > have happened. That might actually be the right way to clean this up,
> > but I'd like to see Eric's thoughts first.
>
> How about something like this?
I think this looks pretty good. Much clearer overall and the
consolidation is nice. I'll try to get it tested soon.
josh
>
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 7c69b3e..ced2cac 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -626,7 +626,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;
>
> @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool
> from_file)
>* already done the capabilities checks at open time.
>*/
> if (from_file && type != SYSLOG_ACTION_OPEN)
> - return 0;
> + goto ok;
>
> if (syslog_action_restricted(type)) {
> if (capable(CAP_SYSLOG))
> - return 0;
> + 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));
> - return 0;
> + goto ok;
> }
> return -EPERM;
> }
> - return 0;
> +ok:
> + return security_syslog(type);
> }
>
> #if defined(CONFIG_PRINTK_TIME)
> @@ -1133,10 +1134,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;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
Fine Fine, I'll get off my lazy butt and look at this. On Wed, 2013-02-27 at 10:14 -0800, Kees Cook wrote: > On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer wrote: > > On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote: > >> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: > >> > 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. Not sure this is correct. There was no devkmsg_open() in commit 12b3052c3ee. That was added in e11fea92e. Looks like before that commit the devkmsg code was even worse. It didn't call security_syslog or capable(). Uggh. > 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 > >> > > >> > Reported-by: Christian Kujau > >> > CC: sta...@vger.kernel.org > >> > Signed-off-by: Josh Boyer > >> > --- > >> > kernel/printk.c | 3 +++ > >> > 1 file changed, 3 insertions(+) > >> > > >> > diff --git a/kernel/printk.c b/kernel/printk.c > >> > index f24633a..398ef9a 100644 > >> > --- a/kernel/printk.c > >> > +++ b/kernel/printk.c > >> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct > >> > file *file) > >> > struct devkmsg_user *user; > >> > int err; > >> > > >> > + if (dmesg_restrict && !capable(CAP_SYSLOG)) > >> > + return -EACCES; > >> > + > >> > >> I think this should use check_syslog_permissions() instead, as done for > >> /proc/kmsg and the syslog syscall. > >> > >> err = check_syslog_permissions(SYSLOG_ACTION_OPTION, > >> SYSLOG_FROM_FILE); > > > > Did you mean SYSLOG_ACTION_OPEN? > > Oops, yes, typo. > > > I didn't code it that way because the comment in that function about the > > capability checks already being done seem pretty off to me. I could > > have just misread the /proc code though. I can resend with the change > > you suggest if everyone thinks that's a better way. > > Yeah, the comment is meaningful if you examine how /proc/kmsg works, > which was basically as a wrapper to the syslog syscall. The issue was > that we had to catch (and potentially block) the "open" action on > /proc/kmsg vs blocking the the syslog read action. In this case, we've > got another file-based interface, so we should use OPEN and FROM_FILE > to block the open. (Though it could be argued that we only want to > block the open if it's reading, which is exactly what Kay was trying > to do originally it looks like.) Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg and the syscall both use do_syslog() which calls check_syslog_permissions() and security_syslog(). /dev/kmsg only calls security_syslog(), which we all agree needs fixed. > > Also, the LSM hooks aren't doing any capability checks at all that I can > > see, which may or may not be a bug in and of itself but I have no idea. > > I was hoping Eric would speak up about that. I wouldn't call it a bug. But it sure is a pretty shitty design pattern to have security_* sometimes the right thing to do and sometimes capable() is the right thing to do. It is pervasive in the kernel that you have either/or, but I can't think of anywhere that functions are expected to do BOTH. So yeah, that needs fixed. > Eric explicitly removed the cap check since it was cluttering things > the way it was originally written. I do think security_syslog() should > pass through check_syslog_permissions(), though. Then this wouldn't > have happened. That might actually be the right way to clean this up, > but I'd like to see Eric's thoughts first. How about something like this? diff --git a/kernel/printk.c b/kernel/printk.c index 7c69b3e..ced2cac 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -626,7 +626,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; @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool from_file) * already done the capabilities checks at open time. */ if (from_file && type != SYSLOG_ACTION_OPEN) -
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer wrote: > On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote: >> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: >> > 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 >> > >> > Reported-by: Christian Kujau >> > CC: sta...@vger.kernel.org >> > Signed-off-by: Josh Boyer >> > --- >> > kernel/printk.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/kernel/printk.c b/kernel/printk.c >> > index f24633a..398ef9a 100644 >> > --- a/kernel/printk.c >> > +++ b/kernel/printk.c >> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct >> > file *file) >> > struct devkmsg_user *user; >> > int err; >> > >> > + if (dmesg_restrict && !capable(CAP_SYSLOG)) >> > + return -EACCES; >> > + >> >> I think this should use check_syslog_permissions() instead, as done for >> /proc/kmsg and the syslog syscall. >> >> err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); > > Did you mean SYSLOG_ACTION_OPEN? Oops, yes, typo. > I didn't code it that way because the comment in that function about the > capability checks already being done seem pretty off to me. I could > have just misread the /proc code though. I can resend with the change > you suggest if everyone thinks that's a better way. Yeah, the comment is meaningful if you examine how /proc/kmsg works, which was basically as a wrapper to the syslog syscall. The issue was that we had to catch (and potentially block) the "open" action on /proc/kmsg vs blocking the the syslog read action. In this case, we've got another file-based interface, so we should use OPEN and FROM_FILE to block the open. (Though it could be argued that we only want to block the open if it's reading, which is exactly what Kay was trying to do originally it looks like.) > Also, the LSM hooks aren't doing any capability checks at all that I can > see, which may or may not be a bug in and of itself but I have no idea. > I was hoping Eric would speak up about that. Eric explicitly removed the cap check since it was cluttering things the way it was originally written. I do think security_syslog() should pass through check_syslog_permissions(), though. Then this wouldn't have happened. That might actually be the right way to clean this up, but I'd like to see Eric's thoughts first. >> if (err) >> return err; >> >> And going forward we should probably think about dropping the CAP_SYS_ADMIN >> backward-compat code in check_syslog_permissions. > > Sure, but that's a separate commit. Yup. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 10:05:47AM -0800, Kees Cook wrote: > Hi, > > On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: > > 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. > > Actually, are the security_syslog() checks in /dev/kmsg correct? There is > only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't > it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to > devkmsg_read? (And should we add one for write?) You ask wonderful questions! I have no idea. Either way, it needs to still somehow check for capable(CAP_SYSLOG) which security_syslog doesn't do. So either what I have already, or your suggestion of using the existing function with SYSLOG_ACTION_OPEN in devkmsg_open. josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
Hi, On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: > 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. Actually, are the security_syslog() checks in /dev/kmsg correct? There is only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to devkmsg_read? (And should we add one for write?) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote: > On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: > > 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 > > > > Reported-by: Christian Kujau > > CC: sta...@vger.kernel.org > > Signed-off-by: Josh Boyer > > --- > > kernel/printk.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/kernel/printk.c b/kernel/printk.c > > index f24633a..398ef9a 100644 > > --- a/kernel/printk.c > > +++ b/kernel/printk.c > > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct > > file *file) > > struct devkmsg_user *user; > > int err; > > > > + if (dmesg_restrict && !capable(CAP_SYSLOG)) > > + return -EACCES; > > + > > I think this should use check_syslog_permissions() instead, as done for > /proc/kmsg and the syslog syscall. > > err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); Did you mean SYSLOG_ACTION_OPEN? I didn't code it that way because the comment in that function about the capability checks already being done seem pretty off to me. I could have just misread the /proc code though. I can resend with the change you suggest if everyone thinks that's a better way. Also, the LSM hooks aren't doing any capability checks at all that I can see, which may or may not be a bug in and of itself but I have no idea. I was hoping Eric would speak up about that. > if (err) > return err; > > And going forward we should probably think about dropping the CAP_SYS_ADMIN > backward-compat code in check_syslog_permissions. Sure, but that's a separate commit. josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: > 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 > > Reported-by: Christian Kujau > CC: sta...@vger.kernel.org > Signed-off-by: Josh Boyer > --- > kernel/printk.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/printk.c b/kernel/printk.c > index f24633a..398ef9a 100644 > --- a/kernel/printk.c > +++ b/kernel/printk.c > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file > *file) > struct devkmsg_user *user; > int err; > > + if (dmesg_restrict && !capable(CAP_SYSLOG)) > + return -EACCES; > + I think this should use check_syslog_permissions() instead, as done for /proc/kmsg and the syslog syscall. err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); if (err) return err; And going forward we should probably think about dropping the CAP_SYS_ADMIN backward-compat code in check_syslog_permissions. > /* write-only does not need any file context */ > if ((file->f_flags & O_ACCMODE) == O_WRONLY) > return 0; -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: 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 Reported-by: Christian Kujau li...@nerdbynature.de CC: sta...@vger.kernel.org Signed-off-by: Josh Boyer jwbo...@redhat.com --- kernel/printk.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/printk.c b/kernel/printk.c index f24633a..398ef9a 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file) struct devkmsg_user *user; int err; + if (dmesg_restrict !capable(CAP_SYSLOG)) + return -EACCES; + I think this should use check_syslog_permissions() instead, as done for /proc/kmsg and the syslog syscall. err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); if (err) return err; And going forward we should probably think about dropping the CAP_SYS_ADMIN backward-compat code in check_syslog_permissions. /* write-only does not need any file context */ if ((file-f_flags O_ACCMODE) == O_WRONLY) return 0; -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote: On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: 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 Reported-by: Christian Kujau li...@nerdbynature.de CC: sta...@vger.kernel.org Signed-off-by: Josh Boyer jwbo...@redhat.com --- kernel/printk.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/printk.c b/kernel/printk.c index f24633a..398ef9a 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file) struct devkmsg_user *user; int err; + if (dmesg_restrict !capable(CAP_SYSLOG)) + return -EACCES; + I think this should use check_syslog_permissions() instead, as done for /proc/kmsg and the syslog syscall. err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); Did you mean SYSLOG_ACTION_OPEN? I didn't code it that way because the comment in that function about the capability checks already being done seem pretty off to me. I could have just misread the /proc code though. I can resend with the change you suggest if everyone thinks that's a better way. Also, the LSM hooks aren't doing any capability checks at all that I can see, which may or may not be a bug in and of itself but I have no idea. I was hoping Eric would speak up about that. if (err) return err; And going forward we should probably think about dropping the CAP_SYS_ADMIN backward-compat code in check_syslog_permissions. Sure, but that's a separate commit. josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
Hi, On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: 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. Actually, are the security_syslog() checks in /dev/kmsg correct? There is only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to devkmsg_read? (And should we add one for write?) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 10:05:47AM -0800, Kees Cook wrote: Hi, On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: 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. Actually, are the security_syslog() checks in /dev/kmsg correct? There is only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to devkmsg_read? (And should we add one for write?) You ask wonderful questions! I have no idea. Either way, it needs to still somehow check for capable(CAP_SYSLOG) which security_syslog doesn't do. So either what I have already, or your suggestion of using the existing function with SYSLOG_ACTION_OPEN in devkmsg_open. josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer jwbo...@redhat.com wrote: On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote: On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote: 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 Reported-by: Christian Kujau li...@nerdbynature.de CC: sta...@vger.kernel.org Signed-off-by: Josh Boyer jwbo...@redhat.com --- kernel/printk.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/printk.c b/kernel/printk.c index f24633a..398ef9a 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file) struct devkmsg_user *user; int err; + if (dmesg_restrict !capable(CAP_SYSLOG)) + return -EACCES; + I think this should use check_syslog_permissions() instead, as done for /proc/kmsg and the syslog syscall. err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE); Did you mean SYSLOG_ACTION_OPEN? Oops, yes, typo. I didn't code it that way because the comment in that function about the capability checks already being done seem pretty off to me. I could have just misread the /proc code though. I can resend with the change you suggest if everyone thinks that's a better way. Yeah, the comment is meaningful if you examine how /proc/kmsg works, which was basically as a wrapper to the syslog syscall. The issue was that we had to catch (and potentially block) the open action on /proc/kmsg vs blocking the the syslog read action. In this case, we've got another file-based interface, so we should use OPEN and FROM_FILE to block the open. (Though it could be argued that we only want to block the open if it's reading, which is exactly what Kay was trying to do originally it looks like.) Also, the LSM hooks aren't doing any capability checks at all that I can see, which may or may not be a bug in and of itself but I have no idea. I was hoping Eric would speak up about that. Eric explicitly removed the cap check since it was cluttering things the way it was originally written. I do think security_syslog() should pass through check_syslog_permissions(), though. Then this wouldn't have happened. That might actually be the right way to clean this up, but I'd like to see Eric's thoughts first. if (err) return err; And going forward we should probably think about dropping the CAP_SYS_ADMIN backward-compat code in check_syslog_permissions. Sure, but that's a separate commit. Yup. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
Fine Fine, I'll get off my lazy butt and look at this.
On Wed, 2013-02-27 at 10:14 -0800, Kees Cook wrote:
On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer jwbo...@redhat.com wrote:
On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
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.
Not sure this is correct. There was no devkmsg_open() in commit
12b3052c3ee. That was added in e11fea92e. Looks like before that
commit the devkmsg code was even worse. It didn't call security_syslog
or capable(). Uggh.
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
Reported-by: Christian Kujau li...@nerdbynature.de
CC: sta...@vger.kernel.org
Signed-off-by: Josh Boyer jwbo...@redhat.com
---
kernel/printk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/printk.c b/kernel/printk.c
index f24633a..398ef9a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct
file *file)
struct devkmsg_user *user;
int err;
+ if (dmesg_restrict !capable(CAP_SYSLOG))
+ return -EACCES;
+
I think this should use check_syslog_permissions() instead, as done for
/proc/kmsg and the syslog syscall.
err = check_syslog_permissions(SYSLOG_ACTION_OPTION,
SYSLOG_FROM_FILE);
Did you mean SYSLOG_ACTION_OPEN?
Oops, yes, typo.
I didn't code it that way because the comment in that function about the
capability checks already being done seem pretty off to me. I could
have just misread the /proc code though. I can resend with the change
you suggest if everyone thinks that's a better way.
Yeah, the comment is meaningful if you examine how /proc/kmsg works,
which was basically as a wrapper to the syslog syscall. The issue was
that we had to catch (and potentially block) the open action on
/proc/kmsg vs blocking the the syslog read action. In this case, we've
got another file-based interface, so we should use OPEN and FROM_FILE
to block the open. (Though it could be argued that we only want to
block the open if it's reading, which is exactly what Kay was trying
to do originally it looks like.)
Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg
and the syscall both use do_syslog() which calls
check_syslog_permissions() and security_syslog(). /dev/kmsg only calls
security_syslog(), which we all agree needs fixed.
Also, the LSM hooks aren't doing any capability checks at all that I can
see, which may or may not be a bug in and of itself but I have no idea.
I was hoping Eric would speak up about that.
I wouldn't call it a bug. But it sure is a pretty shitty design pattern
to have security_* sometimes the right thing to do and sometimes
capable() is the right thing to do. It is pervasive in the kernel that
you have either/or, but I can't think of anywhere that functions are
expected to do BOTH. So yeah, that needs fixed.
Eric explicitly removed the cap check since it was cluttering things
the way it was originally written. I do think security_syslog() should
pass through check_syslog_permissions(), though. Then this wouldn't
have happened. That might actually be the right way to clean this up,
but I'd like to see Eric's thoughts first.
How about something like this?
diff --git a/kernel/printk.c b/kernel/printk.c
index 7c69b3e..ced2cac 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -626,7 +626,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;
@@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool
from_file)
* already done the capabilities checks at open time.
*/
if (from_file type != SYSLOG_ACTION_OPEN)
- return 0;
+ goto ok;
if (syslog_action_restricted(type)) {
if (capable(CAP_SYSLOG))
-
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
Fine Fine, I'll get off my lazy butt and look at this.
Shock!
Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg
and the syscall both use do_syslog() which calls
check_syslog_permissions() and security_syslog(). /dev/kmsg only calls
security_syslog(), which we all agree needs fixed.
Also, the LSM hooks aren't doing any capability checks at all that I can
see, which may or may not be a bug in and of itself but I have no idea.
I was hoping Eric would speak up about that.
I wouldn't call it a bug. But it sure is a pretty shitty design pattern
to have security_* sometimes the right thing to do and sometimes
capable() is the right thing to do. It is pervasive in the kernel that
you have either/or, but I can't think of anywhere that functions are
expected to do BOTH. So yeah, that needs fixed.
OK.
Eric explicitly removed the cap check since it was cluttering things
the way it was originally written. I do think security_syslog() should
pass through check_syslog_permissions(), though. Then this wouldn't
have happened. That might actually be the right way to clean this up,
but I'd like to see Eric's thoughts first.
How about something like this?
I think this looks pretty good. Much clearer overall and the
consolidation is nice. I'll try to get it tested soon.
josh
diff --git a/kernel/printk.c b/kernel/printk.c
index 7c69b3e..ced2cac 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -626,7 +626,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;
@@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool
from_file)
* already done the capabilities checks at open time.
*/
if (from_file type != SYSLOG_ACTION_OPEN)
- return 0;
+ goto ok;
if (syslog_action_restricted(type)) {
if (capable(CAP_SYSLOG))
- return 0;
+ 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));
- return 0;
+ goto ok;
}
return -EPERM;
}
- return 0;
+ok:
+ return security_syslog(type);
}
#if defined(CONFIG_PRINTK_TIME)
@@ -1133,10 +1134,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;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
On Wed, Feb 27, 2013 at 2:19 PM, Josh Boyer jwbo...@redhat.com wrote:
On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
Fine Fine, I'll get off my lazy butt and look at this.
Shock!
Right. Now we have /proc/kmsg, /dev/kmsg, and the syscall. /proc/kmsg
and the syscall both use do_syslog() which calls
check_syslog_permissions() and security_syslog(). /dev/kmsg only calls
security_syslog(), which we all agree needs fixed.
Also, the LSM hooks aren't doing any capability checks at all that I can
see, which may or may not be a bug in and of itself but I have no idea.
I was hoping Eric would speak up about that.
I wouldn't call it a bug. But it sure is a pretty shitty design pattern
to have security_* sometimes the right thing to do and sometimes
capable() is the right thing to do. It is pervasive in the kernel that
you have either/or, but I can't think of anywhere that functions are
expected to do BOTH. So yeah, that needs fixed.
OK.
Eric explicitly removed the cap check since it was cluttering things
the way it was originally written. I do think security_syslog() should
pass through check_syslog_permissions(), though. Then this wouldn't
have happened. That might actually be the right way to clean this up,
but I'd like to see Eric's thoughts first.
How about something like this?
I think this looks pretty good. Much clearer overall and the
consolidation is nice. I'll try to get it tested soon.
josh
diff --git a/kernel/printk.c b/kernel/printk.c
index 7c69b3e..ced2cac 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -626,7 +626,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);
Yes, this looks correct with the consolidation below. Nice!
if (err)
return err;
@@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool
from_file)
* already done the capabilities checks at open time.
*/
if (from_file type != SYSLOG_ACTION_OPEN)
- return 0;
+ goto ok;
if (syslog_action_restricted(type)) {
if (capable(CAP_SYSLOG))
- return 0;
+ 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));
- return 0;
+ goto ok;
}
return -EPERM;
}
- return 0;
+ok:
+ return security_syslog(type);
}
#if defined(CONFIG_PRINTK_TIME)
@@ -1133,10 +1134,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;
I think for completeness, we need to add a
check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE)
call to devkmsg_read().
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
