Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1712743 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg  dumps" Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-08-16 10:00 +0200
    Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" Kees Cook <keescook@chromium.org> - 2017-08-16 17:40 +0200
      Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg  dumps" Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-08-17 03:30 +0200
        Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" Kees Cook <keescook@chromium.org> - 2017-08-18 01:10 +0200
          Re: [PATCH 2/2] Revert "pstore: Honor dmesg_restrict sysctl on dmesg  dumps" Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-08-18 01:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1713469

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1714482

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web