* Re: [RESEND] security/keys: remove possessor verify after key permission check
@ 2020-05-05 9:19 Will Deacon
2020-05-05 13:07 ` Jarkko Sakkinen
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Will Deacon @ 2020-05-05 9:19 UTC (permalink / raw)
To: keyrings
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND] security/keys: remove possessor verify after key permission check
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
@ 2020-05-05 13:07 ` Jarkko Sakkinen
2020-05-07 7:24 ` Jarkko Sakkinen
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-05-05 13:07 UTC (permalink / raw)
To: keyrings
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND] security/keys: remove possessor verify after key permission check
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
2020-05-05 13:07 ` Jarkko Sakkinen
@ 2020-05-07 7:24 ` Jarkko Sakkinen
2020-05-13 16:50 ` Jarkko Sakkinen
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-05-07 7:24 UTC (permalink / raw)
To: keyrings
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND] security/keys: remove possessor verify after key permission check
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
2020-05-05 13:07 ` Jarkko Sakkinen
2020-05-07 7:24 ` Jarkko Sakkinen
@ 2020-05-13 16:50 ` Jarkko Sakkinen
2020-05-27 19:47 ` Jarkko Sakkinen
2020-05-27 19:58 ` James Bottomley
4 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-05-13 16:50 UTC (permalink / raw)
To: keyrings
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND] security/keys: remove possessor verify after key permission check
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
` (2 preceding siblings ...)
2020-05-13 16:50 ` Jarkko Sakkinen
@ 2020-05-27 19:47 ` Jarkko Sakkinen
2020-05-27 19:58 ` James Bottomley
4 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-05-27 19:47 UTC (permalink / raw)
To: keyrings
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RESEND] security/keys: remove possessor verify after key permission check
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
` (3 preceding siblings ...)
2020-05-27 19:47 ` Jarkko Sakkinen
@ 2020-05-27 19:58 ` James Bottomley
4 siblings, 0 replies; 6+ messages in thread
From: James Bottomley @ 2020-05-27 19:58 UTC (permalink / raw)
To: keyrings
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-27 19:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-05 9:19 [RESEND] security/keys: remove possessor verify after key permission check Will Deacon
2020-05-05 13:07 ` Jarkko Sakkinen
2020-05-07 7:24 ` Jarkko Sakkinen
2020-05-13 16:50 ` Jarkko Sakkinen
2020-05-27 19:47 ` Jarkko Sakkinen
2020-05-27 19:58 ` James Bottomley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.