[PATCH 0/2] pstore: Make default pstorefs root dir perms 0750

[PATCH 0/2] pstore: Make default pstorefs root dir perms 0750

[Kees Cook]

Nick Kralevich pointed out that it was rather problematic to check
capabilities when reading some pstore files. Instead, opt for a more
configurable DAC approach, but retain the general protection by making the
pstorefs root directory mode 0750. It was 0755, but most crash-handlers
will also be performing unlink operations (which DAC would require a
root uid perm for already), so this shouldn't affect anyone, but rather
make permissions more flexible.

-Kees

[PATCH 1/2] pstore: Make default pstorefs root dir perms 0750

[Kees Cook]

Currently only DMESG and CONSOLE record types are protected, and it isn't
obvious that they are using a capability check. Instead switch to explicit
root directory mode of 0750 to keep files private by default.  This will
allow the removal of the capability check, which was non-obvious and
forces a process to have possibly too much privilege when simple post-boot
chgrp for readers would be possible without it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index fefd22611cf6..f1e88b695090 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -471,7 +471,7 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 
 	inode = pstore_get_inode(sb);
 	if (inode) {
-		inode->i_mode = S_IFDIR | 0755;
+		inode->i_mode = S_IFDIR | 0750;
 		inode->i_op = &pstore_dir_inode_operations;
 		inode->i_fop = &simple_dir_operations;
 		inc_nlink(inode);
-- 
2.7.4

Re: [PATCH 1/2] pstore: Make default pstorefs root dir perms 0750

[Sergey Senozhatsky]

Hello,

On (08/10/17 13:36), Kees Cook wrote:
> Currently only DMESG and CONSOLE record types are protected, and it isn't
> obvious that they are using a capability check. Instead switch to explicit
> root directory mode of 0750 to keep files private by default.  This will
> allow the removal of the capability check, which was non-obvious and
> forces a process to have possibly too much privilege

it's CAP_SYSLOG, isn't it? is it too much? does chgrp require less privileges?

	-ss

> when simple post-boot chgrp for readers would be possible without it.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index fefd22611cf6..f1e88b695090 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -471,7 +471,7 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	inode = pstore_get_inode(sb);
>  	if (inode) {
> -		inode->i_mode = S_IFDIR | 0755;
> +		inode->i_mode = S_IFDIR | 0750;
>  		inode->i_op = &pstore_dir_inode_operations;
>  		inode->i_fop = &simple_dir_operations;
>  		inc_nlink(inode);
> -- 
> 2.7.4
> 

[PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Kees Cook]

This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
various conflict clean-ups.

With the default root directory mode set to 0750 now, the capability
check was redundant.

Suggested-by: Nick Kralevich <nnk@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c      | 22 ----------------------
 include/linux/syslog.h |  9 ---------
 kernel/printk/printk.c |  3 +--
 3 files changed, 1 insertion(+), 33 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index f1e88b695090..d814723fb27d 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -36,7 +36,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
-#include <linux/syslog.h>
 
 #include "internal.h"
 
@@ -132,18 +131,6 @@ static const struct seq_operations pstore_ftrace_seq_ops = {
 	.show	= pstore_ftrace_seq_show,
 };
 
-static int pstore_check_syslog_permissions(struct pstore_private *ps)
-{
-	switch (ps->record->type) {
-	case PSTORE_TYPE_DMESG:
-	case PSTORE_TYPE_CONSOLE:
-		return check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
-			SYSLOG_FROM_READER);
-	default:
-		return 0;
-	}
-}
-
 static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
 						size_t count, loff_t *ppos)
 {
@@ -163,10 +150,6 @@ static int pstore_file_open(struct inode *inode, struct file *file)
 	int err;
 	const struct seq_operations *sops = NULL;
 
-	err = pstore_check_syslog_permissions(ps);
-	if (err)
-		return err;
-
 	if (ps->record->type == PSTORE_TYPE_FTRACE)
 		sops = &pstore_ftrace_seq_ops;
 
@@ -204,11 +187,6 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct pstore_private *p = d_inode(dentry)->i_private;
 	struct pstore_record *record = p->record;
-	int err;
-
-	err = pstore_check_syslog_permissions(p);
-	if (err)
-		return err;
 
 	if (!record->psi->erase)
 		return -EPERM;
diff --git a/include/linux/syslog.h b/include/linux/syslog.h
index c3a7f0cc3a27..e1c3632f4e81 100644
--- a/include/linux/syslog.h
+++ b/include/linux/syslog.h
@@ -49,13 +49,4 @@
 
 int do_syslog(int type, char __user *buf, int count, int source);
 
-#ifdef CONFIG_PRINTK
-int check_syslog_permissions(int type, int source);
-#else
-static inline int check_syslog_permissions(int type, int source)
-{
-	return 0;
-}
-#endif
-
 #endif /* _LINUX_SYSLOG_H */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863f629c..97bda7b0655b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -649,7 +649,7 @@ static int syslog_action_restricted(int type)
 	       type != SYSLOG_ACTION_SIZE_BUFFER;
 }
 
-int check_syslog_permissions(int type, int source)
+static int check_syslog_permissions(int type, int source)
 {
 	/*
 	 * If this is from /proc/kmsg and we've already opened it, then we've
@@ -677,7 +677,6 @@ int check_syslog_permissions(int type, int source)
 ok:
 	return security_syslog(type);
 }
-EXPORT_SYMBOL_GPL(check_syslog_permissions);
 
 static void append_char(char **pp, char *e, char c)
 {
-- 
2.7.4

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Petr Mladek]

On Thu 2017-08-10 13:36:35, Kees Cook wrote:
> This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
> various conflict clean-ups.
> 
> With the default root directory mode set to 0750 now, the capability
> check was redundant.
> 
> Suggested-by: Nick Kralevich <nnk@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>

The patch looks correct from the code side. I looked at
it because it touched printk.c. Note that I am not
security expert.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

Best Regards,
Petr

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Kees Cook]

On Tue, Aug 15, 2017 at 4:55 AM, Petr Mladek <pmladek@suse.com> wrote:
> On Thu 2017-08-10 13:36:35, Kees Cook wrote:
>> This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
>> various conflict clean-ups.
>>
>> With the default root directory mode set to 0750 now, the capability
>> check was redundant.
>>
>> Suggested-by: Nick Kralevich <nnk@google.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> The patch looks correct from the code side. I looked at
> it because it touched printk.c. Note that I am not
> security expert.
>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Steven Rostedt]

On Thu, 10 Aug 2017 13:36:35 -0700
Kees Cook <keescook@chromium.org> wrote:

> This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
> various conflict clean-ups.
> 
> With the default root directory mode set to 0750 now, the capability
> check was redundant.

What's wrong with redundancy?

-- Steve


> 
> Suggested-by: Nick Kralevich <nnk@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Kees Cook]

On Tue, Aug 15, 2017 at 5:21 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 10 Aug 2017 13:36:35 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
>> various conflict clean-ups.
>>
>> With the default root directory mode set to 0750 now, the capability
>> check was redundant.
>
> What's wrong with redundancy?

In this case, it actually _blocks_ system builders from being able to
define the access controls on pstore. :(

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Steven Rostedt]

On Tue, 15 Aug 2017 17:29:38 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Tue, Aug 15, 2017 at 5:21 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 10 Aug 2017 13:36:35 -0700
> > Kees Cook <keescook@chromium.org> wrote:
> >  
> >> This reverts commit 68c4a4f8abc60c9440ede9cd123d48b78325f7a3, with
> >> various conflict clean-ups.
> >>
> >> With the default root directory mode set to 0750 now, the capability
> >> check was redundant.  
> >
> > What's wrong with redundancy?  
> 
> In this case, it actually _blocks_ system builders from being able to
> define the access controls on pstore. :(

Then that should be stated in the change log, as it is the real reason
to revert, not just the fact that it is redundant.

Thanks,

-- Steve

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Sergey Senozhatsky]

On (08/10/17 13:36), Kees Cook wrote:
[..]
> -static int pstore_check_syslog_permissions(struct pstore_private *ps)
> -{
> -	switch (ps->record->type) {
> -	case PSTORE_TYPE_DMESG:
> -	case PSTORE_TYPE_CONSOLE:
> -		return check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> -			SYSLOG_FROM_READER);
> -	default:
> -		return 0;
> -	}
> -}
> -
>  static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
>  						size_t count, loff_t *ppos)
>  {
> @@ -163,10 +150,6 @@ static int pstore_file_open(struct inode *inode, struct file *file)
>  	int err;
>  	const struct seq_operations *sops = NULL;
>  
> -	err = pstore_check_syslog_permissions(ps);
> -	if (err)
> -		return err;
> -
>  	if (ps->record->type == PSTORE_TYPE_FTRACE)
>  		sops = &pstore_ftrace_seq_ops;
>  
> @@ -204,11 +187,6 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>  {
>  	struct pstore_private *p = d_inode(dentry)->i_private;
>  	struct pstore_record *record = p->record;
> -	int err;
> -
> -	err = pstore_check_syslog_permissions(p);
> -	if (err)
> -		return err;

it's hard to review security related patches :)

so, effectively, `dmesg_restrict' does not work for pstore anymore? wouldn't
that be a problem? one more thing, doesn't it affect the consistency -- we
respect the `dmesg_restrict' restrictions, except that we ignore it when
access pstore? or do I completely misunderstand the change? sorry if so.

	-ss

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Kees Cook]

On Wed, Aug 16, 2017 at 12:59 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (08/10/17 13:36), Kees Cook wrote:
> [..]
>> -static int pstore_check_syslog_permissions(struct pstore_private *ps)
>> -{
>> -     switch (ps->record->type) {
>> -     case PSTORE_TYPE_DMESG:
>> -     case PSTORE_TYPE_CONSOLE:
>> -             return check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
>> -                     SYSLOG_FROM_READER);
>> -     default:
>> -             return 0;
>> -     }
>> -}
>> -
>>  static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
>>                                               size_t count, loff_t *ppos)
>>  {
>> @@ -163,10 +150,6 @@ static int pstore_file_open(struct inode *inode, struct file *file)
>>       int err;
>>       const struct seq_operations *sops = NULL;
>>
>> -     err = pstore_check_syslog_permissions(ps);
>> -     if (err)
>> -             return err;
>> -
>>       if (ps->record->type == PSTORE_TYPE_FTRACE)
>>               sops = &pstore_ftrace_seq_ops;
>>
>> @@ -204,11 +187,6 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>>  {
>>       struct pstore_private *p = d_inode(dentry)->i_private;
>>       struct pstore_record *record = p->record;
>> -     int err;
>> -
>> -     err = pstore_check_syslog_permissions(p);
>> -     if (err)
>> -             return err;
>
> it's hard to review security related patches :)
>
> so, effectively, `dmesg_restrict' does not work for pstore anymore? wouldn't
> that be a problem? one more thing, doesn't it affect the consistency -- we
> respect the `dmesg_restrict' restrictions, except that we ignore it when
> access pstore? or do I completely misunderstand the change? sorry if so.

This revert is combined with the commit before it which changes the
perms of the pstorefs rootdir to 750. Privacy is retained, but now a
system owner can modify access to specific entirely unprivileged
groups (that do no require CAP_SYSLOG). Note, also, that pstore
(normally) shows only the prior boot's console and crash.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Sergey Senozhatsky]

Hello Kees,

On (08/16/17 08:38), Kees Cook wrote:
[..]
> > so, effectively, `dmesg_restrict' does not work for pstore anymore? wouldn't
> > that be a problem? one more thing, doesn't it affect the consistency -- we
> > respect the `dmesg_restrict' restrictions, except that we ignore it when
> > access pstore? or do I completely misunderstand the change? sorry if so.
> 
> This revert is combined with the commit before it which changes the
> perms of the pstorefs rootdir to 750. Privacy is retained, but now a
> system owner can modify access to specific entirely unprivileged
> groups (that do no require CAP_SYSLOG).

sure, I saw the 0750 change.

> Note, also, that pstore (normally) shows only the prior boot's console
> and crash.

one more question,

can we accidentally "leak" kernel pointers or some other critical
info? kptr_restrict requires CAP_SYSLOG and pstore read used to
require CAP_SYSLOG, but it seems that now we can bypass it by
letting "entirely unprivileged groups" to read pstore. is there
something to be concerned about (or at least mention it in the
commit messages)?

	-ss

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Kees Cook]

On Wed, Aug 16, 2017 at 6:29 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> can we accidentally "leak" kernel pointers or some other critical
> info? kptr_restrict requires CAP_SYSLOG and pstore read used to
> require CAP_SYSLOG, but it seems that now we can bypass it by
> letting "entirely unprivileged groups" to read pstore. is there
> something to be concerned about (or at least mention it in the
> commit messages)?

I can expand the commit message a bit more, sure. There may be
sensitive things in pstorefs, and it's up to a system builder to
decide how they want to deal with that risk. Most users of pstore
don't mount with update_ms=N so pstorefs contains (mostly) old
addresses. Without this change, though, a builder can't give
permissions to an unprivileged crash dump process without also giving
it CAP_SYSLOG which has much MORE privilege that it would need
(reading and wiping _current_ dmesg, for example).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

[Sergey Senozhatsky]

Hello,

On (08/17/17 16:01), Kees Cook wrote:
> On Wed, Aug 16, 2017 at 6:29 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> > can we accidentally "leak" kernel pointers or some other critical
> > info? kptr_restrict requires CAP_SYSLOG and pstore read used to
> > require CAP_SYSLOG, but it seems that now we can bypass it by
> > letting "entirely unprivileged groups" to read pstore. is there
> > something to be concerned about (or at least mention it in the
> > commit messages)?
> 
> I can expand the commit message a bit more, sure.

that would be lovely. please do.

> There may be sensitive things in pstorefs, and it's up to a system builder
> to decide how they want to deal with that risk. Most users of pstore
> don't mount with update_ms=N so pstorefs contains (mostly) old
> addresses.

I see...

> Without this change, though, a builder can't give permissions to an
> unprivileged crash dump process without also giving it CAP_SYSLOG which
> has much MORE privilege that it would need (reading and wiping _current_
> dmesg, for example).

ok, the "CAP_SYSLOG and _current_ dmesg" point is surely interesting.
could you please also add this to the commit message?


FWIW, both patches

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss