Re: [RESEND] security/keys: remove possessor verify after key permission check

Re: [RESEND] security/keys: remove possessor verify after key permission check

[Will Deacon]

On Thu, Apr 30, 2020 at 10:34:03AM +0300, Alexey Krasikov wrote:
> In security/keys/keyctl.c: keyctl_read_key() after key_permission() check
> is called is_key_possessed(). According to the current logic, if the caller is
> a possessor, then it can read the key regardless of whether it has rights
> to do so.
> 
> if I remove the possessor read rights:
>     keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_SETATTR));
> the calling process will still be able to read the key if it is possessor.
> 
> In other words, if the possessor doesn't have read rights, it doesn't matter.
> 
> ---
> I may be misunderstanding the logic behind it, but here's the patch to
> stir the discussion.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  security/keys/keyctl.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)

Hmm, looks like this still didn't make it to the keyrings@ list :(

On the off-chance that my reply /does/ make it, I've left the whole of the
patch intact below. Please can somebody have a look?

Will

--->8

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 5e01192e222a..61e53c6b5adb 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -845,22 +845,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  
>  	/* see if we can read it directly */
>  	ret = key_permission(key_ref, KEY_NEED_READ);
> -	if (ret = 0)
> -		goto can_read_key;
> -	if (ret != -EACCES)
> +	if (ret != 0)
>  		goto key_put_out;
>  
> -	/* we can't; see if it's searchable from this process's keyrings
> -	 * - we automatically take account of the fact that it may be
> -	 *   dangling off an instantiation key
> -	 */
> -	if (!is_key_possessed(key_ref)) {
> -		ret = -EACCES;
> -		goto key_put_out;
> -	}
> -
> -	/* the key is probably readable - now try to read it */
> -can_read_key:
>  	if (!key->type->read) {
>  		ret = -EOPNOTSUPP;
>  		goto key_put_out;
> -- 
> 2.17.1
> 

Re: [RESEND] security/keys: remove possessor verify after key permission check

[Jarkko Sakkinen]

On Tue, May 05, 2020 at 10:19:59AM +0100, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 10:34:03AM +0300, Alexey Krasikov wrote:
> > In security/keys/keyctl.c: keyctl_read_key() after key_permission() check
> > is called is_key_possessed(). According to the current logic, if the caller is
> > a possessor, then it can read the key regardless of whether it has rights
> > to do so.
> > 
> > if I remove the possessor read rights:
> >     keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_SETATTR));
> > the calling process will still be able to read the key if it is possessor.
> > 
> > In other words, if the possessor doesn't have read rights, it doesn't matter.
> > 
> > ---
> > I may be misunderstanding the logic behind it, but here's the patch to
> > stir the discussion.
> > 
> > Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> > ---
> >  security/keys/keyctl.c | 15 +--------------
> >  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> Hmm, looks like this still didn't make it to the keyrings@ list :(
> 
> On the off-chance that my reply /does/ make it, I've left the whole of the
> patch intact below. Please can somebody have a look?
> 
> Will

Hi, I'm on this. Just didn't have time last week. Looking it through
on *some* day this week properly.

/Jarkko

Re: [RESEND] security/keys: remove possessor verify after key permission check

[Jarkko Sakkinen]

On Thu, 2020-04-30 at 10:34 +0300, Alexey Krasikov wrote:
> In security/keys/keyctl.c: keyctl_read_key() after key_permission() check
> is called is_key_possessed(). According to the current logic, if the caller is
> a possessor, then it can read the key regardless of whether it has rights
> to do so.
> 
> if I remove the possessor read rights:
>     keyctl_setperm(key, KEY_POS_ALL & (~KEY_POS_SETATTR));
> the calling process will still be able to read the key if it is possessor.
> 
> In other words, if the possessor doesn't have read rights, it doesn't matter.
> 
> ---
> I may be misunderstanding the logic behind it, but here's the patch to
> stir the discussion.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>

Tried this:

$ KEYID=$(keyctl add user john smith @u)
$ keyctl setperm $KEYID 0x20000000
$ keyctl read $KEYID
keyctl_read_alloc: Permission denied

You did not provide a full example like this so I have to assume that
this is what you meant.

/Jarkko

Re: [RESEND] security/keys: remove possessor verify after key permission check

[Jarkko Sakkinen]

On Thu, May 07, 2020 at 11:39:48AM +0300, Алексей Красиков wrote:
>     
>    You've removed all the attributes except the reading.
>    Nothing can be done with the key if it can't be found.
>    Try this:
>     
>    $ KEYID=$(keyctl add user john smith @u)
>    $ keyctl describe $KEYID
>    $ keyctl setperm $KEYID 0x3d000000
>    $ keyctl describe $KEYID
>    $ keyctl print $KEYID
>     
>    /Alexey

Hi please re-respond with plain text.

HTML mails do not go to vger.kernel.org. Also please try to avoid
top posting (i.e. quote and respond inline).


/Jarkko

Re: [RESEND] security/keys: remove possessor verify after key permission check

[Jarkko Sakkinen]

On Mon, 2020-05-25 at 13:56 +0300, Alexey Krasikov wrote:
> 14.05.2020 12:46, Alexey Krasikov пишет:
> > > 07.05.2020, 10:24, "Jarkko Sakkinen" <jarkko.sakkinen@linux.intel.com>:
> > > 
> > >     Tried this:
> > > 
> > > 
> > >     $ KEYID=$(keyctl add user john smith @u)
> > >     $ keyctl setperm $KEYID 0x20000000
> > >     $ keyctl read $KEYID
> > >     keyctl_read_alloc: Permission denied
> > > 
> > >     You did not provide a full example like this so I have to assume 
> > > that
> > >     this is what you meant.
> > > 
> > >     /Jarkko
> > > 
> > > 
> > 
> > You've removed all the attributes except the reading.
> > Nothing can be done with the key if it can't be found.
> > Try this:
> > $ KEYID=$(keyctl add user john smith @u)
> > $ keyctl describe $KEYID
> > $ keyctl setperm $KEYID 0x3d000000
> > $ keyctl describe $KEYID
> > $ keyctl print $KEYID
> > /Alexey
> 
> ping

Please send a new version with a full example of the scenario that
you are referring. This thread became too messy to follow with the
HTML emails included (that do no reach vger).

/Jarkko

Re: [RESEND] security/keys: remove possessor verify after key permission check

[James Bottomley]

On Wed, 2020-05-27 at 22:47 +0300, Jarkko Sakkinen wrote:
[...]
> > ping
> 
> Please send a new version with a full example of the scenario that
> you are referring. This thread became too messy to follow with the
> HTML emails included (that do no reach vger).

Yes, please ... I'm missing most of the emails because of the vger and
html problem.  I think the request is to remove the possessor check in
keyctl_read, but just done blindly that would completely destroy our
namespaced security system for keys, so it doesn't sound like a good
idea at all.  What's the actual problem this is trying to solve?  It's
annoying that root has to join the session keyring to read a key, but
the reason for it is well justified and the fact that even root can't
reach some session keyrings is a feature not a bug.

James