Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Paul Moore]

I just notice that the "selinux: Implement the watch_key security
hook" patch made it's way into linux-next via 9ba09998baa9:

  commit 9ba09998baa995518d94c9a32e6329b28ccb9045
  Author: David Howells <dhowells@redhat.com>
  Date:   Tue Jan 14 17:07:13 2020 +0000

   selinux: Implement the watch_key security hook

   Implement the watch_key security hook to make sure that a key grants the
   caller View permission in order to set a watch on a key.

   For the moment, the watch_devices security hook is left unimplemented as
   it's not obvious what the object should be since the queue is global and
   didn't previously exist.

   Signed-off-by: David Howells <dhowells@redhat.com>
   Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

I'm reasonably confident that this code hasn't been tested as I expect
it would fail, or at the very least behave in unintended ways.  The
problem is the selinux_watch_key(...) function, shown below:

+static int selinux_watch_key(struct key *key)
+{
+       struct key_security_struct *ksec = key->security;
+       u32 sid = current_sid();
+
+       return avc_has_perm(&selinux_state,
+                           sid, ksec->sid, SECCLASS_KEY, KEY_NEED_VIEW, NULL);
+}

... in particular it is the fifth argument to avc_has_perm(),
"KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
permission and would likely result in odd behavior when passed to
avc_has_perm().  Given that the keyring permission to SELinux object
class permission is variable depending on the key_perms policy
capability, it probably makes the most sense to pull the permission
mapping in selinux_key_permission() out into a separate function, e.g.
key_perm_to_av(...) (see the other XXX_to_av() functions in
security/selinux/hooks.c), and then use this newly created mapping
function in both selinux_key_permission() and selinux_watch_key().  Or
you could just duplicate the KEY_NEED_VIEW mapping code in both
functions, but I would advise against that.

-- 
paul moore
www.paul-moore.com

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Richard Haines]

On Fri, 2020-04-17 at 11:48 -0400, Paul Moore wrote:
> I just notice that the "selinux: Implement the watch_key security
> hook" patch made it's way into linux-next via 9ba09998baa9:
> 
>   commit 9ba09998baa995518d94c9a32e6329b28ccb9045
>   Author: David Howells <dhowells@redhat.com>
>   Date:   Tue Jan 14 17:07:13 2020 +0000
> 
>    selinux: Implement the watch_key security hook
> 
>    Implement the watch_key security hook to make sure that a key
> grants the
>    caller View permission in order to set a watch on a key.
> 
>    For the moment, the watch_devices security hook is left
> unimplemented as
>    it's not obvious what the object should be since the queue is
> global and
>    didn't previously exist.
> 
>    Signed-off-by: David Howells <dhowells@redhat.com>
>    Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> I'm reasonably confident that this code hasn't been tested as I
> expect
> it would fail, or at the very least behave in unintended ways.  The
> problem is the selinux_watch_key(...) function, shown below:

I built an selinx-testsuite test for this last year and it worked fine
then. I'll send the test as an RFC patch as its been some time since I
ran it. David also has a test in kernel
samples/watch_queue/watch_test.c

> 
> +static int selinux_watch_key(struct key *key)
> +{
> +       struct key_security_struct *ksec = key->security;
> +       u32 sid = current_sid();
> +
> +       return avc_has_perm(&selinux_state,
> +                           sid, ksec->sid, SECCLASS_KEY,
> KEY_NEED_VIEW, NULL);
> +}
> 
> ... in particular it is the fifth argument to avc_has_perm(),
> "KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux

True, however by magic the KEY_NEED_* perms match with the bits defined
in classmap.h. I did some work on this during the 'keys' saga, see
various emails in list like [1]

[1] 
https://lore.kernel.org/selinux/20200220181031.156674-2-richard_c_haines@btinternet.com/

> permission and would likely result in odd behavior when passed to
> avc_has_perm().  Given that the keyring permission to SELinux object
> class permission is variable depending on the key_perms policy
> capability, it probably makes the most sense to pull the permission
> mapping in selinux_key_permission() out into a separate function,
> e.g.
> key_perm_to_av(...) (see the other XXX_to_av() functions in
> security/selinux/hooks.c), and then use this newly created mapping
> function in both selinux_key_permission() and
> selinux_watch_key().  Or
> you could just duplicate the KEY_NEED_VIEW mapping code in both
> functions, but I would advise against that.
> 

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Paul Moore]

On Fri, Apr 17, 2020 at 12:32 PM Richard Haines
<richard_c_haines@btinternet.com> wrote:
> On Fri, 2020-04-17 at 11:48 -0400, Paul Moore wrote:
> > I just notice that the "selinux: Implement the watch_key security
> > hook" patch made it's way into linux-next via 9ba09998baa9:
> >
> >   commit 9ba09998baa995518d94c9a32e6329b28ccb9045
> >   Author: David Howells <dhowells@redhat.com>
> >   Date:   Tue Jan 14 17:07:13 2020 +0000
> >
> >    selinux: Implement the watch_key security hook
> >
> >    Implement the watch_key security hook to make sure that a key
> > grants the
> >    caller View permission in order to set a watch on a key.
> >
> >    For the moment, the watch_devices security hook is left
> > unimplemented as
> >    it's not obvious what the object should be since the queue is
> > global and
> >    didn't previously exist.
> >
> >    Signed-off-by: David Howells <dhowells@redhat.com>
> >    Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> >
> > I'm reasonably confident that this code hasn't been tested as I
> > expect
> > it would fail, or at the very least behave in unintended ways.  The
> > problem is the selinux_watch_key(...) function, shown below:
>
> I built an selinx-testsuite test for this last year and it worked fine
> then. I'll send the test as an RFC patch as its been some time since I
> ran it. David also has a test in kernel
> samples/watch_queue/watch_test.c

See below.

> > +static int selinux_watch_key(struct key *key)
> > +{
> > +       struct key_security_struct *ksec = key->security;
> > +       u32 sid = current_sid();
> > +
> > +       return avc_has_perm(&selinux_state,
> > +                           sid, ksec->sid, SECCLASS_KEY,
> > KEY_NEED_VIEW, NULL);
> > +}
> >
> > ... in particular it is the fifth argument to avc_has_perm(),
> > "KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
>
> True, however by magic the KEY_NEED_* perms match with the bits defined
> in classmap.h. I did some work on this during the 'keys' saga, see
> various emails in list like [1]
>
> [1]
> https://lore.kernel.org/selinux/20200220181031.156674-2-richard_c_haines@btinternet.com/

Esh, relying on the constants to line up is a recipe for disaster.  We
really need that permission translation layer.

-- 
paul moore
www.paul-moore.com

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[David Howells]

Paul Moore <paul@paul-moore.com> wrote:

> ... in particular it is the fifth argument to avc_has_perm(),
> "KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
> permission and would likely result in odd behavior when passed to
> avc_has_perm().

I think it works because KEY_NEED_VIEW happens to coincide with KEY__VIEW.  It
should just use KEY__VIEW instead.

> it probably makes the most sense to pull the permission mapping in
> selinux_key_permission() out into a separate function, e.g.
> key_perm_to_av(...)

Agreed.  How about the attached patch?  I wonder if I should do bit-by-bit
translation rather than using a switch?  But then it's difficult to decide
what it means if someone passes in two KEY_NEED_* flags OR'd together - is it
either or both?

> and then use this newly created mapping function in [...]
> selinux_watch_key()

No, I think I should just hard-code KEY__VIEW there.

David
---
commit 70d1d82aa014ae4511976b4d80c17138006dddec
Author: David Howells <dhowells@redhat.com>
Date:   Tue Apr 21 13:15:16 2020 +0100

    selinux: Fix use of KEY_NEED_* instead of KEY__* perms
    
    selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
    security_sid_to_context() instead of the KEY__* values.  It happens to work
    because the values are all coincident.
    
    Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
    Reported-by: Paul Moore <paul@paul-moore.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..32f7fa538c5f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6539,12 +6539,27 @@ static void selinux_key_free(struct key *k)
 	kfree(ksec);
 }
 
+static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
+{
+	switch (need_perm) {
+	case KEY_NEED_VIEW:	return KEY__VIEW;
+	case KEY_NEED_READ:	return KEY__READ;
+	case KEY_NEED_WRITE:	return KEY__WRITE;
+	case KEY_NEED_SEARCH:	return KEY__SEARCH;
+	case KEY_NEED_LINK:	return KEY__LINK;
+	case KEY_NEED_SETATTR:	return KEY__SETATTR;
+	default:
+		return 0;
+	}
+}
+
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  unsigned need_perm)
 {
 	struct key *key;
 	struct key_security_struct *ksec;
+	unsigned int perm;
 	u32 sid;
 
 	/* if no specific permissions are requested, we skip the
@@ -6553,6 +6568,7 @@ static int selinux_key_permission(key_ref_t key_ref,
 	if (perm == 0)
 		return 0;
 
+	perm = selinux_keyperm_to_av(need_perm);
 	sid = cred_sid(cred);
 
 	key = key_ref_to_ptr(key_ref);

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Paul Moore]

On Tue, Apr 21, 2020 at 8:29 AM David Howells <dhowells@redhat.com> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>
> > ... in particular it is the fifth argument to avc_has_perm(),
> > "KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
> > permission and would likely result in odd behavior when passed to
> > avc_has_perm().
>
> I think it works because KEY_NEED_VIEW happens to coincide with KEY__VIEW.  It
> should just use KEY__VIEW instead.

Yes, it looks like it.  To be clear, it is dangerous to rely on
permission values from outside SELinux aligning with SELinux
permissions; changing the outside permission values w/o adjusting the
SELinux hook code to do the necessary translation will result in some
scary behavior (wrong permission checks).

> > it probably makes the most sense to pull the permission mapping in
> > selinux_key_permission() out into a separate function, e.g.
> > key_perm_to_av(...)
>
> Agreed.  How about the attached patch?  I wonder if I should do bit-by-bit
> translation rather than using a switch?  But then it's difficult to decide
> what it means if someone passes in two KEY_NEED_* flags OR'd together - is it
> either or both?

Comments inline.

> > and then use this newly created mapping function in [...]
> > selinux_watch_key()
>
> No, I think I should just hard-code KEY__VIEW there.

FWIW, my comment was based on a version of linux-next where you were
making policycap based permission adjustments to KEY_VIEW and I
thought you would want the same adjustments to be applied to both
access control points.  That code appears to now be gone in
linux-next.

> ---
> commit 70d1d82aa014ae4511976b4d80c17138006dddec
> Author: David Howells <dhowells@redhat.com>
> Date:   Tue Apr 21 13:15:16 2020 +0100
>
>     selinux: Fix use of KEY_NEED_* instead of KEY__* perms
>
>     selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
>     security_sid_to_context() instead of the KEY__* values.  It happens to work
>     because the values are all coincident.
>
>     Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
>     Reported-by: Paul Moore <paul@paul-moore.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..32f7fa538c5f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6539,12 +6539,27 @@ static void selinux_key_free(struct key *k)
>         kfree(ksec);
>  }
>
> +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> +{
> +       switch (need_perm) {
> +       case KEY_NEED_VIEW:     return KEY__VIEW;
> +       case KEY_NEED_READ:     return KEY__READ;
> +       case KEY_NEED_WRITE:    return KEY__WRITE;
> +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> +       case KEY_NEED_LINK:     return KEY__LINK;
> +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> +       default:
> +               return 0;
> +       }

Regarding your question of permission translation via switch-statement
as opposed to bit-by-bit comparison, I think it depends on if multiple
permissions are going to be required in a single call to the hook.
The failure mode for the code above if multiple permissions are
requested is not very good, it defaults to *no* permission which means
that if someone requested KEY_NEED_SEARCH|KEY_NEED_VIEW (or some other
combination) then the SELinux check would not check any permissions
... that seems wrong to me.

If we want to stick with a switch statement I think we should have it
return -EPERM for the default case to protect against this.  We don't
need the full 32-bits afforded us by the unsigned int.

> +}
> +
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 unsigned need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> +       unsigned int perm;
>         u32 sid;
>
>         /* if no specific permissions are requested, we skip the
> @@ -6553,6 +6568,7 @@ static int selinux_key_permission(key_ref_t key_ref,
>         if (perm == 0)
>                 return 0;
>
> +       perm = selinux_keyperm_to_av(need_perm);

... and add a check for (perm < 0) as discussed above if we stick with
the switch statement.

>         sid = cred_sid(cred);
>
>         key = key_ref_to_ptr(key_ref);
>

-- 
paul moore
www.paul-moore.com

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Paul Moore]

On Wed, Apr 22, 2020 at 3:20 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Apr 21, 2020 at 8:29 AM David Howells <dhowells@redhat.com> wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> >
> > > ... in particular it is the fifth argument to avc_has_perm(),
> > > "KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
> > > permission and would likely result in odd behavior when passed to
> > > avc_has_perm().
> >
> > I think it works because KEY_NEED_VIEW happens to coincide with KEY__VIEW.  It
> > should just use KEY__VIEW instead.
>
> Yes, it looks like it.  To be clear, it is dangerous to rely on
> permission values from outside SELinux aligning with SELinux
> permissions; changing the outside permission values w/o adjusting the
> SELinux hook code to do the necessary translation will result in some
> scary behavior (wrong permission checks).
>
> > > it probably makes the most sense to pull the permission mapping in
> > > selinux_key_permission() out into a separate function, e.g.
> > > key_perm_to_av(...)
> >
> > Agreed.  How about the attached patch?  I wonder if I should do bit-by-bit
> > translation rather than using a switch?  But then it's difficult to decide
> > what it means if someone passes in two KEY_NEED_* flags OR'd together - is it
> > either or both?
>
> Comments inline.
>
> > > and then use this newly created mapping function in [...]
> > > selinux_watch_key()
> >
> > No, I think I should just hard-code KEY__VIEW there.
>
> FWIW, my comment was based on a version of linux-next where you were
> making policycap based permission adjustments to KEY_VIEW and I
> thought you would want the same adjustments to be applied to both
> access control points.  That code appears to now be gone in
> linux-next.
>
> > ---
> > commit 70d1d82aa014ae4511976b4d80c17138006dddec
> > Author: David Howells <dhowells@redhat.com>
> > Date:   Tue Apr 21 13:15:16 2020 +0100
> >
> >     selinux: Fix use of KEY_NEED_* instead of KEY__* perms
> >
> >     selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
> >     security_sid_to_context() instead of the KEY__* values.  It happens to work
> >     because the values are all coincident.
> >
> >     Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
> >     Reported-by: Paul Moore <paul@paul-moore.com>
> >     Signed-off-by: David Howells <dhowells@redhat.com>
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0b4e32161b77..32f7fa538c5f 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6539,12 +6539,27 @@ static void selinux_key_free(struct key *k)
> >         kfree(ksec);
> >  }
> >
> > +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> > +{
> > +       switch (need_perm) {
> > +       case KEY_NEED_VIEW:     return KEY__VIEW;
> > +       case KEY_NEED_READ:     return KEY__READ;
> > +       case KEY_NEED_WRITE:    return KEY__WRITE;
> > +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> > +       case KEY_NEED_LINK:     return KEY__LINK;
> > +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> > +       default:
> > +               return 0;
> > +       }
>
> Regarding your question of permission translation via switch-statement
> as opposed to bit-by-bit comparison, I think it depends on if multiple
> permissions are going to be required in a single call to the hook.
> The failure mode for the code above if multiple permissions are
> requested is not very good, it defaults to *no* permission which means
> that if someone requested KEY_NEED_SEARCH|KEY_NEED_VIEW (or some other
> combination) then the SELinux check would not check any permissions
> ... that seems wrong to me.
>
> If we want to stick with a switch statement I think we should have it
> return -EPERM for the default case to protect against this.  We don't
> need the full 32-bits afforded us by the unsigned int.
>
> > +}
> > +
> >  static int selinux_key_permission(key_ref_t key_ref,
> >                                   const struct cred *cred,
> > -                                 unsigned perm)
> > +                                 unsigned need_perm)
> >  {
> >         struct key *key;
> >         struct key_security_struct *ksec;
> > +       unsigned int perm;
> >         u32 sid;
> >
> >         /* if no specific permissions are requested, we skip the
> > @@ -6553,6 +6568,7 @@ static int selinux_key_permission(key_ref_t key_ref,
> >         if (perm == 0)
> >                 return 0;
> >
> > +       perm = selinux_keyperm_to_av(need_perm);
>
> ... and add a check for (perm < 0) as discussed above if we stick with
> the switch statement.

... and we should probably emit some sort of message to indicate that
an invalid permission set was used.

-- 
paul moore
www.paul-moore.com

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[David Howells]

Paul Moore <paul@paul-moore.com> wrote:

> > > and then use this newly created mapping function in [...]
> > > selinux_watch_key()
> >
> > No, I think I should just hard-code KEY__VIEW there.
> 
> FWIW, my comment was based on a version of linux-next where you were
> making policycap based permission adjustments to KEY_VIEW and I
> thought you would want the same adjustments to be applied to both
> access control points.  That code appears to now be gone in
> linux-next.

I don't think I changed KEY_VIEW specifically; anyway, that code is on hold
for the moment since it collides with this.

What I was wondering is if I should change KEY_NEED_xxx from a bitmask into an
enum to remove the confusion about whether or not you're allowed to provide
multiple 'needs' OR'd together.

> > +       perm = selinux_keyperm_to_av(need_perm);
> 
> ... and add a check for (perm < 0) as discussed above if we stick with
> the switch statement.

Actually, there was supposed to be a:

	if (!perm)
		return -EPERM;

after that line.

David

Re: Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next

[Paul Moore]

On Fri, Apr 24, 2020 at 7:43 PM David Howells <dhowells@redhat.com> wrote:
>
> Paul Moore <paul@paul-moore.com> wrote:
>
> > > > and then use this newly created mapping function in [...]
> > > > selinux_watch_key()
> > >
> > > No, I think I should just hard-code KEY__VIEW there.
> >
> > FWIW, my comment was based on a version of linux-next where you were
> > making policycap based permission adjustments to KEY_VIEW and I
> > thought you would want the same adjustments to be applied to both
> > access control points.  That code appears to now be gone in
> > linux-next.
>
> I don't think I changed KEY_VIEW specifically; anyway, that code is on hold
> for the moment since it collides with this.
>
> What I was wondering is if I should change KEY_NEED_xxx from a bitmask into an
> enum to remove the confusion about whether or not you're allowed to provide
> multiple 'needs' OR'd together.
>
> > > +       perm = selinux_keyperm_to_av(need_perm);
> >
> > ... and add a check for (perm < 0) as discussed above if we stick with
> > the switch statement.
>
> Actually, there was supposed to be a:
>
>         if (!perm)
>                 return -EPERM;
>
> after that line.

Okay, can you send the next version of the patch to the SELinux list for review?

Thank you.

-- 
paul moore
www.paul-moore.com

[PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms

[David Howells]

Paul Moore <paul@paul-moore.com> wrote:

> Okay, can you send the next version of the patch to the SELinux list for
> review?

Here you go.  Note that I did this a few days ago and I actually used EACCES
rather than EPERM.  Which one is one preferred for this?

David
---
selinux: Fix use of KEY_NEED_* instead of KEY__* perms

selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
security_sid_to_context() instead of the KEY__* values.  It happens to work
because the values are all coincident.

Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
Reported-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
 security/selinux/hooks.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..6087955b49d8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6539,20 +6539,38 @@ static void selinux_key_free(struct key *k)
 	kfree(ksec);
 }
 
+static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
+{
+	switch (need_perm) {
+	case KEY_NEED_VIEW:	return KEY__VIEW;
+	case KEY_NEED_READ:	return KEY__READ;
+	case KEY_NEED_WRITE:	return KEY__WRITE;
+	case KEY_NEED_SEARCH:	return KEY__SEARCH;
+	case KEY_NEED_LINK:	return KEY__LINK;
+	case KEY_NEED_SETATTR:	return KEY__SETATTR;
+	default:
+		return 0;
+	}
+}
+
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  unsigned need_perm)
 {
 	struct key *key;
 	struct key_security_struct *ksec;
+	unsigned int perm;
 	u32 sid;
 
 	/* if no specific permissions are requested, we skip the
 	   permission check. No serious, additional covert channels
 	   appear to be created. */
-	if (perm == 0)
+	if (need_perm == 0)
 		return 0;
 
+	perm = selinux_keyperm_to_av(need_perm);
+	if (perm == 0)
+		return -EACCES;
 	sid = cred_sid(cred);
 
 	key = key_ref_to_ptr(key_ref);

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms

[Stephen Smalley]

On Mon, Apr 27, 2020 at 10:13 AM David Howells <dhowells@redhat.com> wrote:
>
> Paul Moore <paul@paul-moore.com> wrote:
>
> > Okay, can you send the next version of the patch to the SELinux list for
> > review?
>
> Here you go.  Note that I did this a few days ago and I actually used EACCES
> rather than EPERM.  Which one is one preferred for this?

Generally SELinux returns EACCES unless the hook normally returns
EPERM (e.g. capable).
Should we use a build-time or runtime guard to catch introduction of
new KEY_NEED values without corresponding SELinux
permissions?

>
> David
> ---
> selinux: Fix use of KEY_NEED_* instead of KEY__* perms
>
> selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
> security_sid_to_context() instead of the KEY__* values.  It happens to work

s/security_sid_to_context/avc_has_perm

> because the values are all coincident.

Shrug.  That was just a requirement on key permissions when they were
introduced; same is true of capabilities.
Not opposed to explicitly mapping them now but it isn't really a bug.

>
> Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
> Reported-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>  security/selinux/hooks.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..6087955b49d8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6539,20 +6539,38 @@ static void selinux_key_free(struct key *k)
>         kfree(ksec);
>  }
>
> +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> +{
> +       switch (need_perm) {
> +       case KEY_NEED_VIEW:     return KEY__VIEW;
> +       case KEY_NEED_READ:     return KEY__READ;
> +       case KEY_NEED_WRITE:    return KEY__WRITE;
> +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> +       case KEY_NEED_LINK:     return KEY__LINK;
> +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> +       default:
> +               return 0;
> +       }
> +}
> +
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 unsigned need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> +       unsigned int perm;
>         u32 sid;
>
>         /* if no specific permissions are requested, we skip the
>            permission check. No serious, additional covert channels
>            appear to be created. */
> -       if (perm == 0)
> +       if (need_perm == 0)
>                 return 0;
>
> +       perm = selinux_keyperm_to_av(need_perm);
> +       if (perm == 0)
> +               return -EACCES;
>         sid = cred_sid(cred);
>
>         key = key_ref_to_ptr(key_ref);
>

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms

[Paul Moore]

On Mon, Apr 27, 2020 at 10:36 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Apr 27, 2020 at 10:13 AM David Howells <dhowells@redhat.com> wrote:
> >
> > Paul Moore <paul@paul-moore.com> wrote:
> >
> > > Okay, can you send the next version of the patch to the SELinux list for
> > > review?
> >
> > Here you go.  Note that I did this a few days ago and I actually used EACCES
> > rather than EPERM.  Which one is one preferred for this?
>
> Generally SELinux returns EACCES unless the hook normally returns
> EPERM (e.g. capable).
> Should we use a build-time or runtime guard to catch introduction of
> new KEY_NEED values without corresponding SELinux
> permissions?
>
> >
> > David
> > ---
> > selinux: Fix use of KEY_NEED_* instead of KEY__* perms
> >
> > selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
> > security_sid_to_context() instead of the KEY__* values.  It happens to work
>
> s/security_sid_to_context/avc_has_perm
>
> > because the values are all coincident.
>
> Shrug.  That was just a requirement on key permissions when they were
> introduced; same is true of capabilities.
> Not opposed to explicitly mapping them now but it isn't really a bug.

I haven't looked at the rest of the patch yet, but I wanted to make a
quick comment on this ... over the years I've seen a number of
problems that crop up because of cross-subsytem assumptions, unless
there is some performance critical path where the mapping is
problematic I would prefer to see a translation layer to help protect
SELinux.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms

[Stephen Smalley]

On Mon, Apr 27, 2020 at 10:13 AM David Howells <dhowells@redhat.com> wrote:
>
> Paul Moore <paul@paul-moore.com> wrote:
>
> > Okay, can you send the next version of the patch to the SELinux list for
> > review?
>
> Here you go.  Note that I did this a few days ago and I actually used EACCES
> rather than EPERM.  Which one is one preferred for this?
>
> David
> ---
> selinux: Fix use of KEY_NEED_* instead of KEY__* perms
>
> selinux_key_getsecurity() is passing the KEY_NEED_* permissions to
> security_sid_to_context() instead of the KEY__* values.  It happens to work
> because the values are all coincident.

Both function names in the above description are wrong.

> Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
> Reported-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>  security/selinux/hooks.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..6087955b49d8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6539,20 +6539,38 @@ static void selinux_key_free(struct key *k)
>         kfree(ksec);
>  }
>
> +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> +{
> +       switch (need_perm) {
> +       case KEY_NEED_VIEW:     return KEY__VIEW;
> +       case KEY_NEED_READ:     return KEY__READ;
> +       case KEY_NEED_WRITE:    return KEY__WRITE;
> +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> +       case KEY_NEED_LINK:     return KEY__LINK;
> +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> +       default:

Possibly WARN() or BUG() here?  Or BUILD_BUG_ON(KEY_NEED_ALL != 0x3f)
to force an update here
whenever a new key permission is defined?

> +               return 0;
> +       }
> +}
> +
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 unsigned need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> +       unsigned int perm;
>         u32 sid;
>
>         /* if no specific permissions are requested, we skip the
>            permission check. No serious, additional covert channels
>            appear to be created. */
> -       if (perm == 0)
> +       if (need_perm == 0)
>                 return 0;
>
> +       perm = selinux_keyperm_to_av(need_perm);
> +       if (perm == 0)
> +               return -EACCES;

We should log or audit some kind of message here, whether via WARN(),
audit_log(), or something, to avoid silent denials.

>         sid = cred_sid(cred);
>
>         key = key_ref_to_ptr(key_ref);
>

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms

[Paul Moore]

On Mon, Apr 27, 2020 at 1:02 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Apr 27, 2020 at 10:13 AM David Howells <dhowells@redhat.com> wrote:

...

> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0b4e32161b77..6087955b49d8 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6539,20 +6539,38 @@ static void selinux_key_free(struct key *k)
> >         kfree(ksec);
> >  }
> >
> > +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> > +{
> > +       switch (need_perm) {
> > +       case KEY_NEED_VIEW:     return KEY__VIEW;
> > +       case KEY_NEED_READ:     return KEY__READ;
> > +       case KEY_NEED_WRITE:    return KEY__WRITE;
> > +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> > +       case KEY_NEED_LINK:     return KEY__LINK;
> > +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> > +       default:
>
> Possibly WARN() or BUG() here?  Or BUILD_BUG_ON(KEY_NEED_ALL != 0x3f)
> to force an update here
> whenever a new key permission is defined?

My vote is for a build time check via BUILD_BUG_ON() or similar
mechanism.  It's worked really well in other places, I'm thinking
specifically of the SELinux netlink mapping.

> > +               return 0;
> > +       }
> > +}
> > +
> >  static int selinux_key_permission(key_ref_t key_ref,
> >                                   const struct cred *cred,
> > -                                 unsigned perm)
> > +                                 unsigned need_perm)
> >  {
> >         struct key *key;
> >         struct key_security_struct *ksec;
> > +       unsigned int perm;
> >         u32 sid;
> >
> >         /* if no specific permissions are requested, we skip the
> >            permission check. No serious, additional covert channels
> >            appear to be created. */
> > -       if (perm == 0)
> > +       if (need_perm == 0)
> >                 return 0;
> >
> > +       perm = selinux_keyperm_to_av(need_perm);
> > +       if (perm == 0)
> > +               return -EACCES;
>
> We should log or audit some kind of message here, whether via WARN(),
> audit_log(), or something, to avoid silent denials.

Assuming we add a build time check (see above), I think a WARN here is okay.

-- 
paul moore
www.paul-moore.com

[PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2]

[David Howells]

selinux: Fix use of KEY_NEED_* instead of KEY__* perms
    
selinux_key_permission() is passing the KEY_NEED_* permissions to
avc_has_perm() instead of the KEY__* values.  It happens to work because
the values are all coincident.

Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
Reported-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
 security/selinux/hooks.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..4b6624e5dab4 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6539,20 +6539,39 @@ static void selinux_key_free(struct key *k)
 	kfree(ksec);
 }
 
+static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
+{
+	switch (need_perm) {
+	case KEY_NEED_VIEW:	return KEY__VIEW;
+	case KEY_NEED_READ:	return KEY__READ;
+	case KEY_NEED_WRITE:	return KEY__WRITE;
+	case KEY_NEED_SEARCH:	return KEY__SEARCH;
+	case KEY_NEED_LINK:	return KEY__LINK;
+	case KEY_NEED_SETATTR:	return KEY__SETATTR;
+	default:
+		WARN_ON(1);
+		return 0;
+	}
+}
+
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  unsigned need_perm)
 {
 	struct key *key;
 	struct key_security_struct *ksec;
+	unsigned int perm;
 	u32 sid;
 
 	/* if no specific permissions are requested, we skip the
 	   permission check. No serious, additional covert channels
 	   appear to be created. */
-	if (perm == 0)
+	if (need_perm == 0)
 		return 0;
 
+	perm = selinux_keyperm_to_av(need_perm);
+	if (perm == 0)
+		return -EPERM;
 	sid = cred_sid(cred);
 
 	key = key_ref_to_ptr(key_ref);

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2]

[Stephen Smalley]

On Tue, Apr 28, 2020 at 8:54 AM David Howells <dhowells@redhat.com> wrote:
>
> selinux: Fix use of KEY_NEED_* instead of KEY__* perms
>
> selinux_key_permission() is passing the KEY_NEED_* permissions to
> avc_has_perm() instead of the KEY__* values.  It happens to work because
> the values are all coincident.
>
> Fixes: d720024e94de ("[PATCH] selinux: add hooks for key subsystem")
> Reported-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>  security/selinux/hooks.c |   23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..4b6624e5dab4 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6539,20 +6539,39 @@ static void selinux_key_free(struct key *k)
>         kfree(ksec);
>  }
>
> +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> +{
> +       switch (need_perm) {
> +       case KEY_NEED_VIEW:     return KEY__VIEW;
> +       case KEY_NEED_READ:     return KEY__READ;
> +       case KEY_NEED_WRITE:    return KEY__WRITE;
> +       case KEY_NEED_SEARCH:   return KEY__SEARCH;
> +       case KEY_NEED_LINK:     return KEY__LINK;
> +       case KEY_NEED_SETATTR:  return KEY__SETATTR;
> +       default:
> +               WARN_ON(1);
> +               return 0;
> +       }
> +}
> +
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 unsigned need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> +       unsigned int perm;
>         u32 sid;
>
>         /* if no specific permissions are requested, we skip the
>            permission check. No serious, additional covert channels
>            appear to be created. */
> -       if (perm == 0)
> +       if (need_perm == 0)
>                 return 0;
>
> +       perm = selinux_keyperm_to_av(need_perm);
> +       if (perm == 0)
> +               return -EPERM;
>         sid = cred_sid(cred);
>
>         key = key_ref_to_ptr(key_ref);

1) Are we guaranteed that the caller only ever passes a single
KEY_NEED_* perm at a time (i.e. hook is never called with a bitmask
of multiple permissions)?  Where is that guarantee enforced?

2) We had talked about adding a BUILD_BUG_ON() or other build-time
guard to ensure that new KEY_NEED_* permissions
are not added without updating SELinux.  We already have similar
constructs for catching new capabilities (#if CAP_LAST_CAP > 63 #error
...), socket address families (#if PF_MAX > 45 #error ...),  RTM_* and
XFRM_MSG* values.

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2]

[David Howells]

Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> 1) Are we guaranteed that the caller only ever passes a single
> KEY_NEED_* perm at a time (i.e. hook is never called with a bitmask
> of multiple permissions)?  Where is that guarantee enforced?

Currently it's the case that only one perm is ever used at once.  I'm tempted
to enforce this by switching the KEY_NEED_* to an enum rather than a bitmask.

I'm not sure how I would actually define the meaning of two perms being OR'd
together.  Either okay?  Both required?

> 2) We had talked about adding a BUILD_BUG_ON() or other build-time
> guard

That doesn't help you trap unallowed perm combinations, though.

> to ensure that new KEY_NEED_* permissions
> are not added without updating SELinux.  We already have similar
> constructs for catching new capabilities (#if CAP_LAST_CAP > 63 #error
> ...), socket address families (#if PF_MAX > 45 #error ...),  RTM_* and
> XFRM_MSG* values.

David

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2]

[Stephen Smalley]

On Tue, Apr 28, 2020 at 11:58 AM David Howells <dhowells@redhat.com> wrote:
>
> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>
> > 1) Are we guaranteed that the caller only ever passes a single
> > KEY_NEED_* perm at a time (i.e. hook is never called with a bitmask
> > of multiple permissions)?  Where is that guarantee enforced?
>
> Currently it's the case that only one perm is ever used at once.  I'm tempted
> to enforce this by switching the KEY_NEED_* to an enum rather than a bitmask.
>
> I'm not sure how I would actually define the meaning of two perms being OR'd
> together.  Either okay?  Both required?

Both required is the usual convention in functions like
inode_permission() or avc_has_perm().
But if you know that you'll never use combinations, we should just
prohibit it up front, e.g.
key_task_permission() or whatever can reject them before they reach
the hook call.  Then the
hook code doesn't have to revisit the issue.

>
> > 2) We had talked about adding a BUILD_BUG_ON() or other build-time
> > guard
>
> That doesn't help you trap unallowed perm combinations, though.

I think we want both.

>
> > to ensure that new KEY_NEED_* permissions
> > are not added without updating SELinux.  We already have similar
> > constructs for catching new capabilities (#if CAP_LAST_CAP > 63 #error
> > ...), socket address families (#if PF_MAX > 45 #error ...),  RTM_* and
> > XFRM_MSG* values.
>
> David
>

Re: [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2]

[Paul Moore]

On Tue, Apr 28, 2020 at 12:19 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Apr 28, 2020 at 11:58 AM David Howells <dhowells@redhat.com> wrote:
> > Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> >
> > > 1) Are we guaranteed that the caller only ever passes a single
> > > KEY_NEED_* perm at a time (i.e. hook is never called with a bitmask
> > > of multiple permissions)?  Where is that guarantee enforced?
> >
> > Currently it's the case that only one perm is ever used at once.  I'm tempted
> > to enforce this by switching the KEY_NEED_* to an enum rather than a bitmask.
> >
> > I'm not sure how I would actually define the meaning of two perms being OR'd
> > together.  Either okay?  Both required?
>
> Both required is the usual convention in functions like
> inode_permission() or avc_has_perm().
> But if you know that you'll never use combinations, we should just
> prohibit it up front, e.g.
> key_task_permission() or whatever can reject them before they reach
> the hook call.  Then the
> hook code doesn't have to revisit the issue.
>
> >
> > > 2) We had talked about adding a BUILD_BUG_ON() or other build-time
> > > guard
> >
> > That doesn't help you trap unallowed perm combinations, though.
>
> I think we want both.

Yep, we want both. #moarsafety

-- 
paul moore
www.paul-moore.com

[PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[David Howells]

Since the meaning of combining the KEY_NEED_* constants is undefined, make
it so that you can't do that by turning them into an enum.

The enum is also given some extra values to represent special
circumstances, such as:

 (1) The '0' value is reserved and causes a warning to trap the parameter
     being unset.

 (2) The key is to be unlinked and we require no permissions on it, only
     the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).

 (3) An override due to CAP_SYS_ADMIN.

 (4) An override due to an instantiation token being present.

 (5) The permissions check is being deferred to later key_permission()
     calls.

The extra values give the opportunity for LSMs to audit these situations.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
cc: Paul Moore <paul@paul-moore.com>
cc: Stephen Smalley <stephen.smalley.work@gmail.com>
cc: Casey Schaufler <casey@schaufler-ca.com>
cc: keyrings@vger.kernel.org
cc: selinux@vger.kernel.org
---

 include/linux/key.h          |   30 ++++++++++++++++-----------
 include/linux/security.h     |    6 +++--
 security/keys/internal.h     |    8 ++++---
 security/keys/keyctl.c       |   16 ++++++++-------
 security/keys/permission.c   |   31 ++++++++++++++++++++++------
 security/keys/process_keys.c |   46 ++++++++++++++++++++----------------------
 security/security.c          |    6 +++--
 security/selinux/hooks.c     |   25 ++++++++++++++++-------
 security/smack/smack_lsm.c   |   31 +++++++++++++++++++++-------
 9 files changed, 124 insertions(+), 75 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index b99b40db08fc..0f2e24f13c2b 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -71,6 +71,23 @@ struct net;
 
 #define KEY_PERM_UNDEF	0xffffffff
 
+/*
+ * The permissions required on a key that we're looking up.
+ */
+enum key_need_perm {
+	KEY_NEED_UNSPECIFIED,	/* Needed permission unspecified */
+	KEY_NEED_VIEW,		/* Require permission to view attributes */
+	KEY_NEED_READ,		/* Require permission to read content */
+	KEY_NEED_WRITE,		/* Require permission to update / modify */
+	KEY_NEED_SEARCH,	/* Require permission to search (keyring) or find (key) */
+	KEY_NEED_LINK,		/* Require permission to link */
+	KEY_NEED_SETATTR,	/* Require permission to change attributes */
+	KEY_NEED_UNLINK,	/* Require permission to unlink key */
+	KEY_SYSADMIN_OVERRIDE,	/* Special: override by CAP_SYS_ADMIN */
+	KEY_AUTHTOKEN_OVERRIDE,	/* Special: override by possession of auth token */
+	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
+};
+
 struct seq_file;
 struct user_struct;
 struct signal_struct;
@@ -420,20 +437,9 @@ static inline key_serial_t key_serial(const struct key *key)
 extern void key_set_timeout(struct key *, unsigned);
 
 extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
-				 key_perm_t perm);
+				 enum key_need_perm need_perm);
 extern void key_free_user_ns(struct user_namespace *);
 
-/*
- * The permissions required on a key that we're looking up.
- */
-#define	KEY_NEED_VIEW	0x01	/* Require permission to view attributes */
-#define	KEY_NEED_READ	0x02	/* Require permission to read content */
-#define	KEY_NEED_WRITE	0x04	/* Require permission to update / modify */
-#define	KEY_NEED_SEARCH	0x08	/* Require permission to search (keyring) or find (key) */
-#define	KEY_NEED_LINK	0x10	/* Require permission to link */
-#define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
-#define	KEY_NEED_ALL	0x3f	/* All the above permissions */
-
 static inline short key_read_state(const struct key *key)
 {
 	/* Barrier versus mark_key_instantiated(). */
diff --git a/include/linux/security.h b/include/linux/security.h
index e7914e4e0b02..57aac14e3418 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1767,8 +1767,8 @@ static inline int security_path_chroot(const struct path *path)
 
 int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
 void security_key_free(struct key *key);
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm);
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm);
 int security_key_getsecurity(struct key *key, char **_buffer);
 
 #else
@@ -1786,7 +1786,7 @@ static inline void security_key_free(struct key *key)
 
 static inline int security_key_permission(key_ref_t key_ref,
 					  const struct cred *cred,
-					  unsigned perm)
+					  enum key_need_perm need_perm)
 {
 	return 0;
 }
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 28e17f4f3328..1fc17cb317a9 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -167,7 +167,6 @@ extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
 #define KEY_LOOKUP_CREATE	0x01
 #define KEY_LOOKUP_PARTIAL	0x02
-#define KEY_LOOKUP_FOR_UNLINK	0x04
 
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);
@@ -183,7 +182,7 @@ extern void key_gc_keytype(struct key_type *ktype);
 
 extern int key_task_permission(const key_ref_t key_ref,
 			       const struct cred *cred,
-			       key_perm_t perm);
+			       enum key_need_perm need_perm);
 
 static inline void notify_key(struct key *key,
 			      enum key_notification_subtype subtype, u32 aux)
@@ -205,9 +204,10 @@ static inline void notify_key(struct key *key,
 /*
  * Check to see whether permission is granted to use a key in the desired way.
  */
-static inline int key_permission(const key_ref_t key_ref, unsigned perm)
+static inline int key_permission(const key_ref_t key_ref,
+				 enum key_need_perm need_perm)
 {
-	return key_task_permission(key_ref, current_cred(), perm);
+	return key_task_permission(key_ref, current_cred(), need_perm);
 }
 
 extern struct key_type key_type_request_key_auth;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 7d8de1c9a478..6763ee45e04d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -434,7 +434,7 @@ long keyctl_invalidate_key(key_serial_t id)
 
 		/* Root is permitted to invalidate certain special keys */
 		if (capable(CAP_SYS_ADMIN)) {
-			key_ref = lookup_user_key(id, 0, 0);
+			key_ref = lookup_user_key(id, 0, KEY_SYSADMIN_OVERRIDE);
 			if (IS_ERR(key_ref))
 				goto error;
 			if (test_bit(KEY_FLAG_ROOT_CAN_INVAL,
@@ -479,7 +479,8 @@ long keyctl_keyring_clear(key_serial_t ringid)
 
 		/* Root is permitted to invalidate certain special keyrings */
 		if (capable(CAP_SYS_ADMIN)) {
-			keyring_ref = lookup_user_key(ringid, 0, 0);
+			keyring_ref = lookup_user_key(ringid, 0,
+						      KEY_SYSADMIN_OVERRIDE);
 			if (IS_ERR(keyring_ref))
 				goto error;
 			if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR,
@@ -563,7 +564,7 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 		goto error;
 	}
 
-	key_ref = lookup_user_key(id, KEY_LOOKUP_FOR_UNLINK, 0);
+	key_ref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_UNLINK);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error2;
@@ -663,7 +664,7 @@ long keyctl_describe_key(key_serial_t keyid,
 				key_put(instkey);
 				key_ref = lookup_user_key(keyid,
 							  KEY_LOOKUP_PARTIAL,
-							  0);
+							  KEY_AUTHTOKEN_OVERRIDE);
 				if (!IS_ERR(key_ref))
 					goto okay;
 			}
@@ -833,7 +834,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 	size_t key_data_len;
 
 	/* find the key first */
-	key_ref = lookup_user_key(keyid, 0, 0);
+	key_ref = lookup_user_key(keyid, 0, KEY_DEFER_PERM_CHECK);
 	if (IS_ERR(key_ref)) {
 		ret = -ENOKEY;
 		goto out;
@@ -1471,7 +1472,7 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout)
 				key_put(instkey);
 				key_ref = lookup_user_key(id,
 							  KEY_LOOKUP_PARTIAL,
-							  0);
+							  KEY_AUTHTOKEN_OVERRIDE);
 				if (!IS_ERR(key_ref))
 					goto okay;
 			}
@@ -1579,7 +1580,8 @@ long keyctl_get_security(key_serial_t keyid,
 			return PTR_ERR(instkey);
 		key_put(instkey);
 
-		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, 0);
+		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL,
+					  KEY_AUTHTOKEN_OVERRIDE);
 		if (IS_ERR(key_ref))
 			return PTR_ERR(key_ref);
 	}
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 085f907b64ac..4a61f804e80f 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -13,7 +13,7 @@
  * key_task_permission - Check a key can be used
  * @key_ref: The key to check.
  * @cred: The credentials to use.
- * @perm: The permissions to check for.
+ * @need_perm: The permission required.
  *
  * Check to see whether permission is granted to use a key in the desired way,
  * but permit the security modules to override.
@@ -24,12 +24,30 @@
  * permissions bits or the LSM check.
  */
 int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
-			unsigned perm)
+			enum key_need_perm need_perm)
 {
 	struct key *key;
-	key_perm_t kperm;
+	key_perm_t kperm, mask;
 	int ret;
 
+	switch (need_perm) {
+	default:
+		WARN_ON(1);
+		return -EACCES;
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
+		goto lsm;
+
+	case KEY_NEED_VIEW:	mask = KEY_OTH_VIEW;	break;
+	case KEY_NEED_READ:	mask = KEY_OTH_READ;	break;
+	case KEY_NEED_WRITE:	mask = KEY_OTH_WRITE;	break;
+	case KEY_NEED_SEARCH:	mask = KEY_OTH_SEARCH;	break;
+	case KEY_NEED_LINK:	mask = KEY_OTH_LINK;	break;
+	case KEY_NEED_SETATTR:	mask = KEY_OTH_SETATTR;	break;
+	}
+
 	key = key_ref_to_ptr(key_ref);
 
 	/* use the second 8-bits of permissions for keys the caller owns */
@@ -64,13 +82,12 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 	if (is_key_possessed(key_ref))
 		kperm |= key->perm >> 24;
 
-	kperm = kperm & perm & KEY_NEED_ALL;
-
-	if (kperm != perm)
+	if ((kperm & mask) != mask)
 		return -EACCES;
 
 	/* let LSM be the final arbiter */
-	return security_key_permission(key_ref, cred, perm);
+lsm:
+	return security_key_permission(key_ref, cred, need_perm);
 }
 EXPORT_SYMBOL(key_task_permission);
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 09541de31f2f..7e0232db1707 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -609,7 +609,7 @@ bool lookup_user_key_possessed(const struct key *key,
  * returned key reference.
  */
 key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
-			  key_perm_t perm)
+			  enum key_need_perm need_perm)
 {
 	struct keyring_search_context ctx = {
 		.match_data.cmp		= lookup_user_key_possessed,
@@ -773,35 +773,33 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 
 	/* unlink does not use the nominated key in any way, so can skip all
 	 * the permission checks as it is only concerned with the keyring */
-	if (lflags & KEY_LOOKUP_FOR_UNLINK) {
-		ret = 0;
-		goto error;
-	}
-
-	if (!(lflags & KEY_LOOKUP_PARTIAL)) {
-		ret = wait_for_key_construction(key, true);
-		switch (ret) {
-		case -ERESTARTSYS:
-			goto invalid_key;
-		default:
-			if (perm)
+	if (need_perm != KEY_NEED_UNLINK) {
+		if (!(lflags & KEY_LOOKUP_PARTIAL)) {
+			ret = wait_for_key_construction(key, true);
+			switch (ret) {
+			case -ERESTARTSYS:
+				goto invalid_key;
+			default:
+				if (need_perm != KEY_AUTHTOKEN_OVERRIDE &&
+				    need_perm != KEY_DEFER_PERM_CHECK)
+					goto invalid_key;
+			case 0:
+				break;
+			}
+		} else if (need_perm != KEY_DEFER_PERM_CHECK) {
+			ret = key_validate(key);
+			if (ret < 0)
 				goto invalid_key;
-		case 0:
-			break;
 		}
-	} else if (perm) {
-		ret = key_validate(key);
-		if (ret < 0)
+
+		ret = -EIO;
+		if (!(lflags & KEY_LOOKUP_PARTIAL) &&
+		    key_read_state(key) == KEY_IS_UNINSTANTIATED)
 			goto invalid_key;
 	}
 
-	ret = -EIO;
-	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
-	    key_read_state(key) == KEY_IS_UNINSTANTIATED)
-		goto invalid_key;
-
 	/* check the permissions */
-	ret = key_task_permission(key_ref, ctx.cred, perm);
+	ret = key_task_permission(key_ref, ctx.cred, need_perm);
 	if (ret < 0)
 		goto invalid_key;
 
diff --git a/security/security.c b/security/security.c
index c73334ab2882..af32d4cd0462 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2398,10 +2398,10 @@ void security_key_free(struct key *key)
 	call_void_hook(key_free, key);
 }
 
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm)
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm)
 {
-	return call_int_hook(key_permission, 0, key_ref, cred, perm);
+	return call_int_hook(key_permission, 0, key_ref, cred, need_perm);
 }
 
 int security_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..3ff6b6dfc5ca 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6541,20 +6541,31 @@ static void selinux_key_free(struct key *k)
 
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  enum key_need_perm need_perm)
 {
 	struct key *key;
 	struct key_security_struct *ksec;
-	u32 sid;
+	u32 perm, sid;
 
-	/* if no specific permissions are requested, we skip the
-	   permission check. No serious, additional covert channels
-	   appear to be created. */
-	if (perm == 0)
+	switch (need_perm) {
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
 		return 0;
+	default:
+		WARN_ON(1);
+		return -EPERM;
 
-	sid = cred_sid(cred);
+	case KEY_NEED_VIEW:	perm = KEY__VIEW;	break;
+	case KEY_NEED_READ:	perm = KEY__READ;	break;
+	case KEY_NEED_WRITE:	perm = KEY__WRITE;	break;
+	case KEY_NEED_SEARCH:	perm = KEY__SEARCH;	break;
+	case KEY_NEED_LINK:	perm = KEY__LINK;	break;
+	case KEY_NEED_SETATTR:	perm = KEY__SETATTR;	break;
+	}
 
+	sid = cred_sid(cred);
 	key = key_ref_to_ptr(key_ref);
 	ksec = key->security;
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8c61d175e195..627ca7dc9b27 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4230,13 +4230,14 @@ static void smack_key_free(struct key *key)
  * smack_key_permission - Smack access on a key
  * @key_ref: gets to the object
  * @cred: the credentials to use
- * @perm: requested key permissions
+ * @need_perm: requested key permission
  *
  * Return 0 if the task has read and write to the object,
  * an error code otherwise
  */
 static int smack_key_permission(key_ref_t key_ref,
-				const struct cred *cred, unsigned perm)
+				const struct cred *cred,
+				enum key_need_perm need_perm)
 {
 	struct key *keyp;
 	struct smk_audit_info ad;
@@ -4247,8 +4248,26 @@ static int smack_key_permission(key_ref_t key_ref,
 	/*
 	 * Validate requested permissions
 	 */
-	if (perm & ~KEY_NEED_ALL)
-		return -EINVAL;
+	switch (need_perm) {
+	default:
+		return -EACCES;
+	case KEY_NEED_UNSPECIFIED:
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
+		return 0;
+	case KEY_NEED_READ:
+	case KEY_NEED_SEARCH:
+	case KEY_NEED_VIEW:
+		request |= MAY_READ;
+		break;
+	case KEY_NEED_WRITE:
+	case KEY_NEED_LINK:
+	case KEY_NEED_SETATTR:
+		request |= MAY_WRITE;
+		break;
+	}
 
 	keyp = key_ref_to_ptr(key_ref);
 	if (keyp == NULL)
@@ -4273,10 +4292,6 @@ static int smack_key_permission(key_ref_t key_ref,
 	ad.a.u.key_struct.key = keyp->serial;
 	ad.a.u.key_struct.key_desc = keyp->description;
 #endif
-	if (perm & (KEY_NEED_READ | KEY_NEED_SEARCH | KEY_NEED_VIEW))
-		request |= MAY_READ;
-	if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
-		request |= MAY_WRITE;
 	rc = smk_access(tkp, keyp->security, request, &ad);
 	rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
 	return rc;

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Paul Moore]

On Tue, May 12, 2020 at 6:33 PM David Howells <dhowells@redhat.com> wrote:
> Since the meaning of combining the KEY_NEED_* constants is undefined, make
> it so that you can't do that by turning them into an enum.
>
> The enum is also given some extra values to represent special
> circumstances, such as:
>
>  (1) The '0' value is reserved and causes a warning to trap the parameter
>      being unset.
>
>  (2) The key is to be unlinked and we require no permissions on it, only
>      the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).
>
>  (3) An override due to CAP_SYS_ADMIN.
>
>  (4) An override due to an instantiation token being present.
>
>  (5) The permissions check is being deferred to later key_permission()
>      calls.
>
> The extra values give the opportunity for LSMs to audit these situations.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> cc: Paul Moore <paul@paul-moore.com>
> cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> cc: Casey Schaufler <casey@schaufler-ca.com>
> cc: keyrings@vger.kernel.org
> cc: selinux@vger.kernel.org
> ---
>
>  include/linux/key.h          |   30 ++++++++++++++++-----------
>  include/linux/security.h     |    6 +++--
>  security/keys/internal.h     |    8 ++++---
>  security/keys/keyctl.c       |   16 ++++++++-------
>  security/keys/permission.c   |   31 ++++++++++++++++++++++------
>  security/keys/process_keys.c |   46 ++++++++++++++++++++----------------------
>  security/security.c          |    6 +++--
>  security/selinux/hooks.c     |   25 ++++++++++++++++-------
>  security/smack/smack_lsm.c   |   31 +++++++++++++++++++++-------
>  9 files changed, 124 insertions(+), 75 deletions(-)

Thanks for clarifying this, it helps a lot.

My comments below are nitpicky, but take them into account, the style
of the SELinux code changes makes my eyes hurt.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..3ff6b6dfc5ca 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6541,20 +6541,31 @@ static void selinux_key_free(struct key *k)
>
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 enum key_need_perm need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> -       u32 sid;
> +       u32 perm, sid;
>
> -       /* if no specific permissions are requested, we skip the
> -          permission check. No serious, additional covert channels
> -          appear to be created. */
> -       if (perm == 0)
> +       switch (need_perm) {
> +       case KEY_NEED_UNLINK:
> +       case KEY_SYSADMIN_OVERRIDE:
> +       case KEY_AUTHTOKEN_OVERRIDE:
> +       case KEY_DEFER_PERM_CHECK:
>                 return 0;
> +       default:
> +               WARN_ON(1);
> +               return -EPERM;

Please move the default case to the bottom of the switch statement.

> -       sid = cred_sid(cred);
> +       case KEY_NEED_VIEW:     perm = KEY__VIEW;       break;
> +       case KEY_NEED_READ:     perm = KEY__READ;       break;
> +       case KEY_NEED_WRITE:    perm = KEY__WRITE;      break;
> +       case KEY_NEED_SEARCH:   perm = KEY__SEARCH;     break;
> +       case KEY_NEED_LINK:     perm = KEY__LINK;       break;
> +       case KEY_NEED_SETATTR:  perm = KEY__SETATTR;    break;

Please don't put the case statements all on one line, use the more
traditional multi-line format.  For example:

  case KEY_NEED_SETATTR:
          perm = KEY__SETATTR;
          break;

> +       }
>
> +       sid = cred_sid(cred);
>         key = key_ref_to_ptr(key_ref);
>         ksec = key->security;

-- 
paul moore
www.paul-moore.com

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Stephen Smalley]

On Tue, May 12, 2020 at 6:33 PM David Howells <dhowells@redhat.com> wrote:
>
> Since the meaning of combining the KEY_NEED_* constants is undefined, make
> it so that you can't do that by turning them into an enum.
>
> The enum is also given some extra values to represent special
> circumstances, such as:
>
>  (1) The '0' value is reserved and causes a warning to trap the parameter
>      being unset.
>
>  (2) The key is to be unlinked and we require no permissions on it, only
>      the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).
>
>  (3) An override due to CAP_SYS_ADMIN.

CAP_SYS_ADMIN should never skip SELinux checking.  Even for Smack,
there is a separate capability (CAP_MAC_ADMIN) for that purpose.

>  (4) An override due to an instantiation token being present.

Not sure what this means but again we shouldn't skip SELinux checking
based on mere possession of an object capability (not a POSIX
capability).

>
>  (5) The permissions check is being deferred to later key_permission()
>      calls.
>
> The extra values give the opportunity for LSMs to audit these situations.
> ---

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 7d8de1c9a478..6763ee45e04d 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -434,7 +434,7 @@ long keyctl_invalidate_key(key_serial_t id)
>
>                 /* Root is permitted to invalidate certain special keys */
>                 if (capable(CAP_SYS_ADMIN)) {
> -                       key_ref = lookup_user_key(id, 0, 0);
> +                       key_ref = lookup_user_key(id, 0, KEY_SYSADMIN_OVERRIDE);

It would be better if the permission indicated the actual operation
(e.g. KEY_NEED_INVALIDATE_SPECIAL), and the decision whether to permit
CAP_SYS_ADMIN processes to override was left to the security modules.
SELinux doesn't automatically allow CAP_SYS_ADMIN processes to do
everything.

> @@ -479,7 +479,8 @@ long keyctl_keyring_clear(key_serial_t ringid)
>
>                 /* Root is permitted to invalidate certain special keyrings */
>                 if (capable(CAP_SYS_ADMIN)) {
> -                       keyring_ref = lookup_user_key(ringid, 0, 0);
> +                       keyring_ref = lookup_user_key(ringid, 0,
> +                                                     KEY_SYSADMIN_OVERRIDE);

Ditto.

> @@ -663,7 +664,7 @@ long keyctl_describe_key(key_serial_t keyid,
>                                 key_put(instkey);
>                                 key_ref = lookup_user_key(keyid,
>                                                           KEY_LOOKUP_PARTIAL,
> -                                                         0);
> +                                                         KEY_AUTHTOKEN_OVERRIDE);

Similarly, it would be better if the permission indicated the
operation (e.g. KEY_NEED_DESCRIBE) rather than the means by which it
is being authorized.  A MAC scheme won't allow mere knowledge of a
token/password-capability to permit violation of its policy.

> @@ -1471,7 +1472,7 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout)
>                                 key_put(instkey);
>                                 key_ref = lookup_user_key(id,
>                                                           KEY_LOOKUP_PARTIAL,
> -                                                         0);
> +                                                         KEY_AUTHTOKEN_OVERRIDE);

Ditto.

> @@ -1579,7 +1580,8 @@ long keyctl_get_security(key_serial_t keyid,
>                         return PTR_ERR(instkey);
>                 key_put(instkey);
>
> -               key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, 0);
> +               key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL,
> +                                         KEY_AUTHTOKEN_OVERRIDE);

Ditto

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..3ff6b6dfc5ca 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6541,20 +6541,31 @@ static void selinux_key_free(struct key *k)
>
>  static int selinux_key_permission(key_ref_t key_ref,
>                                   const struct cred *cred,
> -                                 unsigned perm)
> +                                 enum key_need_perm need_perm)
>  {
>         struct key *key;
>         struct key_security_struct *ksec;
> -       u32 sid;
> +       u32 perm, sid;
>
> -       /* if no specific permissions are requested, we skip the
> -          permission check. No serious, additional covert channels
> -          appear to be created. */
> -       if (perm == 0)
> +       switch (need_perm) {
> +       case KEY_NEED_UNLINK:
> +       case KEY_SYSADMIN_OVERRIDE:
> +       case KEY_AUTHTOKEN_OVERRIDE:
> +       case KEY_DEFER_PERM_CHECK:
>                 return 0;

We really shouldn't be skipping any/all checking on CAP_SYS_ADMIN or
an AUTHTOKEN; those should still be subject to MAC policy.

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Casey Schaufler]

On 5/12/2020 3:33 PM, David Howells wrote:
> Since the meaning of combining the KEY_NEED_* constants is undefined, make
> it so that you can't do that by turning them into an enum.
>
> The enum is also given some extra values to represent special
> circumstances, such as:
>
>  (1) The '0' value is reserved and causes a warning to trap the parameter
>      being unset.
>
>  (2) The key is to be unlinked and we require no permissions on it, only
>      the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).
>
>  (3) An override due to CAP_SYS_ADMIN.
>
>  (4) An override due to an instantiation token being present.
>
>  (5) The permissions check is being deferred to later key_permission()
>      calls.
>
> The extra values give the opportunity for LSMs to audit these situations.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> cc: Paul Moore <paul@paul-moore.com>
> cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> cc: Casey Schaufler <casey@schaufler-ca.com>
> cc: keyrings@vger.kernel.org
> cc: selinux@vger.kernel.org
> ---
>
>  include/linux/key.h          |   30 ++++++++++++++++-----------
>  include/linux/security.h     |    6 +++--
>  security/keys/internal.h     |    8 ++++---
>  security/keys/keyctl.c       |   16 ++++++++-------
>  security/keys/permission.c   |   31 ++++++++++++++++++++++------
>  security/keys/process_keys.c |   46 ++++++++++++++++++++----------------------
>  security/security.c          |    6 +++--
>  security/selinux/hooks.c     |   25 ++++++++++++++++-------
>  security/smack/smack_lsm.c   |   31 +++++++++++++++++++++-------
>  9 files changed, 124 insertions(+), 75 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index b99b40db08fc..0f2e24f13c2b 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -71,6 +71,23 @@ struct net;
>  
>  #define KEY_PERM_UNDEF	0xffffffff
>  
> +/*
> + * The permissions required on a key that we're looking up.
> + */
> +enum key_need_perm {
> +	KEY_NEED_UNSPECIFIED,	/* Needed permission unspecified */
> +	KEY_NEED_VIEW,		/* Require permission to view attributes */
> +	KEY_NEED_READ,		/* Require permission to read content */
> +	KEY_NEED_WRITE,		/* Require permission to update / modify */
> +	KEY_NEED_SEARCH,	/* Require permission to search (keyring) or find (key) */
> +	KEY_NEED_LINK,		/* Require permission to link */
> +	KEY_NEED_SETATTR,	/* Require permission to change attributes */
> +	KEY_NEED_UNLINK,	/* Require permission to unlink key */
> +	KEY_SYSADMIN_OVERRIDE,	/* Special: override by CAP_SYS_ADMIN */
> +	KEY_AUTHTOKEN_OVERRIDE,	/* Special: override by possession of auth token */
> +	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
> +};
> +
>  struct seq_file;
>  struct user_struct;
>  struct signal_struct;
> @@ -420,20 +437,9 @@ static inline key_serial_t key_serial(const struct key *key)
>  extern void key_set_timeout(struct key *, unsigned);
>  
>  extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
> -				 key_perm_t perm);
> +				 enum key_need_perm need_perm);
>  extern void key_free_user_ns(struct user_namespace *);
>  
> -/*
> - * The permissions required on a key that we're looking up.
> - */
> -#define	KEY_NEED_VIEW	0x01	/* Require permission to view attributes */
> -#define	KEY_NEED_READ	0x02	/* Require permission to read content */
> -#define	KEY_NEED_WRITE	0x04	/* Require permission to update / modify */
> -#define	KEY_NEED_SEARCH	0x08	/* Require permission to search (keyring) or find (key) */
> -#define	KEY_NEED_LINK	0x10	/* Require permission to link */
> -#define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
> -#define	KEY_NEED_ALL	0x3f	/* All the above permissions */
> -
>  static inline short key_read_state(const struct key *key)
>  {
>  	/* Barrier versus mark_key_instantiated(). */
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e7914e4e0b02..57aac14e3418 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1767,8 +1767,8 @@ static inline int security_path_chroot(const struct path *path)
>  
>  int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
>  void security_key_free(struct key *key);
> -int security_key_permission(key_ref_t key_ref,
> -			    const struct cred *cred, unsigned perm);
> +int security_key_permission(key_ref_t key_ref, const struct cred *cred,
> +			    enum key_need_perm need_perm);
>  int security_key_getsecurity(struct key *key, char **_buffer);
>  
>  #else
> @@ -1786,7 +1786,7 @@ static inline void security_key_free(struct key *key)
>  
>  static inline int security_key_permission(key_ref_t key_ref,
>  					  const struct cred *cred,
> -					  unsigned perm)
> +					  enum key_need_perm need_perm)
>  {
>  	return 0;
>  }
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 28e17f4f3328..1fc17cb317a9 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -167,7 +167,6 @@ extern bool lookup_user_key_possessed(const struct key *key,
>  				      const struct key_match_data *match_data);
>  #define KEY_LOOKUP_CREATE	0x01
>  #define KEY_LOOKUP_PARTIAL	0x02
> -#define KEY_LOOKUP_FOR_UNLINK	0x04
>  
>  extern long join_session_keyring(const char *name);
>  extern void key_change_session_keyring(struct callback_head *twork);
> @@ -183,7 +182,7 @@ extern void key_gc_keytype(struct key_type *ktype);
>  
>  extern int key_task_permission(const key_ref_t key_ref,
>  			       const struct cred *cred,
> -			       key_perm_t perm);
> +			       enum key_need_perm need_perm);
>  
>  static inline void notify_key(struct key *key,
>  			      enum key_notification_subtype subtype, u32 aux)
> @@ -205,9 +204,10 @@ static inline void notify_key(struct key *key,
>  /*
>   * Check to see whether permission is granted to use a key in the desired way.
>   */
> -static inline int key_permission(const key_ref_t key_ref, unsigned perm)
> +static inline int key_permission(const key_ref_t key_ref,
> +				 enum key_need_perm need_perm)
>  {
> -	return key_task_permission(key_ref, current_cred(), perm);
> +	return key_task_permission(key_ref, current_cred(), need_perm);
>  }
>  
>  extern struct key_type key_type_request_key_auth;
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 7d8de1c9a478..6763ee45e04d 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -434,7 +434,7 @@ long keyctl_invalidate_key(key_serial_t id)
>  
>  		/* Root is permitted to invalidate certain special keys */
>  		if (capable(CAP_SYS_ADMIN)) {
> -			key_ref = lookup_user_key(id, 0, 0);
> +			key_ref = lookup_user_key(id, 0, KEY_SYSADMIN_OVERRIDE);
>  			if (IS_ERR(key_ref))
>  				goto error;
>  			if (test_bit(KEY_FLAG_ROOT_CAN_INVAL,
> @@ -479,7 +479,8 @@ long keyctl_keyring_clear(key_serial_t ringid)
>  
>  		/* Root is permitted to invalidate certain special keyrings */
>  		if (capable(CAP_SYS_ADMIN)) {
> -			keyring_ref = lookup_user_key(ringid, 0, 0);
> +			keyring_ref = lookup_user_key(ringid, 0,
> +						      KEY_SYSADMIN_OVERRIDE);
>  			if (IS_ERR(keyring_ref))
>  				goto error;
>  			if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR,
> @@ -563,7 +564,7 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
>  		goto error;
>  	}
>  
> -	key_ref = lookup_user_key(id, KEY_LOOKUP_FOR_UNLINK, 0);
> +	key_ref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_UNLINK);
>  	if (IS_ERR(key_ref)) {
>  		ret = PTR_ERR(key_ref);
>  		goto error2;
> @@ -663,7 +664,7 @@ long keyctl_describe_key(key_serial_t keyid,
>  				key_put(instkey);
>  				key_ref = lookup_user_key(keyid,
>  							  KEY_LOOKUP_PARTIAL,
> -							  0);
> +							  KEY_AUTHTOKEN_OVERRIDE);
>  				if (!IS_ERR(key_ref))
>  					goto okay;
>  			}
> @@ -833,7 +834,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  	size_t key_data_len;
>  
>  	/* find the key first */
> -	key_ref = lookup_user_key(keyid, 0, 0);
> +	key_ref = lookup_user_key(keyid, 0, KEY_DEFER_PERM_CHECK);
>  	if (IS_ERR(key_ref)) {
>  		ret = -ENOKEY;
>  		goto out;
> @@ -1471,7 +1472,7 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout)
>  				key_put(instkey);
>  				key_ref = lookup_user_key(id,
>  							  KEY_LOOKUP_PARTIAL,
> -							  0);
> +							  KEY_AUTHTOKEN_OVERRIDE);
>  				if (!IS_ERR(key_ref))
>  					goto okay;
>  			}
> @@ -1579,7 +1580,8 @@ long keyctl_get_security(key_serial_t keyid,
>  			return PTR_ERR(instkey);
>  		key_put(instkey);
>  
> -		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, 0);
> +		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL,
> +					  KEY_AUTHTOKEN_OVERRIDE);
>  		if (IS_ERR(key_ref))
>  			return PTR_ERR(key_ref);
>  	}
> diff --git a/security/keys/permission.c b/security/keys/permission.c
> index 085f907b64ac..4a61f804e80f 100644
> --- a/security/keys/permission.c
> +++ b/security/keys/permission.c
> @@ -13,7 +13,7 @@
>   * key_task_permission - Check a key can be used
>   * @key_ref: The key to check.
>   * @cred: The credentials to use.
> - * @perm: The permissions to check for.
> + * @need_perm: The permission required.
>   *
>   * Check to see whether permission is granted to use a key in the desired way,
>   * but permit the security modules to override.
> @@ -24,12 +24,30 @@
>   * permissions bits or the LSM check.
>   */
>  int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
> -			unsigned perm)
> +			enum key_need_perm need_perm)
>  {
>  	struct key *key;
> -	key_perm_t kperm;
> +	key_perm_t kperm, mask;
>  	int ret;
>  
> +	switch (need_perm) {
> +	default:
> +		WARN_ON(1);
> +		return -EACCES;
> +	case KEY_NEED_UNLINK:
> +	case KEY_SYSADMIN_OVERRIDE:
> +	case KEY_AUTHTOKEN_OVERRIDE:
> +	case KEY_DEFER_PERM_CHECK:
> +		goto lsm;
> +
> +	case KEY_NEED_VIEW:	mask = KEY_OTH_VIEW;	break;
> +	case KEY_NEED_READ:	mask = KEY_OTH_READ;	break;
> +	case KEY_NEED_WRITE:	mask = KEY_OTH_WRITE;	break;
> +	case KEY_NEED_SEARCH:	mask = KEY_OTH_SEARCH;	break;
> +	case KEY_NEED_LINK:	mask = KEY_OTH_LINK;	break;
> +	case KEY_NEED_SETATTR:	mask = KEY_OTH_SETATTR;	break;
> +	}
> +
>  	key = key_ref_to_ptr(key_ref);
>  
>  	/* use the second 8-bits of permissions for keys the caller owns */
> @@ -64,13 +82,12 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
>  	if (is_key_possessed(key_ref))
>  		kperm |= key->perm >> 24;
>  
> -	kperm = kperm & perm & KEY_NEED_ALL;
> -
> -	if (kperm != perm)
> +	if ((kperm & mask) != mask)
>  		return -EACCES;
>  
>  	/* let LSM be the final arbiter */
> -	return security_key_permission(key_ref, cred, perm);
> +lsm:
> +	return security_key_permission(key_ref, cred, need_perm);
>  }
>  EXPORT_SYMBOL(key_task_permission);
>  
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 09541de31f2f..7e0232db1707 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -609,7 +609,7 @@ bool lookup_user_key_possessed(const struct key *key,
>   * returned key reference.
>   */
>  key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> -			  key_perm_t perm)
> +			  enum key_need_perm need_perm)
>  {
>  	struct keyring_search_context ctx = {
>  		.match_data.cmp		= lookup_user_key_possessed,
> @@ -773,35 +773,33 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
>  
>  	/* unlink does not use the nominated key in any way, so can skip all
>  	 * the permission checks as it is only concerned with the keyring */
> -	if (lflags & KEY_LOOKUP_FOR_UNLINK) {
> -		ret = 0;
> -		goto error;
> -	}
> -
> -	if (!(lflags & KEY_LOOKUP_PARTIAL)) {
> -		ret = wait_for_key_construction(key, true);
> -		switch (ret) {
> -		case -ERESTARTSYS:
> -			goto invalid_key;
> -		default:
> -			if (perm)
> +	if (need_perm != KEY_NEED_UNLINK) {
> +		if (!(lflags & KEY_LOOKUP_PARTIAL)) {
> +			ret = wait_for_key_construction(key, true);
> +			switch (ret) {
> +			case -ERESTARTSYS:
> +				goto invalid_key;
> +			default:
> +				if (need_perm != KEY_AUTHTOKEN_OVERRIDE &&
> +				    need_perm != KEY_DEFER_PERM_CHECK)
> +					goto invalid_key;
> +			case 0:
> +				break;
> +			}
> +		} else if (need_perm != KEY_DEFER_PERM_CHECK) {
> +			ret = key_validate(key);
> +			if (ret < 0)
>  				goto invalid_key;
> -		case 0:
> -			break;
>  		}
> -	} else if (perm) {
> -		ret = key_validate(key);
> -		if (ret < 0)
> +
> +		ret = -EIO;
> +		if (!(lflags & KEY_LOOKUP_PARTIAL) &&
> +		    key_read_state(key) == KEY_IS_UNINSTANTIATED)
>  			goto invalid_key;
>  	}
>  
> -	ret = -EIO;
> -	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
> -	    key_read_state(key) == KEY_IS_UNINSTANTIATED)
> -		goto invalid_key;
> -
>  	/* check the permissions */
> -	ret = key_task_permission(key_ref, ctx.cred, perm);
> +	ret = key_task_permission(key_ref, ctx.cred, need_perm);
>  	if (ret < 0)
>  		goto invalid_key;
>  
> diff --git a/security/security.c b/security/security.c
> index c73334ab2882..af32d4cd0462 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2398,10 +2398,10 @@ void security_key_free(struct key *key)
>  	call_void_hook(key_free, key);
>  }
>  
> -int security_key_permission(key_ref_t key_ref,
> -			    const struct cred *cred, unsigned perm)
> +int security_key_permission(key_ref_t key_ref, const struct cred *cred,
> +			    enum key_need_perm need_perm)
>  {
> -	return call_int_hook(key_permission, 0, key_ref, cred, perm);
> +	return call_int_hook(key_permission, 0, key_ref, cred, need_perm);
>  }
>  
>  int security_key_getsecurity(struct key *key, char **_buffer)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b77..3ff6b6dfc5ca 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6541,20 +6541,31 @@ static void selinux_key_free(struct key *k)
>  
>  static int selinux_key_permission(key_ref_t key_ref,
>  				  const struct cred *cred,
> -				  unsigned perm)
> +				  enum key_need_perm need_perm)
>  {
>  	struct key *key;
>  	struct key_security_struct *ksec;
> -	u32 sid;
> +	u32 perm, sid;
>  
> -	/* if no specific permissions are requested, we skip the
> -	   permission check. No serious, additional covert channels
> -	   appear to be created. */
> -	if (perm == 0)
> +	switch (need_perm) {
> +	case KEY_NEED_UNLINK:
> +	case KEY_SYSADMIN_OVERRIDE:
> +	case KEY_AUTHTOKEN_OVERRIDE:
> +	case KEY_DEFER_PERM_CHECK:
>  		return 0;
> +	default:
> +		WARN_ON(1);
> +		return -EPERM;
>  
> -	sid = cred_sid(cred);
> +	case KEY_NEED_VIEW:	perm = KEY__VIEW;	break;
> +	case KEY_NEED_READ:	perm = KEY__READ;	break;
> +	case KEY_NEED_WRITE:	perm = KEY__WRITE;	break;
> +	case KEY_NEED_SEARCH:	perm = KEY__SEARCH;	break;
> +	case KEY_NEED_LINK:	perm = KEY__LINK;	break;
> +	case KEY_NEED_SETATTR:	perm = KEY__SETATTR;	break;
> +	}
>  
> +	sid = cred_sid(cred);
>  	key = key_ref_to_ptr(key_ref);
>  	ksec = key->security;
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8c61d175e195..627ca7dc9b27 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4230,13 +4230,14 @@ static void smack_key_free(struct key *key)
>   * smack_key_permission - Smack access on a key
>   * @key_ref: gets to the object
>   * @cred: the credentials to use
> - * @perm: requested key permissions
> + * @need_perm: requested key permission
>   *
>   * Return 0 if the task has read and write to the object,
>   * an error code otherwise
>   */
>  static int smack_key_permission(key_ref_t key_ref,
> -				const struct cred *cred, unsigned perm)
> +				const struct cred *cred,
> +				enum key_need_perm need_perm)
>  {
>  	struct key *keyp;
>  	struct smk_audit_info ad;
> @@ -4247,8 +4248,26 @@ static int smack_key_permission(key_ref_t key_ref,
>  	/*
>  	 * Validate requested permissions
>  	 */
> -	if (perm & ~KEY_NEED_ALL)
> -		return -EINVAL;
> +	switch (need_perm) {
> +	default:
> +		return -EACCES;

Put the default at the end of the switch.
Because that's just the way it's done.

Is the change from -EINVAL to -EACCES a bug fix?
Does it introduce an incompatibility?

> +	case KEY_NEED_UNSPECIFIED:
> +	case KEY_NEED_UNLINK:
> +	case KEY_SYSADMIN_OVERRIDE:
> +	case KEY_AUTHTOKEN_OVERRIDE:
> +	case KEY_DEFER_PERM_CHECK:
> +		return 0;
> +	case KEY_NEED_READ:
> +	case KEY_NEED_SEARCH:
> +	case KEY_NEED_VIEW:
> +		request |= MAY_READ;
> +		break;
> +	case KEY_NEED_WRITE:
> +	case KEY_NEED_LINK:
> +	case KEY_NEED_SETATTR:
> +		request |= MAY_WRITE;
> +		break;
> +	}
>  
>  	keyp = key_ref_to_ptr(key_ref);
>  	if (keyp == NULL)
> @@ -4273,10 +4292,6 @@ static int smack_key_permission(key_ref_t key_ref,
>  	ad.a.u.key_struct.key = keyp->serial;
>  	ad.a.u.key_struct.key_desc = keyp->description;
>  #endif
> -	if (perm & (KEY_NEED_READ | KEY_NEED_SEARCH | KEY_NEED_VIEW))
> -		request |= MAY_READ;
> -	if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
> -		request |= MAY_WRITE;
>  	rc = smk_access(tkp, keyp->security, request, &ad);
>  	rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
>  	return rc;
>
>

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[David Howells]

Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> >  (3) An override due to CAP_SYS_ADMIN.
> 
> CAP_SYS_ADMIN should never skip SELinux checking.  Even for Smack,
> there is a separate capability (CAP_MAC_ADMIN) for that purpose.

The LSM doesn't get consulted at the moment.  With this patch, it will get
consulted.

> >  (4) An override due to an instantiation token being present.
> 
> Not sure what this means but again we shouldn't skip SELinux checking
> based on mere possession of an object capability (not a POSIX
> capability).

The kernel has delegated the instantiation of a key to the calling process and
has given it a temporary key of type ".request_key_auth" which it has put into
force with keyctl(KEYCTL_ASSUME_AUTHORITY).

This authorisation token grants the caller the ability to (a) perform
operations on the key it wouldn't otherwise have permission to do, (b) use the
key instantiation keyctls and (c) temporarily search the keyrings of the
caller of request_key() using the creds of that caller and to read/use the
keys found therein if the caller was permitted to do so.

> It would be better if the permission indicated the actual operation
> (e.g. KEY_NEED_INVALIDATE_SPECIAL), and the decision whether to permit
> CAP_SYS_ADMIN processes to override was left to the security modules.
> SELinux doesn't automatically allow CAP_SYS_ADMIN processes to do
> everything.

These individual permissions don't exist yet.  I have an ACL patchset that
allows me to add a greater range - though there's issues with SELinux there
also.

Also, the keyrings are specially marked to say that the sysadmin is allowed to
flush them at the moment - but that can go away with the ACL stuff.

> > +       switch (need_perm) {
> > +       case KEY_NEED_UNLINK:
> > +       case KEY_SYSADMIN_OVERRIDE:
> > +       case KEY_AUTHTOKEN_OVERRIDE:
> > +       case KEY_DEFER_PERM_CHECK:
> >                 return 0;
> 
> We really shouldn't be skipping any/all checking on CAP_SYS_ADMIN or
> an AUTHTOKEN; those should still be subject to MAC policy.

I'm not sure how to do that.

Note that KEY_NEED_UNLINK *must not* be overruled by the MAC policy.  The
value is only there because lookup_user_key() requires something to be put
into that parameter - it's more of a courtesy thing, I suppose.

Why should AUTHTOKEN be subject to MAC policy?  The kernel has told the
process to go and instantiate a key.  It shouldn't really then turn around and
tell the process "oh, but you're not actually allowed to do that".

David

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Stephen Smalley]

On Wed, May 13, 2020 at 7:13 PM David Howells <dhowells@redhat.com> wrote:
>
> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>
> > >  (3) An override due to CAP_SYS_ADMIN.
> >
> > CAP_SYS_ADMIN should never skip SELinux checking.  Even for Smack,
> > there is a separate capability (CAP_MAC_ADMIN) for that purpose.
>
> The LSM doesn't get consulted at the moment.  With this patch, it will get
> consulted.
>
> > >  (4) An override due to an instantiation token being present.
> >
> > Not sure what this means but again we shouldn't skip SELinux checking
> > based on mere possession of an object capability (not a POSIX
> > capability).
>
> The kernel has delegated the instantiation of a key to the calling process and
> has given it a temporary key of type ".request_key_auth" which it has put into
> force with keyctl(KEYCTL_ASSUME_AUTHORITY).
>
> This authorisation token grants the caller the ability to (a) perform
> operations on the key it wouldn't otherwise have permission to do, (b) use the
> key instantiation keyctls and (c) temporarily search the keyrings of the
> caller of request_key() using the creds of that caller and to read/use the
> keys found therein if the caller was permitted to do so.
>
> > It would be better if the permission indicated the actual operation
> > (e.g. KEY_NEED_INVALIDATE_SPECIAL), and the decision whether to permit
> > CAP_SYS_ADMIN processes to override was left to the security modules.
> > SELinux doesn't automatically allow CAP_SYS_ADMIN processes to do
> > everything.
>
> These individual permissions don't exist yet.  I have an ACL patchset that
> allows me to add a greater range - though there's issues with SELinux there
> also.
>
> Also, the keyrings are specially marked to say that the sysadmin is allowed to
> flush them at the moment - but that can go away with the ACL stuff.
>
> > > +       switch (need_perm) {
> > > +       case KEY_NEED_UNLINK:
> > > +       case KEY_SYSADMIN_OVERRIDE:
> > > +       case KEY_AUTHTOKEN_OVERRIDE:
> > > +       case KEY_DEFER_PERM_CHECK:
> > >                 return 0;
> >
> > We really shouldn't be skipping any/all checking on CAP_SYS_ADMIN or
> > an AUTHTOKEN; those should still be subject to MAC policy.
>
> I'm not sure how to do that.
>
> Note that KEY_NEED_UNLINK *must not* be overruled by the MAC policy.  The
> value is only there because lookup_user_key() requires something to be put
> into that parameter - it's more of a courtesy thing, I suppose.
>
> Why should AUTHTOKEN be subject to MAC policy?  The kernel has told the
> process to go and instantiate a key.  It shouldn't really then turn around and
> tell the process "oh, but you're not actually allowed to do that".

On what basis did the kernel authorize the process to instantiate the
key?  At what point did a security module get involved in the decision
as to what process(es) are authorize to instantiate a key,
particularly for a process with a different credential/security
context?

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Stephen Smalley]

On Thu, May 14, 2020 at 8:08 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, May 13, 2020 at 7:13 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> >
> > > >  (3) An override due to CAP_SYS_ADMIN.
> > >
> > > CAP_SYS_ADMIN should never skip SELinux checking.  Even for Smack,
> > > there is a separate capability (CAP_MAC_ADMIN) for that purpose.
> >
> > The LSM doesn't get consulted at the moment.  With this patch, it will get
> > consulted.
> >
> > > >  (4) An override due to an instantiation token being present.
> > >
> > > Not sure what this means but again we shouldn't skip SELinux checking
> > > based on mere possession of an object capability (not a POSIX
> > > capability).
> >
> > The kernel has delegated the instantiation of a key to the calling process and
> > has given it a temporary key of type ".request_key_auth" which it has put into
> > force with keyctl(KEYCTL_ASSUME_AUTHORITY).
> >
> > This authorisation token grants the caller the ability to (a) perform
> > operations on the key it wouldn't otherwise have permission to do, (b) use the
> > key instantiation keyctls and (c) temporarily search the keyrings of the
> > caller of request_key() using the creds of that caller and to read/use the
> > keys found therein if the caller was permitted to do so.
> >
> > > It would be better if the permission indicated the actual operation
> > > (e.g. KEY_NEED_INVALIDATE_SPECIAL), and the decision whether to permit
> > > CAP_SYS_ADMIN processes to override was left to the security modules.
> > > SELinux doesn't automatically allow CAP_SYS_ADMIN processes to do
> > > everything.
> >
> > These individual permissions don't exist yet.  I have an ACL patchset that
> > allows me to add a greater range - though there's issues with SELinux there
> > also.
> >
> > Also, the keyrings are specially marked to say that the sysadmin is allowed to
> > flush them at the moment - but that can go away with the ACL stuff.
> >
> > > > +       switch (need_perm) {
> > > > +       case KEY_NEED_UNLINK:
> > > > +       case KEY_SYSADMIN_OVERRIDE:
> > > > +       case KEY_AUTHTOKEN_OVERRIDE:
> > > > +       case KEY_DEFER_PERM_CHECK:
> > > >                 return 0;
> > >
> > > We really shouldn't be skipping any/all checking on CAP_SYS_ADMIN or
> > > an AUTHTOKEN; those should still be subject to MAC policy.
> >
> > I'm not sure how to do that.
> >
> > Note that KEY_NEED_UNLINK *must not* be overruled by the MAC policy.  The
> > value is only there because lookup_user_key() requires something to be put
> > into that parameter - it's more of a courtesy thing, I suppose.
> >
> > Why should AUTHTOKEN be subject to MAC policy?  The kernel has told the
> > process to go and instantiate a key.  It shouldn't really then turn around and
> > tell the process "oh, but you're not actually allowed to do that".
>
> On what basis did the kernel authorize the process to instantiate the
> key?  At what point did a security module get involved in the decision
> as to what process(es) are authorize to instantiate a key,
> particularly for a process with a different credential/security
> context?

BTW I'm not saying you have to change this patch since IIUC it merely
preserves the existing behavior for these special cases.  But we will
want to address these gaps (or clarify that no real gap exists) in LSM
control over key operations going forward.

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[David Howells]

Paul Moore <paul@paul-moore.com> wrote:

> > +       case KEY_NEED_VIEW:     perm = KEY__VIEW;       break;
> > +       case KEY_NEED_READ:     perm = KEY__READ;       break;
> > +       case KEY_NEED_WRITE:    perm = KEY__WRITE;      break;
> > +       case KEY_NEED_SEARCH:   perm = KEY__SEARCH;     break;
> > +       case KEY_NEED_LINK:     perm = KEY__LINK;       break;
> > +       case KEY_NEED_SETATTR:  perm = KEY__SETATTR;    break;
> 
> Please don't put the case statements all on one line, use the more
> traditional multi-line format.  For example:
> 
>   case KEY_NEED_SETATTR:
>           perm = KEY__SETATTR;
>           break;

Tabulation was invented something like 6000 years ago for just this kind of
purpose;-)  It's less readable your way, but whatever...

David

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[David Howells]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> > -	if (perm & ~KEY_NEED_ALL)
> > -		return -EINVAL;
> > +	switch (need_perm) {
> > +	default:
> > +		return -EACCES;
> ...
> Is the change from -EINVAL to -EACCES a bug fix?
> Does it introduce an incompatibility?

It shouldn't happen.  All the actual cases should be covered explicitly in the
switch.  It's to catch a programming issue in the kernel where a new value
gets added to the enum but not propagated to all the places that check for it.

I'd actually prefer it to be something even more obvious, especially as EINVAL
is so widely used in the kernel.  Should I put a WARN_ON in there?

David

Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask

[Jarkko Sakkinen]

On Tue, 2020-05-12 at 23:33 +0100, David Howells wrote:
> Since the meaning of combining the KEY_NEED_* constants is undefined, make
> it so that you can't do that by turning them into an enum.
> 
> The enum is also given some extra values to represent special
> circumstances, such as:
> 
>  (1) The '0' value is reserved and causes a warning to trap the parameter
>      being unset.
> 
>  (2) The key is to be unlinked and we require no permissions on it, only
>      the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).
> 
>  (3) An override due to CAP_SYS_ADMIN.
> 
>  (4) An override due to an instantiation token being present.
> 
>  (5) The permissions check is being deferred to later key_permission()
>      calls.
> 
> The extra values give the opportunity for LSMs to audit these situations.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> cc: Paul Moore <paul@paul-moore.com>
> cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> cc: Casey Schaufler <casey@schaufler-ca.com>
> cc: keyrings@vger.kernel.org
> cc: selinux@vger.kernel.org

So extensive comments already from Stephen and Paul that I'll just
wait for the next version (agree with the idea though).

/Jarkko

[PATCH] keys: Move permissions checking decisions into the checking code

[David Howells]

How about this then?

David
---
commit fa37b6c7e2f86d16ede1e0e3cb73857152d51825
Author: David Howells <dhowells@redhat.com>
Date:   Thu May 14 17:48:55 2020 +0100

    keys: Move permissions checking decisions into the checking code
    
    Overhaul the permissions checking, moving the decisions of which permits to
    request for what operation and what overrides to allow into the permissions
    checking functions in keyrings, SELinux and Smack.
    
    To this end, the KEY_NEED_* constants are turned into an enum and expanded
    in number to cover operation types individually.
    
    Note that some more tweaking is probably needed to separate kernel uses,
    e.g. AFS using rxrpc keys, from direct userspace users.
    
    Some overrides are available and this needs to be handled specially:
    
     (1) KEY_FLAG_KEEP in key->flags - The key may not be deleted and/or things
         may not be removed from the keyring.
    
     (2) KEY_FLAG_ROOT_CAN_CLEAR in key->flags - The keyring can be cleared by
         CAP_SYS_ADMIN.
    
     (3) KEY_FLAG_ROOT_CAN_INVAL in key->flags - The key can be invalidated by
         CAP_SYS_ADMIN.
    
     (4) An appropriate auth token being set in cred->request_key_auth that
         gives a process transient permission to view and instantiate a key.
         This is used by the kernel to delegate instantiation to userspace.
    
    Note that this requires some tweaks to the testsuite as some of the error
    codes change.
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
    cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    cc: Paul Moore <paul@paul-moore.com>
    cc: Stephen Smalley <stephen.smalley.work@gmail.com>
    cc: Casey Schaufler <casey@schaufler-ca.com>
    cc: keyrings@vger.kernel.org
    cc: selinux@vger.kernel.org

diff --git a/include/linux/key.h b/include/linux/key.h
index b99b40db08fc..7fb00128c5ba 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -71,6 +71,34 @@ struct net;
 
 #define KEY_PERM_UNDEF	0xffffffff
 
+/*
+ * The permissions required on a key that we're looking up.
+ */
+enum key_need_perm {
+	/* 0 is left undefined */
+	KEY_NEED_ASSUME_AUTHORITY = 1,	/* Want to assume instantiation authority */
+	KEY_NEED_CHOWN,			/* Want to change key's ownership/group */
+	KEY_NEED_DESCRIBE,		/* Want to get a key's attributes */
+	KEY_NEED_GET_SECURITY,		/* Want to get a key's security label */
+	KEY_NEED_INSTANTIATE,		/* Want to instantiate a key */
+	KEY_NEED_INVALIDATE,		/* Want to invalidate key */
+	KEY_NEED_JOIN,			/* Want to set a keyring as the session keyring */
+	KEY_NEED_KEYRING_ADD,		/* Want to add a link to a keyring */
+	KEY_NEED_KEYRING_CLEAR,		/* Want to clear a keyring */
+	KEY_NEED_KEYRING_DELETE,	/* Want to remove a link from a keyring */
+	KEY_NEED_LINK,			/* Want to create a link to a key */
+	KEY_NEED_READ,			/* Want to read content to userspace */
+	KEY_NEED_REVOKE,		/* Want to revoke a key */
+	KEY_NEED_SEARCH,		/* Want to find a key in a search */
+	KEY_NEED_SETPERM,		/* Want to set the permissions mask */
+	KEY_NEED_SET_RESTRICTION,	/* Want to set a restriction on a keyring */
+	KEY_NEED_SET_TIMEOUT,		/* Want to set the expiration time on a key */
+	KEY_NEED_UNLINK,		/* Want to remove a link from a key */
+	KEY_NEED_UPDATE,		/* Want to update a key's payload */
+	KEY_NEED_USE,			/* Want to use a key (in kernel) */
+	KEY_NEED_WATCH,			/* Want to watch a key for events */
+};
+
 struct seq_file;
 struct user_struct;
 struct signal_struct;
@@ -420,20 +448,9 @@ static inline key_serial_t key_serial(const struct key *key)
 extern void key_set_timeout(struct key *, unsigned);
 
 extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
-				 key_perm_t perm);
+				 enum key_need_perm need_perm);
 extern void key_free_user_ns(struct user_namespace *);
 
-/*
- * The permissions required on a key that we're looking up.
- */
-#define	KEY_NEED_VIEW	0x01	/* Require permission to view attributes */
-#define	KEY_NEED_READ	0x02	/* Require permission to read content */
-#define	KEY_NEED_WRITE	0x04	/* Require permission to update / modify */
-#define	KEY_NEED_SEARCH	0x08	/* Require permission to search (keyring) or find (key) */
-#define	KEY_NEED_LINK	0x10	/* Require permission to link */
-#define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
-#define	KEY_NEED_ALL	0x3f	/* All the above permissions */
-
 static inline short key_read_state(const struct key *key)
 {
 	/* Barrier versus mark_key_instantiated(). */
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 733659613bf8..72debb96b002 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -357,7 +357,7 @@ LSM_HOOK(int, 0, key_alloc, struct key *key, const struct cred *cred,
 	 unsigned long flags)
 LSM_HOOK(void, LSM_RET_VOID, key_free, struct key *key)
 LSM_HOOK(int, 0, key_permission, key_ref_t key_ref, const struct cred *cred,
-	 unsigned perm)
+	 enum key_need_perm need_perm, unsigned int flags)
 LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **_buffer)
 #endif /* CONFIG_KEYS */
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3f1374cffb76..1cb01f6d2bed 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1102,6 +1102,14 @@
  *	@cred points to the credentials to provide the context against which to
  *	evaluate the security data on the key.
  *	@perm describes the combination of permissions required of this key.
+ *	@flags indicates any special conditions set in the normal checks, such
+ *	as:
+ *		KEY_PERMISSION_USED_AUTH_OVERRIDE - A lack of permission was
+ *		overridden by the presence of an instantiation authorisation
+ *		token.
+ *		KEY_PERMISSION_USED_SYSADMIN_OVERRIDE - A lack of permission was
+ *		overridden by the presence of a KEY_FLAG_ROOT_CAN_xxx flag on
+ *		the key an the success of a CAP_SYS_ADMIN check.
  *	Return 0 if permission is granted, -ve error otherwise.
  * @key_getsecurity:
  *	Get a textual representation of the security context attached to a key
diff --git a/include/linux/security.h b/include/linux/security.h
index e7914e4e0b02..a6a5fefcf4e0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1762,13 +1762,17 @@ static inline int security_path_chroot(const struct path *path)
 }
 #endif	/* CONFIG_SECURITY_PATH */
 
+/* Flags for security_key_permission() */
+#define KEY_PERMISSION_USED_AUTH_OVERRIDE	0x01 /* Auth token overrode lack of permission */
+#define KEY_PERMISSION_USED_SYSADMIN_OVERRIDE	0x02 /* Sysadmin overrode lack of permission */
+
 #ifdef CONFIG_KEYS
 #ifdef CONFIG_SECURITY
 
 int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
 void security_key_free(struct key *key);
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm);
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm, unsigned int flags);
 int security_key_getsecurity(struct key *key, char **_buffer);
 
 #else
@@ -1786,7 +1790,8 @@ static inline void security_key_free(struct key *key)
 
 static inline int security_key_permission(key_ref_t key_ref,
 					  const struct cred *cred,
-					  unsigned perm)
+					  enum key_need_perm need_perm,
+					  unsigned int flags)
 {
 	return 0;
 }
diff --git a/security/keys/dh.c b/security/keys/dh.c
index c4c629bb1c03..e43731d22310 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -22,10 +22,8 @@ static ssize_t dh_data_from_key(key_serial_t keyid, void **data)
 	ssize_t ret;
 
 	key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
-	if (IS_ERR(key_ref)) {
-		ret = -ENOKEY;
-		goto error;
-	}
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
 	key = key_ref_to_ptr(key_ref);
 
@@ -52,7 +50,6 @@ static ssize_t dh_data_from_key(key_serial_t keyid, void **data)
 	}
 
 	key_put(key);
-error:
 	return ret;
 }
 
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 28e17f4f3328..d97cb9d98dc3 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -108,6 +108,14 @@ extern void __key_link_end(struct key *keyring,
 
 extern key_ref_t find_key_to_update(key_ref_t keyring_ref,
 				    const struct keyring_index_key *index_key);
+extern key_ref_t key_create_or_update_perm_checked(key_ref_t keyring_ref,
+						   const char *type,
+						   const char *description,
+						   const void *payload,
+						   size_t plen,
+						   key_perm_t perm,
+						   unsigned long flags);
+extern int key_update_perm_checked(key_ref_t key_ref, const void *payload, size_t plen);
 
 extern struct key *keyring_search_instkey(struct key *keyring,
 					  key_serial_t target_id);
@@ -165,9 +173,9 @@ extern struct key *request_key_and_link(struct key_type *type,
 
 extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
-#define KEY_LOOKUP_CREATE	0x01
-#define KEY_LOOKUP_PARTIAL	0x02
-#define KEY_LOOKUP_FOR_UNLINK	0x04
+#define KEY_LOOKUP_CREATE		0x01
+#define KEY_LOOKUP_PARTIAL		0x02
+#define KEY_LOOKUP_AUTH_OVERRIDE	0x04
 
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);
@@ -183,7 +191,7 @@ extern void key_gc_keytype(struct key_type *ktype);
 
 extern int key_task_permission(const key_ref_t key_ref,
 			       const struct cred *cred,
-			       key_perm_t perm);
+			       enum key_need_perm need_perm);
 
 static inline void notify_key(struct key *key,
 			      enum key_notification_subtype subtype, u32 aux)
@@ -205,9 +213,10 @@ static inline void notify_key(struct key *key,
 /*
  * Check to see whether permission is granted to use a key in the desired way.
  */
-static inline int key_permission(const key_ref_t key_ref, unsigned perm)
+static inline int key_permission(const key_ref_t key_ref,
+				 enum key_need_perm need_perm)
 {
-	return key_task_permission(key_ref, current_cred(), perm);
+	return key_task_permission(key_ref, current_cred(), need_perm);
 }
 
 extern struct key_type key_type_request_key_auth;
diff --git a/security/keys/key.c b/security/keys/key.c
index e282c6179b21..d77d5dd61d42 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -755,7 +755,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 	int ret;
 
 	/* need write permission on the key to update it */
-	ret = key_permission(key_ref, KEY_NEED_WRITE);
+	ret = key_permission(key_ref, KEY_NEED_UPDATE);
 	if (ret < 0)
 		goto error;
 
@@ -810,13 +810,13 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
  * On success, the possession flag from the keyring ref will be tacked on to
  * the key ref before it is returned.
  */
-key_ref_t key_create_or_update(key_ref_t keyring_ref,
-			       const char *type,
-			       const char *description,
-			       const void *payload,
-			       size_t plen,
-			       key_perm_t perm,
-			       unsigned long flags)
+key_ref_t key_create_or_update_perm_checked(key_ref_t keyring_ref,
+					    const char *type,
+					    const char *description,
+					    const void *payload,
+					    size_t plen,
+					    key_perm_t perm,
+					    unsigned long flags)
 {
 	struct keyring_index_key index_key = {
 		.description	= description,
@@ -894,14 +894,6 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		}
 	}
 
-	/* if we're going to allocate a new key, we're going to have
-	 * to modify the keyring */
-	ret = key_permission(keyring_ref, KEY_NEED_WRITE);
-	if (ret < 0) {
-		key_ref = ERR_PTR(ret);
-		goto error_link_end;
-	}
-
 	/* if it's possible to update this type of key, search for an existing
 	 * key of the same type and description in the destination keyring and
 	 * update that instead if possible
@@ -981,6 +973,27 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 
 	goto error_free_prep;
 }
+
+key_ref_t key_create_or_update(key_ref_t keyring_ref,
+			       const char *type,
+			       const char *description,
+			       const void *payload,
+			       size_t plen,
+			       key_perm_t perm,
+			       unsigned long flags)
+{
+	int ret;
+
+	/* if we're going to allocate a new key, we're going to have
+	 * to modify the keyring */
+	ret = key_permission(keyring_ref, KEY_NEED_KEYRING_ADD);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	return key_create_or_update_perm_checked(keyring_ref, type,
+						 description, payload,
+						 plen, perm, flags);
+}
 EXPORT_SYMBOL(key_create_or_update);
 
 /**
@@ -996,19 +1009,12 @@ EXPORT_SYMBOL(key_create_or_update);
  * Returns 0 on success, -EACCES if not permitted and -EOPNOTSUPP if the key
  * type does not support updating.  The key type may return other errors.
  */
-int key_update(key_ref_t key_ref, const void *payload, size_t plen)
+int key_update_perm_checked(key_ref_t key_ref, const void *payload, size_t plen)
 {
 	struct key_preparsed_payload prep;
 	struct key *key = key_ref_to_ptr(key_ref);
 	int ret;
 
-	key_check(key);
-
-	/* the key must be writable */
-	ret = key_permission(key_ref, KEY_NEED_WRITE);
-	if (ret < 0)
-		return ret;
-
 	/* attempt to update it if supported */
 	if (!key->type->update)
 		return -EOPNOTSUPP;
@@ -1040,6 +1046,20 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 		key->type->free_preparse(&prep);
 	return ret;
 }
+
+int key_update(key_ref_t key_ref, const void *payload, size_t plen)
+{
+	int ret;
+
+	key_check(key_ref_to_ptr(key_ref));
+
+	/* the key must be writable */
+	ret = key_permission(key_ref, KEY_NEED_UPDATE);
+	if (ret < 0)
+		return ret;
+
+	return key_update_perm_checked(key_ref, payload, plen);
+}
 EXPORT_SYMBOL(key_update);
 
 /**
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 7d8de1c9a478..5e25b431a9b5 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -123,7 +123,8 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 	}
 
 	/* find the target keyring (which must be writable) */
-	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+				      KEY_NEED_KEYRING_ADD);
 	if (IS_ERR(keyring_ref)) {
 		ret = PTR_ERR(keyring_ref);
 		goto error3;
@@ -131,9 +132,9 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 
 	/* create or update the requested key and add it to the target
 	 * keyring */
-	key_ref = key_create_or_update(keyring_ref, type, description,
-				       payload, plen, KEY_PERM_UNDEF,
-				       KEY_ALLOC_IN_QUOTA);
+	key_ref = key_create_or_update_perm_checked(keyring_ref, type, description,
+						    payload, plen, KEY_PERM_UNDEF,
+						    KEY_ALLOC_IN_QUOTA);
 	if (!IS_ERR(key_ref)) {
 		ret = key_ref_to_ptr(key_ref)->serial;
 		key_ref_put(key_ref);
@@ -207,7 +208,7 @@ SYSCALL_DEFINE4(request_key, const char __user *, _type,
 	dest_ref = NULL;
 	if (destringid) {
 		dest_ref = lookup_user_key(destringid, KEY_LOOKUP_CREATE,
-					   KEY_NEED_WRITE);
+					   KEY_NEED_KEYRING_ADD);
 		if (IS_ERR(dest_ref)) {
 			ret = PTR_ERR(dest_ref);
 			goto error3;
@@ -351,14 +352,14 @@ long keyctl_update_key(key_serial_t id,
 	}
 
 	/* find the target key (which must be writable) */
-	key_ref = lookup_user_key(id, 0, KEY_NEED_WRITE);
+	key_ref = lookup_user_key(id, 0, KEY_NEED_UPDATE);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error2;
 	}
 
 	/* update the key */
-	ret = key_update(key_ref, payload, plen);
+	ret = key_update_perm_checked(key_ref, payload, plen);
 
 	key_ref_put(key_ref);
 error2:
@@ -382,31 +383,14 @@ long keyctl_update_key(key_serial_t id,
 long keyctl_revoke_key(key_serial_t id)
 {
 	key_ref_t key_ref;
-	struct key *key;
-	long ret;
-
-	key_ref = lookup_user_key(id, 0, KEY_NEED_WRITE);
-	if (IS_ERR(key_ref)) {
-		ret = PTR_ERR(key_ref);
-		if (ret != -EACCES)
-			goto error;
-		key_ref = lookup_user_key(id, 0, KEY_NEED_SETATTR);
-		if (IS_ERR(key_ref)) {
-			ret = PTR_ERR(key_ref);
-			goto error;
-		}
-	}
 
-	key = key_ref_to_ptr(key_ref);
-	ret = 0;
-	if (test_bit(KEY_FLAG_KEEP, &key->flags))
-		ret = -EPERM;
-	else
-		key_revoke(key);
+	key_ref = lookup_user_key(id, 0, KEY_NEED_REVOKE);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
+	key_revoke(key_ref_to_ptr(key_ref));
 	key_ref_put(key_ref);
-error:
-	return ret;
+	return 0;
 }
 
 /*
@@ -423,41 +407,16 @@ long keyctl_revoke_key(key_serial_t id)
 long keyctl_invalidate_key(key_serial_t id)
 {
 	key_ref_t key_ref;
-	struct key *key;
-	long ret;
 
 	kenter("%d", id);
 
-	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
-	if (IS_ERR(key_ref)) {
-		ret = PTR_ERR(key_ref);
-
-		/* Root is permitted to invalidate certain special keys */
-		if (capable(CAP_SYS_ADMIN)) {
-			key_ref = lookup_user_key(id, 0, 0);
-			if (IS_ERR(key_ref))
-				goto error;
-			if (test_bit(KEY_FLAG_ROOT_CAN_INVAL,
-				     &key_ref_to_ptr(key_ref)->flags))
-				goto invalidate;
-			goto error_put;
-		}
-
-		goto error;
-	}
+	key_ref = lookup_user_key(id, 0, KEY_NEED_INVALIDATE);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
-invalidate:
-	key = key_ref_to_ptr(key_ref);
-	ret = 0;
-	if (test_bit(KEY_FLAG_KEEP, &key->flags))
-		ret = -EPERM;
-	else
-		key_invalidate(key);
-error_put:
+	key_invalidate(key_ref_to_ptr(key_ref));
 	key_ref_put(key_ref);
-error:
-	kleave(" = %ld", ret);
-	return ret;
+	return 0;
 }
 
 /*
@@ -470,36 +429,15 @@ long keyctl_invalidate_key(key_serial_t id)
 long keyctl_keyring_clear(key_serial_t ringid)
 {
 	key_ref_t keyring_ref;
-	struct key *keyring;
 	long ret;
 
-	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
-	if (IS_ERR(keyring_ref)) {
-		ret = PTR_ERR(keyring_ref);
+	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+				      KEY_NEED_KEYRING_CLEAR);
+	if (IS_ERR(keyring_ref))
+		return PTR_ERR(keyring_ref);
 
-		/* Root is permitted to invalidate certain special keyrings */
-		if (capable(CAP_SYS_ADMIN)) {
-			keyring_ref = lookup_user_key(ringid, 0, 0);
-			if (IS_ERR(keyring_ref))
-				goto error;
-			if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR,
-				     &key_ref_to_ptr(keyring_ref)->flags))
-				goto clear;
-			goto error_put;
-		}
-
-		goto error;
-	}
-
-clear:
-	keyring = key_ref_to_ptr(keyring_ref);
-	if (test_bit(KEY_FLAG_KEEP, &keyring->flags))
-		ret = -EPERM;
-	else
-		ret = keyring_clear(keyring);
-error_put:
+	ret = keyring_clear(key_ref_to_ptr(keyring_ref));
 	key_ref_put(keyring_ref);
-error:
 	return ret;
 }
 
@@ -519,7 +457,8 @@ long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
 	key_ref_t keyring_ref, key_ref;
 	long ret;
 
-	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+				      KEY_NEED_KEYRING_ADD);
 	if (IS_ERR(keyring_ref)) {
 		ret = PTR_ERR(keyring_ref);
 		goto error;
@@ -554,28 +493,21 @@ long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
 long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 {
 	key_ref_t keyring_ref, key_ref;
-	struct key *keyring, *key;
 	long ret;
 
-	keyring_ref = lookup_user_key(ringid, 0, KEY_NEED_WRITE);
+	keyring_ref = lookup_user_key(ringid, 0, KEY_NEED_KEYRING_DELETE);
 	if (IS_ERR(keyring_ref)) {
 		ret = PTR_ERR(keyring_ref);
 		goto error;
 	}
 
-	key_ref = lookup_user_key(id, KEY_LOOKUP_FOR_UNLINK, 0);
+	key_ref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_UNLINK);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error2;
 	}
 
-	keyring = key_ref_to_ptr(keyring_ref);
-	key = key_ref_to_ptr(key_ref);
-	if (test_bit(KEY_FLAG_KEEP, &keyring->flags) &&
-	    test_bit(KEY_FLAG_KEEP, &key->flags))
-		ret = -EPERM;
-	else
-		ret = key_unlink(keyring, key);
+	ret = key_unlink(key_ref_to_ptr(keyring_ref), key_ref_to_ptr(key_ref));
 
 	key_ref_put(key_ref);
 error2:
@@ -607,13 +539,13 @@ long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
 	if (IS_ERR(key_ref))
 		return PTR_ERR(key_ref);
 
-	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_KEYRING_DELETE);
 	if (IS_ERR(from_ref)) {
 		ret = PTR_ERR(from_ref);
 		goto error2;
 	}
 
-	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_KEYRING_ADD);
 	if (IS_ERR(to_ref)) {
 		ret = PTR_ERR(to_ref);
 		goto error3;
@@ -647,33 +579,21 @@ long keyctl_describe_key(key_serial_t keyid,
 			 char __user *buffer,
 			 size_t buflen)
 {
-	struct key *key, *instkey;
+	struct key *key;
 	key_ref_t key_ref;
 	char *infobuf;
 	long ret;
 	int desclen, infolen;
 
-	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
-	if (IS_ERR(key_ref)) {
-		/* viewing a key under construction is permitted if we have the
-		 * authorisation token handy */
-		if (PTR_ERR(key_ref) == -EACCES) {
-			instkey = key_get_instantiation_authkey(keyid);
-			if (!IS_ERR(instkey)) {
-				key_put(instkey);
-				key_ref = lookup_user_key(keyid,
-							  KEY_LOOKUP_PARTIAL,
-							  0);
-				if (!IS_ERR(key_ref))
-					goto okay;
-			}
-		}
-
-		ret = PTR_ERR(key_ref);
-		goto error;
-	}
+	/* Viewing a key under construction is permitted if we have the
+	 * authorisation token handy.
+	 */
+	key_ref = lookup_user_key(keyid,
+				  KEY_LOOKUP_PARTIAL | KEY_LOOKUP_AUTH_OVERRIDE,
+				  KEY_NEED_DESCRIBE);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
-okay:
 	key = key_ref_to_ptr(key_ref);
 	desclen = strlen(key->description);
 
@@ -685,23 +605,21 @@ long keyctl_describe_key(key_serial_t keyid,
 			    from_kuid_munged(current_user_ns(), key->uid),
 			    from_kgid_munged(current_user_ns(), key->gid),
 			    key->perm);
-	if (!infobuf)
-		goto error2;
-	infolen = strlen(infobuf);
-	ret = infolen + desclen + 1;
-
-	/* consider returning the data */
-	if (buffer && buflen >= ret) {
-		if (copy_to_user(buffer, infobuf, infolen) != 0 ||
-		    copy_to_user(buffer + infolen, key->description,
-				 desclen + 1) != 0)
-			ret = -EFAULT;
-	}
+	if (infobuf) {
+		infolen = strlen(infobuf);
+		ret = infolen + desclen + 1;
+
+		/* consider returning the data */
+		if (buffer && buflen >= ret) {
+			if (copy_to_user(buffer, infobuf, infolen) != 0 ||
+			    copy_to_user(buffer + infolen, key->description,
+					 desclen + 1) != 0)
+				ret = -EFAULT;
+		}
 
-	kfree(infobuf);
-error2:
+		kfree(infobuf);
+	}
 	key_ref_put(key_ref);
-error:
 	return ret;
 }
 
@@ -747,7 +665,7 @@ long keyctl_keyring_search(key_serial_t ringid,
 	dest_ref = NULL;
 	if (destringid) {
 		dest_ref = lookup_user_key(destringid, KEY_LOOKUP_CREATE,
-					   KEY_NEED_WRITE);
+					   KEY_NEED_KEYRING_ADD);
 		if (IS_ERR(dest_ref)) {
 			ret = PTR_ERR(dest_ref);
 			goto error3;
@@ -817,9 +735,6 @@ static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
 /*
  * Read a key's payload.
  *
- * The key must either grant the caller Read permission, or it must grant the
- * caller Search permission when searched for from the process keyrings.
- *
  * If successful, we place up to buflen bytes of data into the buffer, if one
  * is provided, and return the amount of data that is available in the key,
  * irrespective of how much we copied into the buffer.
@@ -833,36 +748,11 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 	size_t key_data_len;
 
 	/* find the key first */
-	key_ref = lookup_user_key(keyid, 0, 0);
-	if (IS_ERR(key_ref)) {
-		ret = -ENOKEY;
-		goto out;
-	}
+	key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
 	key = key_ref_to_ptr(key_ref);
-
-	ret = key_read_state(key);
-	if (ret < 0)
-		goto key_put_out; /* Negatively instantiated */
-
-	/* 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)
-		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;
@@ -929,18 +819,16 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 key_put_out:
 	key_put(key);
-out:
 	return ret;
 }
 
 /*
  * Change the ownership of a key
  *
- * The key must grant the caller Setattr permission for this to work, though
- * the key need not be fully instantiated yet.  For the UID to be changed, or
- * for the GID to be changed to a group the caller is not a member of, the
- * caller must have sysadmin capability.  If either uid or gid is -1 then that
- * attribute is not changed.
+ * The key need not be fully instantiated for this operation to be applied.
+ * For the UID to be changed, or for the GID to be changed to a group the
+ * caller is not a member of, the caller must have sysadmin capability.  If
+ * either uid or gid is -1 then that attribute is not changed.
  *
  * If the UID is to be changed, the new user must have sufficient quota to
  * accept the key.  The quota deduction will be removed from the old user to
@@ -970,7 +858,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 		goto error;
 
 	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
-				  KEY_NEED_SETATTR);
+				  KEY_NEED_CHOWN);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
@@ -1062,9 +950,9 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 /*
  * Change the permission mask on a key.
  *
- * The key must grant the caller Setattr permission for this to work, though
- * the key need not be fully instantiated yet.  If the caller does not have
- * sysadmin capability, it may only change the permission on keys that it owns.
+ * The key doesn't have to be fully instantiated yet for this to work.  If the
+ * caller does not have sysadmin capability, it may only change the permission
+ * on keys that it owns.
  */
 long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
 {
@@ -1077,7 +965,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
 		goto error;
 
 	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
-				  KEY_NEED_SETATTR);
+				  KEY_NEED_SETPERM);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
@@ -1104,7 +992,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
 
 /*
  * Get the destination keyring for instantiation and check that the caller has
- * Write permission on it.
+ * permission to add a key to it.
  */
 static long get_instantiation_keyring(key_serial_t ringid,
 				      struct request_key_auth *rka,
@@ -1120,7 +1008,8 @@ static long get_instantiation_keyring(key_serial_t ringid,
 
 	/* if a specific keyring is nominated by ID, then use that */
 	if (ringid > 0) {
-		dkref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+		dkref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+					KEY_NEED_KEYRING_ADD);
 		if (IS_ERR(dkref))
 			return PTR_ERR(dkref);
 		*_dest_keyring = key_ref_to_ptr(dkref);
@@ -1161,7 +1050,7 @@ static int keyctl_change_reqkey_auth(struct key *key)
  * Instantiate a key with the specified payload and link the key into the
  * destination keyring if one is given.
  *
- * The caller must have the appropriate instantiation permit set for this to
+ * The caller must have the appropriate instantiation token set for this to
  * work (see keyctl_assume_authority).  No other permissions are required.
  *
  * If successful, 0 will be returned.
@@ -1172,29 +1061,34 @@ long keyctl_instantiate_key_common(key_serial_t id,
 {
 	const struct cred *cred = current_cred();
 	struct request_key_auth *rka;
-	struct key *instkey, *dest_keyring;
+	struct key *key, *instkey, *dest_keyring;
+	key_ref_t kref;
 	size_t plen = from ? iov_iter_count(from) : 0;
 	void *payload;
 	long ret;
 
 	kenter("%d,,%zu,%d", id, plen, ringid);
 
+	if (plen > 1024 * 1024 - 1)
+		return -EINVAL;
+
 	if (!plen)
 		from = NULL;
 
-	ret = -EINVAL;
-	if (plen > 1024 * 1024 - 1)
-		goto error;
-
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
-	ret = -EPERM;
 	instkey = cred->request_key_auth;
 	if (!instkey)
-		goto error;
+		return -EPERM;
+
+	kref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_INSTANTIATE);
+	if (IS_ERR(kref))
+		return PTR_ERR(kref);
+	key = key_ref_to_ptr(kref);
 
+	ret = -EPERM;
 	rka = instkey->payload.data[0];
-	if (rka->target_key->serial != id)
+	if (rka->target_key != key)
 		goto error;
 
 	/* pull the payload in if one was supplied */
@@ -1218,7 +1112,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
 		goto error2;
 
 	/* instantiate the key and link it into a keyring */
-	ret = key_instantiate_and_link(rka->target_key, payload, plen,
+	ret = key_instantiate_and_link(key, payload, plen,
 				       dest_keyring, instkey);
 
 	key_put(dest_keyring);
@@ -1234,6 +1128,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
 		kvfree(payload);
 	}
 error:
+	key_put(key);
 	return ret;
 }
 
@@ -1337,7 +1232,8 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 {
 	const struct cred *cred = current_cred();
 	struct request_key_auth *rka;
-	struct key *instkey, *dest_keyring;
+	struct key *key, *instkey, *dest_keyring;
+	key_ref_t kref;
 	long ret;
 
 	kenter("%d,%u,%u,%d", id, timeout, error, ringid);
@@ -1353,13 +1249,18 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
-	ret = -EPERM;
 	instkey = cred->request_key_auth;
 	if (!instkey)
-		goto error;
+		return -EPERM;
+
+	kref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_INSTANTIATE);
+	if (IS_ERR(kref))
+		return PTR_ERR(kref);
+	key = key_ref_to_ptr(kref);
 
+	ret = -EPERM;
 	rka = instkey->payload.data[0];
-	if (rka->target_key->serial != id)
+	if (rka->target_key != key)
 		goto error;
 
 	/* find the destination keyring if present (which must also be
@@ -1380,6 +1281,7 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 		keyctl_change_reqkey_auth(NULL);
 
 error:
+	key_put(key);
 	return ret;
 }
 
@@ -1443,8 +1345,8 @@ long keyctl_set_reqkey_keyring(int reqkey_defl)
 /*
  * Set or clear the timeout on a key.
  *
- * Either the key must grant the caller Setattr permission or else the caller
- * must hold an instantiation authorisation token for the key.
+ * Either the key must grant the caller permission or else the caller must hold
+ * an instantiation authorisation token for the key.
  *
  * The timeout is either 0 to clear the timeout, or a number of seconds from
  * the current time.  The key and any links to the key will be automatically
@@ -1456,44 +1358,25 @@ long keyctl_set_reqkey_keyring(int reqkey_defl)
  */
 long keyctl_set_timeout(key_serial_t id, unsigned timeout)
 {
-	struct key *key, *instkey;
+	struct key *key;
 	key_ref_t key_ref;
-	long ret;
-
-	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
-				  KEY_NEED_SETATTR);
-	if (IS_ERR(key_ref)) {
-		/* setting the timeout on a key under construction is permitted
-		 * if we have the authorisation token handy */
-		if (PTR_ERR(key_ref) == -EACCES) {
-			instkey = key_get_instantiation_authkey(id);
-			if (!IS_ERR(instkey)) {
-				key_put(instkey);
-				key_ref = lookup_user_key(id,
-							  KEY_LOOKUP_PARTIAL,
-							  0);
-				if (!IS_ERR(key_ref))
-					goto okay;
-			}
-		}
 
-		ret = PTR_ERR(key_ref);
-		goto error;
-	}
+	/* Setting the timeout on a key under construction is permitted if we
+	 * have the authorisation token handy
+	 */
+	key_ref = lookup_user_key(id,
+				  KEY_LOOKUP_CREATE |
+				  KEY_LOOKUP_PARTIAL |
+				  KEY_LOOKUP_AUTH_OVERRIDE,
+				  KEY_NEED_SET_TIMEOUT);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
-okay:
 	key = key_ref_to_ptr(key_ref);
-	ret = 0;
-	if (test_bit(KEY_FLAG_KEEP, &key->flags)) {
-		ret = -EPERM;
-	} else {
-		key_set_timeout(key, timeout);
-		notify_key(key, NOTIFY_KEY_SETATTR, 0);
-	}
+	key_set_timeout(key, timeout);
+	notify_key(key, NOTIFY_KEY_SETATTR, 0);
 	key_put(key);
-
-error:
-	return ret;
+	return 0;
 }
 
 /*
@@ -1562,27 +1445,17 @@ long keyctl_get_security(key_serial_t keyid,
 			 char __user *buffer,
 			 size_t buflen)
 {
-	struct key *key, *instkey;
+	struct key *key;
 	key_ref_t key_ref;
 	char *context;
 	long ret;
 
-	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
-	if (IS_ERR(key_ref)) {
-		if (PTR_ERR(key_ref) != -EACCES)
-			return PTR_ERR(key_ref);
-
-		/* viewing a key under construction is also permitted if we
-		 * have the authorisation token handy */
-		instkey = key_get_instantiation_authkey(keyid);
-		if (IS_ERR(instkey))
-			return PTR_ERR(instkey);
-		key_put(instkey);
-
-		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, 0);
-		if (IS_ERR(key_ref))
-			return PTR_ERR(key_ref);
-	}
+	key_ref = lookup_user_key(keyid,
+				  KEY_LOOKUP_PARTIAL |
+				  KEY_LOOKUP_AUTH_OVERRIDE,
+				  KEY_NEED_GET_SECURITY);
+	if (IS_ERR(key_ref))
+		return PTR_ERR(key_ref);
 
 	key = key_ref_to_ptr(key_ref);
 	ret = security_key_getsecurity(key, &context);
@@ -1614,8 +1487,8 @@ long keyctl_get_security(key_serial_t keyid,
  * Attempt to install the calling process's session keyring on the process's
  * parent process.
  *
- * The keyring must exist and must grant the caller LINK permission, and the
- * parent process must be single-threaded and must have the same effective
+ * The keyring must exist and must grant the caller permission to join it, and
+ * the parent process must be single-threaded and must have the same effective
  * ownership as this process and mustn't be SUID/SGID.
  *
  * The keyring will be emplaced on the parent when it next resumes userspace.
@@ -1631,7 +1504,7 @@ long keyctl_session_to_parent(void)
 	struct cred *cred;
 	int ret;
 
-	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_LINK);
+	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_JOIN);
 	if (IS_ERR(keyring_r))
 		return PTR_ERR(keyring_r);
 
@@ -1733,7 +1606,7 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type,
 	char *restriction = NULL;
 	long ret;
 
-	key_ref = lookup_user_key(id, 0, KEY_NEED_SETATTR);
+	key_ref = lookup_user_key(id, 0, KEY_NEED_SET_RESTRICTION);
 	if (IS_ERR(key_ref))
 		return PTR_ERR(key_ref);
 
@@ -1781,7 +1654,7 @@ long keyctl_watch_key(key_serial_t id, int watch_queue_fd, int watch_id)
 	if (watch_id < -1 || watch_id > 0xff)
 		return -EINVAL;
 
-	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_VIEW);
+	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_WATCH);
 	if (IS_ERR(key_ref))
 		return PTR_ERR(key_ref);
 	key = key_ref_to_ptr(key_ref);
diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 931d8dfb4a7f..aece0651eeae 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -77,7 +77,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params)
  */
 static int keyctl_pkey_params_get(key_serial_t id,
 				  const char __user *_info,
-				  struct kernel_pkey_params *params)
+				  struct kernel_pkey_params *params,
+				  enum key_need_perm need_perm)
 {
 	key_ref_t key_ref;
 	void *p;
@@ -95,7 +96,7 @@ static int keyctl_pkey_params_get(key_serial_t id,
 	if (ret < 0)
 		return ret;
 
-	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
+	key_ref = lookup_user_key(id, 0, need_perm);
 	if (IS_ERR(key_ref))
 		return PTR_ERR(key_ref);
 	params->key = key_ref_to_ptr(key_ref);
@@ -113,7 +114,8 @@ static int keyctl_pkey_params_get(key_serial_t id,
 static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_params,
 				    const char __user *_info,
 				    int op,
-				    struct kernel_pkey_params *params)
+				    struct kernel_pkey_params *params,
+				    enum key_need_perm need_perm)
 {
 	struct keyctl_pkey_params uparams;
 	struct kernel_pkey_query info;
@@ -125,7 +127,7 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 	if (copy_from_user(&uparams, _params, sizeof(uparams)) != 0)
 		return -EFAULT;
 
-	ret = keyctl_pkey_params_get(uparams.key_id, _info, params);
+	ret = keyctl_pkey_params_get(uparams.key_id, _info, params, need_perm);
 	if (ret < 0)
 		return ret;
 
@@ -168,7 +170,7 @@ long keyctl_pkey_query(key_serial_t id,
 
 	memset(&params, 0, sizeof(params));
 
-	ret = keyctl_pkey_params_get(id, _info, &params);
+	ret = keyctl_pkey_params_get(id, _info, &params, KEY_NEED_DESCRIBE);
 	if (ret < 0)
 		goto error;
 
@@ -213,7 +215,8 @@ long keyctl_pkey_e_d_s(int op,
 	void *in, *out;
 	long ret;
 
-	ret = keyctl_pkey_params_get_2(_params, _info, op, &params);
+	ret = keyctl_pkey_params_get_2(_params, _info, op, &params,
+				       KEY_NEED_USE);
 	if (ret < 0)
 		goto error_params;
 
@@ -289,7 +292,7 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
 	long ret;
 
 	ret = keyctl_pkey_params_get_2(_params, _info, KEYCTL_PKEY_VERIFY,
-				       &params);
+				       &params, KEY_NEED_USE);
 	if (ret < 0)
 		goto error_params;
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 14abfe765b7e..6199efbe19b4 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1167,7 +1167,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 				continue;
 		} else {
 			if (key_permission(make_key_ref(keyring, 0),
-					   KEY_NEED_SEARCH) < 0)
+					   KEY_NEED_JOIN) < 0)
 				continue;
 		}
 
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 085f907b64ac..ba8d3b548bbc 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -7,13 +7,120 @@
 
 #include <linux/export.h>
 #include <linux/security.h>
+#include <keys/request_key_auth-type.h>
 #include "internal.h"
 
+/*
+ * Determine if we have sufficient permission to perform a check.
+ */
+static int check_key_permission(struct key *key, const struct cred *cred,
+				key_perm_t perms, enum key_need_perm need_perm)
+{
+	struct request_key_auth *rka;
+
+	switch (need_perm) {
+	case KEY_NEED_ASSUME_AUTHORITY:
+		return 0;
+
+	case KEY_NEED_DESCRIBE:
+	case KEY_NEED_GET_SECURITY:
+		if (perms & KEY_OTH_VIEW)
+			return 0;
+		goto check_auth_override;
+
+	case KEY_NEED_CHOWN:
+	case KEY_NEED_SETPERM:
+	case KEY_NEED_SET_RESTRICTION:
+		return perms & KEY_OTH_SETATTR ? 0 : -EACCES;
+
+	case KEY_NEED_INSTANTIATE:
+		goto check_auth_override;
+
+	case KEY_NEED_INVALIDATE:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		if (perms & KEY_OTH_SEARCH)
+			return 0;
+		if (test_bit(KEY_FLAG_ROOT_CAN_INVAL, &key->flags))
+			goto check_sysadmin_override;
+		return -EACCES;
+
+	case KEY_NEED_JOIN:
+	case KEY_NEED_LINK:
+		return perms & KEY_OTH_LINK ? 0 : -EACCES;
+
+	case KEY_NEED_KEYRING_DELETE:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		/* Fall through. */
+	case KEY_NEED_KEYRING_ADD:
+		return perms & KEY_OTH_WRITE ? 0 : -EACCES;
+
+	case KEY_NEED_KEYRING_CLEAR:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		if (perms & KEY_OTH_WRITE)
+			return 0;
+		if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR, &key->flags))
+			goto check_sysadmin_override;
+		return -EACCES;
+
+	case KEY_NEED_READ:
+		return perms & (KEY_OTH_READ | KEY_OTH_SEARCH) ? 0 : -EACCES;
+
+	case KEY_NEED_REVOKE:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		return perms & (KEY_OTH_WRITE | KEY_OTH_SETATTR) ? 0 : -EACCES;
+
+	case KEY_NEED_SEARCH:
+		return perms & KEY_OTH_SEARCH ? 0 : -EACCES;
+
+	case KEY_NEED_SET_TIMEOUT:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		if (perms & KEY_OTH_SETATTR)
+			return 0;
+		goto check_auth_override;
+
+	case KEY_NEED_UNLINK:
+		if (test_bit(KEY_FLAG_KEEP, &key->flags))
+			return -EPERM;
+		return 0;
+
+	case KEY_NEED_UPDATE:
+		return perms & KEY_OTH_WRITE ? 0 : -EACCES;
+
+	case KEY_NEED_USE:
+		return perms & (KEY_OTH_READ | KEY_OTH_SEARCH) ? 0 : -EACCES;
+
+	case KEY_NEED_WATCH:
+		return perms & KEY_OTH_VIEW ? 0 : -EACCES;
+
+	default:
+		WARN_ON(1);
+		return -EACCES;
+	}
+
+check_auth_override:
+	if (!cred->request_key_auth)
+		return -EACCES;
+	rka = cred->request_key_auth->payload.data[0];
+	if (rka->target_key != key)
+		return -EACCES;
+	return KEY_PERMISSION_USED_AUTH_OVERRIDE;
+
+check_sysadmin_override:
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	return KEY_PERMISSION_USED_SYSADMIN_OVERRIDE;
+}
+
 /**
  * key_task_permission - Check a key can be used
  * @key_ref: The key to check.
  * @cred: The credentials to use.
- * @perm: The permissions to check for.
+ * @need_perm: The permission required.
  *
  * Check to see whether permission is granted to use a key in the desired way,
  * but permit the security modules to override.
@@ -24,7 +131,7 @@
  * permissions bits or the LSM check.
  */
 int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
-			unsigned perm)
+			enum key_need_perm need_perm)
 {
 	struct key *key;
 	key_perm_t kperm;
@@ -57,20 +164,18 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 	kperm = key->perm;
 
 use_these_perms:
-
 	/* use the top 8-bits of permissions for keys the caller possesses
 	 * - possessor permissions are additive with other permissions
 	 */
 	if (is_key_possessed(key_ref))
 		kperm |= key->perm >> 24;
 
-	kperm = kperm & perm & KEY_NEED_ALL;
-
-	if (kperm != perm)
-		return -EACCES;
+	ret = check_key_permission(key, cred, kperm & KEY_OTH_ALL, need_perm);
+	if (ret < 0)
+		return ret;
 
-	/* let LSM be the final arbiter */
-	return security_key_permission(key_ref, cred, perm);
+	/* Let the LSMs be the final arbiter */
+	return security_key_permission(key_ref, cred, need_perm, ret);
 }
 EXPORT_SYMBOL(key_task_permission);
 
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index 97af230aa4b2..6131a1528680 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -151,7 +151,7 @@ long keyctl_get_persistent(uid_t _uid, key_serial_t destid)
 	}
 
 	/* There must be a destination keyring */
-	dest_ref = lookup_user_key(destid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	dest_ref = lookup_user_key(destid, KEY_LOOKUP_CREATE, KEY_NEED_KEYRING_ADD);
 	if (IS_ERR(dest_ref))
 		return PTR_ERR(dest_ref);
 	if (key_ref_to_ptr(dest_ref)->type != &key_type_keyring) {
diff --git a/security/keys/proc.c b/security/keys/proc.c
index d0cde6685627..373e62556fa5 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -188,7 +188,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	}
 
 	/* check whether the current task is allowed to view the key */
-	rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW);
+	rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_DESCRIBE);
 	if (rc < 0)
 		return 0;
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 09541de31f2f..e39d9033c34c 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -609,7 +609,7 @@ bool lookup_user_key_possessed(const struct key *key,
  * returned key reference.
  */
 key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
-			  key_perm_t perm)
+			  enum key_need_perm need_perm)
 {
 	struct keyring_search_context ctx = {
 		.match_data.cmp		= lookup_user_key_possessed,
@@ -773,35 +773,24 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 
 	/* unlink does not use the nominated key in any way, so can skip all
 	 * the permission checks as it is only concerned with the keyring */
-	if (lflags & KEY_LOOKUP_FOR_UNLINK) {
-		ret = 0;
-		goto error;
-	}
+	if (need_perm != KEY_NEED_UNLINK) {
+		if (!(lflags & KEY_LOOKUP_PARTIAL)) {
+			ret = wait_for_key_construction(key, true);
+			if (ret < 0)
+				goto invalid_key;
 
-	if (!(lflags & KEY_LOOKUP_PARTIAL)) {
-		ret = wait_for_key_construction(key, true);
-		switch (ret) {
-		case -ERESTARTSYS:
-			goto invalid_key;
-		default:
-			if (perm)
+			ret = -EIO;
+			if (key_read_state(key) == KEY_IS_UNINSTANTIATED)
+				goto invalid_key;
+		} else {
+			ret = key_validate(key);
+			if (ret < 0)
 				goto invalid_key;
-		case 0:
-			break;
 		}
-	} else if (perm) {
-		ret = key_validate(key);
-		if (ret < 0)
-			goto invalid_key;
 	}
 
-	ret = -EIO;
-	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
-	    key_read_state(key) == KEY_IS_UNINSTANTIATED)
-		goto invalid_key;
-
 	/* check the permissions */
-	ret = key_task_permission(key_ref, ctx.cred, perm);
+	ret = key_task_permission(key_ref, ctx.cred, need_perm);
 	if (ret < 0)
 		goto invalid_key;
 
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e1b9f1a80676..c835b7407a5f 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -332,10 +332,10 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
 			BUG();
 		}
 
-		/*
-		 * Require Write permission on the keyring.  This is essential
-		 * because the default keyring may be the session keyring, and
-		 * joining a keyring only requires Search permission.
+		/* Require permission to add a link to the keyring.  This is
+		 * essential because the default keyring may be the session
+		 * keyring, and joining a keyring only requires Search
+		 * permission.
 		 *
 		 * However, this check is skipped for the "requestor keyring" so
 		 * that /sbin/request-key can itself use request_key() to add
@@ -343,7 +343,7 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
 		 */
 		if (dest_keyring && do_perm_check) {
 			ret = key_permission(make_key_ref(dest_keyring, 1),
-					     KEY_NEED_WRITE);
+					     KEY_NEED_KEYRING_ADD);
 			if (ret) {
 				key_put(dest_keyring);
 				return ret;
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 41e9735006d0..588130b631b8 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -258,6 +258,7 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 	};
 	struct key *authkey;
 	key_ref_t authkey_ref;
+	int ret;
 
 	ctx.index_key.desc_len = sprintf(description, "%x", target_id);
 
@@ -272,6 +273,12 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 		goto error;
 	}
 
+	ret = key_permission(authkey_ref, KEY_NEED_ASSUME_AUTHORITY);
+	if (ret < 0) {
+		key_ref_put(authkey_ref);
+		authkey = ERR_PTR(ret);
+	}
+
 	authkey = key_ref_to_ptr(authkey_ref);
 	if (test_bit(KEY_FLAG_REVOKED, &authkey->flags)) {
 		key_put(authkey);
diff --git a/security/security.c b/security/security.c
index c73334ab2882..e5e2200796f7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2398,10 +2398,10 @@ void security_key_free(struct key *key)
 	call_void_hook(key_free, key);
 }
 
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm)
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm, unsigned int flags)
 {
-	return call_int_hook(key_permission, 0, key_ref, cred, perm);
+	return call_int_hook(key_permission, 0, key_ref, cred, need_perm, flags);
 }
 
 int security_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..0761787dee13 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -91,6 +91,7 @@
 #include <uapi/linux/mount.h>
 #include <linux/fsnotify.h>
 #include <linux/fanotify.h>
+#include <keys/request_key_auth-type.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -6539,27 +6540,144 @@ static void selinux_key_free(struct key *k)
 	kfree(ksec);
 }
 
+/*
+ * Convert the requested KEY_NEED_* permit into an SELinux KEY__* permission.
+ *
+ * flags may also convey override flags such as
+ * KEY_PERMISSION_USED_AUTH/SYSADMIN_OVERRIDE to indicate when the main
+ * permission check overrode the permissions on the key.
+ *
+ * Returns the perms to check for in *_perm and *_perm2.  If either perm is
+ * present, then the operation is allowed.
+ */
+static int selinux_keyperm_to_av(struct key *key, const struct cred *cred,
+				 unsigned int need_perm, unsigned int flags,
+				 u32 *_perm, u32 *_perm2)
+{
+	bool auth_can_override = false; /* See KEYCTL_ASSUME_AUTHORITY */
+	bool sysadmin_can_override = false;
+
+	switch (need_perm) {
+	case KEY_NEED_ASSUME_AUTHORITY:
+		return 0;
+
+	case KEY_NEED_DESCRIBE:
+	case KEY_NEED_GET_SECURITY:
+		*_perm = KEY__VIEW;
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_CHOWN:
+	case KEY_NEED_SETPERM:
+	case KEY_NEED_SET_RESTRICTION:
+		*_perm = KEY__SETATTR;
+		break;
+
+	case KEY_NEED_INSTANTIATE:
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_INVALIDATE:
+		*_perm = KEY__SEARCH;
+		if (test_bit(KEY_FLAG_ROOT_CAN_INVAL, &key->flags))
+			sysadmin_can_override = true;
+		break;
+
+	case KEY_NEED_JOIN:
+	case KEY_NEED_LINK:
+		*_perm = KEY__LINK;
+		break;
+
+	case KEY_NEED_KEYRING_ADD:
+	case KEY_NEED_KEYRING_DELETE:
+		*_perm = KEY__WRITE;
+		break;
+
+	case KEY_NEED_KEYRING_CLEAR:
+		*_perm = KEY__WRITE;
+		if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR, &key->flags))
+			sysadmin_can_override = true;
+		break;
+
+	case KEY_NEED_READ:
+		*_perm = KEY__READ;
+		break;
+
+	case KEY_NEED_REVOKE:
+		*_perm = KEY__SETATTR;
+		*_perm2 = KEY__WRITE;
+		break;
+
+	case KEY_NEED_SEARCH:
+		*_perm = KEY__SEARCH;
+		break;
+
+	case KEY_NEED_SET_TIMEOUT:
+		*_perm = KEY__SETATTR;
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_UNLINK:
+		return 0; /* Mustn't prevent this; KEY_FLAG_KEEP is already
+			   * dealt with. */
+
+	case KEY_NEED_UPDATE:
+		*_perm = KEY__WRITE;
+		break;
+
+	case KEY_NEED_USE:
+		*_perm = KEY__READ;
+		*_perm2 = KEY__SEARCH;
+		break;
+
+	case KEY_NEED_WATCH:
+		*_perm = KEY__VIEW;
+		break;
+
+	default:
+		WARN_ON(1);
+		return -EPERM;
+	}
+
+	/* Just allow the operation if the process has an authorisation token.
+	 * The presence of the token means that the kernel delegated
+	 * instantiation of a key to the process - which is problematic if we
+	 * then say that the process isn't allowed to get the description of
+	 * the key or actually instantiate it.
+	 */
+	if (auth_can_override && cred->request_key_auth) {
+		struct request_key_auth *rka =
+			cred->request_key_auth->payload.data[0];
+		if (rka->target_key == key)
+			*_perm = 0;
+	}
+
+	return 0;
+}
+
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  enum key_need_perm need_perm,
+				  unsigned int flags)
 {
-	struct key *key;
-	struct key_security_struct *ksec;
-	u32 sid;
+	struct key *key = key_ref_to_ptr(key_ref);
+	struct key_security_struct *ksec = key->security;
+	u32 sid, perm = 0, perm2 = 0;
+	int ret;
 
-	/* if no specific permissions are requested, we skip the
-	   permission check. No serious, additional covert channels
-	   appear to be created. */
-	if (perm == 0)
-		return 0;
+	ret = selinux_keyperm_to_av(key, cred, need_perm, flags, &perm, &perm2);
+	if (ret < 0 || !perm)
+		return ret;
 
 	sid = cred_sid(cred);
 
-	key = key_ref_to_ptr(key_ref);
-	ksec = key->security;
+	ret = avc_has_perm(&selinux_state,
+			   sid, ksec->sid, SECCLASS_KEY, perm, NULL);
+	if (ret == 0 || !perm2)
+		return ret;
 
 	return avc_has_perm(&selinux_state,
-			    sid, ksec->sid, SECCLASS_KEY, perm, NULL);
+			    sid, ksec->sid, SECCLASS_KEY, perm2, NULL);
 }
 
 static int selinux_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8c61d175e195..ac9c93c48097 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4230,13 +4230,15 @@ static void smack_key_free(struct key *key)
  * smack_key_permission - Smack access on a key
  * @key_ref: gets to the object
  * @cred: the credentials to use
- * @perm: requested key permissions
+ * @need_perm: requested key permission
  *
  * Return 0 if the task has read and write to the object,
  * an error code otherwise
  */
 static int smack_key_permission(key_ref_t key_ref,
-				const struct cred *cred, unsigned perm)
+				const struct cred *cred,
+				enum key_need_perm need_perm,
+				unsigned int flags)
 {
 	struct key *keyp;
 	struct smk_audit_info ad;
@@ -4244,12 +4246,6 @@ static int smack_key_permission(key_ref_t key_ref,
 	int request = 0;
 	int rc;
 
-	/*
-	 * Validate requested permissions
-	 */
-	if (perm & ~KEY_NEED_ALL)
-		return -EINVAL;
-
 	keyp = key_ref_to_ptr(key_ref);
 	if (keyp == NULL)
 		return -EINVAL;
@@ -4265,6 +4261,71 @@ static int smack_key_permission(key_ref_t key_ref,
 	if (tkp == NULL)
 		return -EACCES;
 
+	/*
+	 * Validate requested permissions
+	 */
+	switch (need_perm) {
+	case KEY_NEED_ASSUME_AUTHORITY:
+		return 0;
+
+	case KEY_NEED_DESCRIBE:
+	case KEY_NEED_GET_SECURITY:
+		request |= MAY_READ;
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_CHOWN:
+	case KEY_NEED_INVALIDATE:
+	case KEY_NEED_JOIN:
+	case KEY_NEED_LINK:
+	case KEY_NEED_KEYRING_ADD:
+	case KEY_NEED_KEYRING_CLEAR:
+	case KEY_NEED_KEYRING_DELETE:
+	case KEY_NEED_REVOKE:
+	case KEY_NEED_SETPERM:
+	case KEY_NEED_SET_RESTRICTION:
+	case KEY_NEED_UPDATE:
+		request |= MAY_WRITE;
+		break;
+
+	case KEY_NEED_INSTANTIATE:
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_READ:
+	case KEY_NEED_SEARCH:
+	case KEY_NEED_USE:
+	case KEY_NEED_WATCH:
+		request |= MAY_READ;
+		break;
+
+	case KEY_NEED_SET_TIMEOUT:
+		request |= MAY_WRITE;
+		auth_can_override = true;
+		break;
+
+	case KEY_NEED_UNLINK:
+		return 0; /* Mustn't prevent this; KEY_FLAG_KEEP is already
+			   * dealt with. */
+
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	/* Just allow the operation if the process has an authorisation token.
+	 * The presence of the token means that the kernel delegated
+	 * instantiation of a key to the process - which is problematic if we
+	 * then say that the process isn't allowed to get the description of
+	 * the key or actually instantiate it.
+	 */
+	if (auth_can_override && cred->request_key_auth) {
+		struct request_key_auth *rka =
+			cred->request_key_auth->payload.data[0];
+		if (rka->target_key == key)
+			*_perm = 0;
+	}
+
 	if (smack_privileged_cred(CAP_MAC_OVERRIDE, cred))
 		return 0;
 
@@ -4273,10 +4334,6 @@ static int smack_key_permission(key_ref_t key_ref,
 	ad.a.u.key_struct.key = keyp->serial;
 	ad.a.u.key_struct.key_desc = keyp->description;
 #endif
-	if (perm & (KEY_NEED_READ | KEY_NEED_SEARCH | KEY_NEED_VIEW))
-		request |= MAY_READ;
-	if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
-		request |= MAY_WRITE;
 	rc = smk_access(tkp, keyp->security, request, &ad);
 	rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
 	return rc;

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[Casey Schaufler]

On 5/14/2020 9:58 AM, David Howells wrote:
> How about this then?
>
> David
> ---
> commit fa37b6c7e2f86d16ede1e0e3cb73857152d51825
> Author: David Howells <dhowells@redhat.com>
> Date:   Thu May 14 17:48:55 2020 +0100
>
>     keys: Move permissions checking decisions into the checking code
>     
>     Overhaul the permissions checking, moving the decisions of which permits to
>     request for what operation and what overrides to allow into the permissions
>     checking functions in keyrings, SELinux and Smack.
>     
>     To this end, the KEY_NEED_* constants are turned into an enum and expanded
>     in number to cover operation types individually.
>     
>     Note that some more tweaking is probably needed to separate kernel uses,
>     e.g. AFS using rxrpc keys, from direct userspace users.
>     
>     Some overrides are available and this needs to be handled specially:
>     
>      (1) KEY_FLAG_KEEP in key->flags - The key may not be deleted and/or things
>          may not be removed from the keyring.
>     
>      (2) KEY_FLAG_ROOT_CAN_CLEAR in key->flags - The keyring can be cleared by
>          CAP_SYS_ADMIN.
>     
>      (3) KEY_FLAG_ROOT_CAN_INVAL in key->flags - The key can be invalidated by
>          CAP_SYS_ADMIN.
>     
>      (4) An appropriate auth token being set in cred->request_key_auth that
>          gives a process transient permission to view and instantiate a key.
>          This is used by the kernel to delegate instantiation to userspace.
>     
>     Note that this requires some tweaks to the testsuite as some of the error
>     codes change.
>     
>     Signed-off-by: David Howells <dhowells@redhat.com>
>     Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>     cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>     cc: Paul Moore <paul@paul-moore.com>
>     cc: Stephen Smalley <stephen.smalley.work@gmail.com>
>     cc: Casey Schaufler <casey@schaufler-ca.com>
>     cc: keyrings@vger.kernel.org
>     cc: selinux@vger.kernel.org
>
<snip> ...

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8c61d175e195..ac9c93c48097 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4230,13 +4230,15 @@ static void smack_key_free(struct key *key)
>   * smack_key_permission - Smack access on a key
>   * @key_ref: gets to the object
>   * @cred: the credentials to use
> - * @perm: requested key permissions
> + * @need_perm: requested key permission
>   *
>   * Return 0 if the task has read and write to the object,
>   * an error code otherwise
>   */
>  static int smack_key_permission(key_ref_t key_ref,
> -				const struct cred *cred, unsigned perm)
> +				const struct cred *cred,
> +				enum key_need_perm need_perm,
> +				unsigned int flags)
>  {
>  	struct key *keyp;
>  	struct smk_audit_info ad;
> @@ -4244,12 +4246,6 @@ static int smack_key_permission(key_ref_t key_ref,
>  	int request = 0;
>  	int rc;
>  
> -	/*
> -	 * Validate requested permissions
> -	 */
> -	if (perm & ~KEY_NEED_ALL)
> -		return -EINVAL;
> -
>  	keyp = key_ref_to_ptr(key_ref);
>  	if (keyp == NULL)
>  		return -EINVAL;
> @@ -4265,6 +4261,71 @@ static int smack_key_permission(key_ref_t key_ref,
>  	if (tkp == NULL)
>  		return -EACCES;
>  
> +	/*
> +	 * Validate requested permissions
> +	 */
> +	switch (need_perm) {
> +	case KEY_NEED_ASSUME_AUTHORITY:
> +		return 0;
> +
> +	case KEY_NEED_DESCRIBE:
> +	case KEY_NEED_GET_SECURITY:
> +		request |= MAY_READ;
> +		auth_can_override = true;
> +		break;
> +
> +	case KEY_NEED_CHOWN:
> +	case KEY_NEED_INVALIDATE:
> +	case KEY_NEED_JOIN:
> +	case KEY_NEED_LINK:
> +	case KEY_NEED_KEYRING_ADD:
> +	case KEY_NEED_KEYRING_CLEAR:
> +	case KEY_NEED_KEYRING_DELETE:
> +	case KEY_NEED_REVOKE:
> +	case KEY_NEED_SETPERM:
> +	case KEY_NEED_SET_RESTRICTION:
> +	case KEY_NEED_UPDATE:
> +		request |= MAY_WRITE;
> +		break;
> +
> +	case KEY_NEED_INSTANTIATE:
> +		auth_can_override = true;
> +		break;
> +
> +	case KEY_NEED_READ:
> +	case KEY_NEED_SEARCH:
> +	case KEY_NEED_USE:
> +	case KEY_NEED_WATCH:
> +		request |= MAY_READ;
> +		break;
> +
> +	case KEY_NEED_SET_TIMEOUT:
> +		request |= MAY_WRITE;
> +		auth_can_override = true;
> +		break;
> +
> +	case KEY_NEED_UNLINK:
> +		return 0; /* Mustn't prevent this; KEY_FLAG_KEEP is already
> +			   * dealt with. */
> +
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	/* Just allow the operation if the process has an authorisation token.
> +	 * The presence of the token means that the kernel delegated
> +	 * instantiation of a key to the process - which is problematic if we
> +	 * then say that the process isn't allowed to get the description of
> +	 * the key or actually instantiate it.
> +	 */
> +	if (auth_can_override && cred->request_key_auth) {
> +		struct request_key_auth *rka =
> +			cred->request_key_auth->payload.data[0];
> +		if (rka->target_key == key)
> +			*_perm = 0;
> +	}
> +
>  	if (smack_privileged_cred(CAP_MAC_OVERRIDE, cred))
>  		return 0;
>  
> @@ -4273,10 +4334,6 @@ static int smack_key_permission(key_ref_t key_ref,
>  	ad.a.u.key_struct.key = keyp->serial;
>  	ad.a.u.key_struct.key_desc = keyp->description;
>  #endif
> -	if (perm & (KEY_NEED_READ | KEY_NEED_SEARCH | KEY_NEED_VIEW))
> -		request |= MAY_READ;
> -	if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
> -		request |= MAY_WRITE;
>  	rc = smk_access(tkp, keyp->security, request, &ad);
>  	rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
>  	return rc;

Better. Thank you.

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[Stephen Smalley]

On Thu, May 14, 2020 at 12:59 PM David Howells <dhowells@redhat.com> wrote:
>
> How about this then?
>
> David
> ---
> commit fa37b6c7e2f86d16ede1e0e3cb73857152d51825
> Author: David Howells <dhowells@redhat.com>
> Date:   Thu May 14 17:48:55 2020 +0100
>
>     keys: Move permissions checking decisions into the checking code
>
>     Overhaul the permissions checking, moving the decisions of which permits to
>     request for what operation and what overrides to allow into the permissions
>     checking functions in keyrings, SELinux and Smack.
>
>     To this end, the KEY_NEED_* constants are turned into an enum and expanded
>     in number to cover operation types individually.
>
>     Note that some more tweaking is probably needed to separate kernel uses,
>     e.g. AFS using rxrpc keys, from direct userspace users.
>
>     Some overrides are available and this needs to be handled specially:
>
>      (1) KEY_FLAG_KEEP in key->flags - The key may not be deleted and/or things
>          may not be removed from the keyring.

Why can't they be deleted / removed?  They can't ever be deleted or
removed or for some period of time?

>      (2) KEY_FLAG_ROOT_CAN_CLEAR in key->flags - The keyring can be cleared by
>          CAP_SYS_ADMIN.

Why do some keyrings get this flag and others do not?  Under what
conditions?  Why is CAP_SYS_ADMIN the right capability for this?

>      (3) KEY_FLAG_ROOT_CAN_INVAL in key->flags - The key can be invalidated by
>          CAP_SYS_ADMIN.

Ditto.

>      (4) An appropriate auth token being set in cred->request_key_auth that
>          gives a process transient permission to view and instantiate a key.
>          This is used by the kernel to delegate instantiation to userspace.

Is this ever allowed across different credentials?  When?  Why?  Is
there a check between the different credentials before the auth token
is created?

>     Note that this requires some tweaks to the testsuite as some of the error
>     codes change.

Which testsuite?  keyring or selinux or both?  What error codes change
(from what to what)?  Does this constitute an ABI change?

I like moving more of the permission checking logic into the security
modules and giving them greater visibility and control.  That said, I
am somewhat concerned by the scale of this change, by the extent to
which you are exposing keyring internals inside the security modules,
and by the extent to which logic is getting duplicated in each
security module.  I'd suggest a more incremental approach, e.g. start
with just the enum patch, then migrate the easy cases, then consider
the more complicated cases.  And possibly we need multiple different
security hooks for the keyring subsystem that are more specialized for
the complicated cases.  If we authorize the delegation up front, we
don't need to check it later.

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[David Howells]

Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> >      (1) KEY_FLAG_KEEP in key->flags - The key may not be deleted and/or things
> >          may not be removed from the keyring.
> 
> Why can't they be deleted / removed?  They can't ever be deleted or
> removed or for some period of time?

This is only settable internally to keep special keys, such as the blacklist
loaded from the EFI BIOS, from being removed.

> >      (2) KEY_FLAG_ROOT_CAN_CLEAR in key->flags - The keyring can be cleared by
> >          CAP_SYS_ADMIN.
> 
> Why do some keyrings get this flag and others do not?  Under what
> conditions?  Why is CAP_SYS_ADMIN the right capability for this?
> 
> >      (3) KEY_FLAG_ROOT_CAN_INVAL in key->flags - The key can be invalidated by
> >          CAP_SYS_ADMIN.
> 
> Ditto.

So that the sysadmin can clear, say, the NFS idmapper keyring or invalidate
DNS lookup keys.

> >      (4) An appropriate auth token being set in cred->request_key_auth that
> >          gives a process transient permission to view and instantiate a key.
> >          This is used by the kernel to delegate instantiation to userspace.
> 
> Is this ever allowed across different credentials?

The kernel upcalls by spawning a daemon.  I want to change this as it's not
compatible with containers since namespaces make this problematic.

> When?

The request_key() system call will do this.  The normal use case is something
like the AFS filesystem asking for a key so that it can do an operation.  The
possibility exists for the kernel to upcall, say, to something that does aklog
on behalf of the user - but aklog in turn needs to get the TGT out of the
keyrings.

> Why?  Is there a check between the different credentials before the
> auth token is created?

No.  I don't even know what the target creds will necessarily be at this
point.

> >     Note that this requires some tweaks to the testsuite as some of the
> >     error codes change.
> 
> Which testsuite?  keyring or selinux or both?

The keyring testsuite.  No idea about the SELinux one.

> What error codes change (from what to what)?  Does this constitute an ABI
> change?

The following:

 (1) Passing the wrong type of key to KEYCTL_DH_COMPUTE now gets you
     EOPNOTSUPP rather than ENOKEY.  This is now as documented in the manual
     page.

 (2) Passing key ID 0 or an invalid negative key ID to KEYCTL_DH_COMPUTE now
     gets you EINVAL rather than ENOKEY.

 (3) Passing key ID 0 or an invalid negative key ID to KEYCTL_READ now gets
     you EINVAL rather than ENOKEY.

Technically, it consistutes an ABI change, I suppose, but I think it is
probably sufficiently minor.

Or maybe on (2) and (3) I should go the other way.  You get ENOKEY for invalid
key IDs (such as 0 or unsupported negative ones) across all callers of
lookup_user_key().  This would at least by consistent with the manual pages.

> I like moving more of the permission checking logic into the security
> modules and giving them greater visibility and control.  That said, I
> am somewhat concerned by the scale of this change, by the extent to
> which you are exposing keyring internals inside the security modules,
> and by the extent to which logic is getting duplicated in each
> security module.

It's what you asked for.

Now, I don't know if the LSM needs to know that the main keyutils permissions
checker invoked an override.  At least one of the overrides will have gone
through the LSM anyway when capable() was called.

> I'd suggest a more incremental approach, e.g. start with just the enum
> patch, then migrate the easy cases, then consider the more complicated
> cases.  And possibly we need multiple different security hooks for the
> keyring subsystem that are more specialized for the complicated cases.  If
> we authorize the delegation up front, we don't need to check it later.

I'll consider it.  But I really need to get what I'm going to include in the
middle of the notifications patchset sorted now - or risk the notifications
and fsinfo patchsets getting bumped again.

Maybe what's needed is a pair of hooks whereby the call to capable() is
replaced with LSM hook specifically to ask about the overrides:

	security_key_use_sysadmin_override(key, cred);
	security_key_use_construction_override(key, cred);

And/or a hook to ask whether the process is allowed to do the request_key()
call that they want:

	security_request_key(struct key_type *type,
			     const char *description,
			     struct key_tag *domain_tag,
			     const void *callout_info,
			     size_t callout_len,
			     void *aux);

I don't really want to do a "can the kernel delegate to process X?" hook just
at the moment, since I want to change/extend that code and I don't want to
commit to any particular security information being present yet.

I can go back to the enum patch for the moment if you and Casey can put up
with that for the moment?

David

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[Stephen Smalley]

On Fri, May 15, 2020 at 12:45 PM David Howells <dhowells@redhat.com> wrote:
> I can go back to the enum patch for the moment if you and Casey can put up
> with that for the moment?

Yes, let's do that.

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[Casey Schaufler]

On 5/15/2020 11:55 AM, Stephen Smalley wrote:
> On Fri, May 15, 2020 at 12:45 PM David Howells <dhowells@redhat.com> wrote:
>> I can go back to the enum patch for the moment if you and Casey can put up
>> with that for the moment?
> Yes, let's do that.

OK by me.

Re: [PATCH] keys: Move permissions checking decisions into the checking code

[David Howells]

Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> > I can go back to the enum patch for the moment if you and Casey can put up
> > with that for the moment?
> 
> Yes, let's do that.

Okay.  I'll use the attached.  I've added a note into the commit message to
indicate what should be done in future.  I won't put the other patch into my
keys-next branch yet as it will conflict with this.

David
---
commit e9c98329b2201e6df4edb34dc386e6ef1eeeb78b
Author: David Howells <dhowells@redhat.com>
Date:   Tue May 12 15:16:29 2020 +0100

    keys: Make the KEY_NEED_* perms an enum rather than a mask
    
    Since the meaning of combining the KEY_NEED_* constants is undefined, make
    it so that you can't do that by turning them into an enum.
    
    The enum is also given some extra values to represent special
    circumstances, such as:
    
     (1) The '0' value is reserved and causes a warning to trap the parameter
         being unset.
    
     (2) The key is to be unlinked and we require no permissions on it, only
         the keyring, (this replaces the KEY_LOOKUP_FOR_UNLINK flag).
    
     (3) An override due to CAP_SYS_ADMIN.
    
     (4) An override due to an instantiation token being present.
    
     (5) The permissions check is being deferred to later key_permission()
         calls.
    
    The extra values give the opportunity for LSMs to audit these situations.
    
    [Note: This really needs overhauling so that lookup_user_key() tells
     key_task_permission() and the LSM what operation is being done and leaves
     it to those functions to decide how to map that onto the available
     permits.  However, I don't really want to make these change in the middle
     of the notifications patchset.]
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    cc: Paul Moore <paul@paul-moore.com>
    cc: Stephen Smalley <stephen.smalley.work@gmail.com>
    cc: Casey Schaufler <casey@schaufler-ca.com>
    cc: keyrings@vger.kernel.org
    cc: selinux@vger.kernel.org

diff --git a/include/linux/key.h b/include/linux/key.h
index b99b40db08fc..0f2e24f13c2b 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -71,6 +71,23 @@ struct net;
 
 #define KEY_PERM_UNDEF	0xffffffff
 
+/*
+ * The permissions required on a key that we're looking up.
+ */
+enum key_need_perm {
+	KEY_NEED_UNSPECIFIED,	/* Needed permission unspecified */
+	KEY_NEED_VIEW,		/* Require permission to view attributes */
+	KEY_NEED_READ,		/* Require permission to read content */
+	KEY_NEED_WRITE,		/* Require permission to update / modify */
+	KEY_NEED_SEARCH,	/* Require permission to search (keyring) or find (key) */
+	KEY_NEED_LINK,		/* Require permission to link */
+	KEY_NEED_SETATTR,	/* Require permission to change attributes */
+	KEY_NEED_UNLINK,	/* Require permission to unlink key */
+	KEY_SYSADMIN_OVERRIDE,	/* Special: override by CAP_SYS_ADMIN */
+	KEY_AUTHTOKEN_OVERRIDE,	/* Special: override by possession of auth token */
+	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
+};
+
 struct seq_file;
 struct user_struct;
 struct signal_struct;
@@ -420,20 +437,9 @@ static inline key_serial_t key_serial(const struct key *key)
 extern void key_set_timeout(struct key *, unsigned);
 
 extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
-				 key_perm_t perm);
+				 enum key_need_perm need_perm);
 extern void key_free_user_ns(struct user_namespace *);
 
-/*
- * The permissions required on a key that we're looking up.
- */
-#define	KEY_NEED_VIEW	0x01	/* Require permission to view attributes */
-#define	KEY_NEED_READ	0x02	/* Require permission to read content */
-#define	KEY_NEED_WRITE	0x04	/* Require permission to update / modify */
-#define	KEY_NEED_SEARCH	0x08	/* Require permission to search (keyring) or find (key) */
-#define	KEY_NEED_LINK	0x10	/* Require permission to link */
-#define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
-#define	KEY_NEED_ALL	0x3f	/* All the above permissions */
-
 static inline short key_read_state(const struct key *key)
 {
 	/* Barrier versus mark_key_instantiated(). */
diff --git a/include/linux/security.h b/include/linux/security.h
index e7914e4e0b02..57aac14e3418 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1767,8 +1767,8 @@ static inline int security_path_chroot(const struct path *path)
 
 int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
 void security_key_free(struct key *key);
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm);
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm);
 int security_key_getsecurity(struct key *key, char **_buffer);
 
 #else
@@ -1786,7 +1786,7 @@ static inline void security_key_free(struct key *key)
 
 static inline int security_key_permission(key_ref_t key_ref,
 					  const struct cred *cred,
-					  unsigned perm)
+					  enum key_need_perm need_perm)
 {
 	return 0;
 }
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 28e17f4f3328..1fc17cb317a9 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -167,7 +167,6 @@ extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
 #define KEY_LOOKUP_CREATE	0x01
 #define KEY_LOOKUP_PARTIAL	0x02
-#define KEY_LOOKUP_FOR_UNLINK	0x04
 
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);
@@ -183,7 +182,7 @@ extern void key_gc_keytype(struct key_type *ktype);
 
 extern int key_task_permission(const key_ref_t key_ref,
 			       const struct cred *cred,
-			       key_perm_t perm);
+			       enum key_need_perm need_perm);
 
 static inline void notify_key(struct key *key,
 			      enum key_notification_subtype subtype, u32 aux)
@@ -205,9 +204,10 @@ static inline void notify_key(struct key *key,
 /*
  * Check to see whether permission is granted to use a key in the desired way.
  */
-static inline int key_permission(const key_ref_t key_ref, unsigned perm)
+static inline int key_permission(const key_ref_t key_ref,
+				 enum key_need_perm need_perm)
 {
-	return key_task_permission(key_ref, current_cred(), perm);
+	return key_task_permission(key_ref, current_cred(), need_perm);
 }
 
 extern struct key_type key_type_request_key_auth;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 7d8de1c9a478..6763ee45e04d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -434,7 +434,7 @@ long keyctl_invalidate_key(key_serial_t id)
 
 		/* Root is permitted to invalidate certain special keys */
 		if (capable(CAP_SYS_ADMIN)) {
-			key_ref = lookup_user_key(id, 0, 0);
+			key_ref = lookup_user_key(id, 0, KEY_SYSADMIN_OVERRIDE);
 			if (IS_ERR(key_ref))
 				goto error;
 			if (test_bit(KEY_FLAG_ROOT_CAN_INVAL,
@@ -479,7 +479,8 @@ long keyctl_keyring_clear(key_serial_t ringid)
 
 		/* Root is permitted to invalidate certain special keyrings */
 		if (capable(CAP_SYS_ADMIN)) {
-			keyring_ref = lookup_user_key(ringid, 0, 0);
+			keyring_ref = lookup_user_key(ringid, 0,
+						      KEY_SYSADMIN_OVERRIDE);
 			if (IS_ERR(keyring_ref))
 				goto error;
 			if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR,
@@ -563,7 +564,7 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 		goto error;
 	}
 
-	key_ref = lookup_user_key(id, KEY_LOOKUP_FOR_UNLINK, 0);
+	key_ref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_UNLINK);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error2;
@@ -663,7 +664,7 @@ long keyctl_describe_key(key_serial_t keyid,
 				key_put(instkey);
 				key_ref = lookup_user_key(keyid,
 							  KEY_LOOKUP_PARTIAL,
-							  0);
+							  KEY_AUTHTOKEN_OVERRIDE);
 				if (!IS_ERR(key_ref))
 					goto okay;
 			}
@@ -833,7 +834,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 	size_t key_data_len;
 
 	/* find the key first */
-	key_ref = lookup_user_key(keyid, 0, 0);
+	key_ref = lookup_user_key(keyid, 0, KEY_DEFER_PERM_CHECK);
 	if (IS_ERR(key_ref)) {
 		ret = -ENOKEY;
 		goto out;
@@ -1471,7 +1472,7 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout)
 				key_put(instkey);
 				key_ref = lookup_user_key(id,
 							  KEY_LOOKUP_PARTIAL,
-							  0);
+							  KEY_AUTHTOKEN_OVERRIDE);
 				if (!IS_ERR(key_ref))
 					goto okay;
 			}
@@ -1579,7 +1580,8 @@ long keyctl_get_security(key_serial_t keyid,
 			return PTR_ERR(instkey);
 		key_put(instkey);
 
-		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, 0);
+		key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL,
+					  KEY_AUTHTOKEN_OVERRIDE);
 		if (IS_ERR(key_ref))
 			return PTR_ERR(key_ref);
 	}
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 085f907b64ac..4a61f804e80f 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -13,7 +13,7 @@
  * key_task_permission - Check a key can be used
  * @key_ref: The key to check.
  * @cred: The credentials to use.
- * @perm: The permissions to check for.
+ * @need_perm: The permission required.
  *
  * Check to see whether permission is granted to use a key in the desired way,
  * but permit the security modules to override.
@@ -24,12 +24,30 @@
  * permissions bits or the LSM check.
  */
 int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
-			unsigned perm)
+			enum key_need_perm need_perm)
 {
 	struct key *key;
-	key_perm_t kperm;
+	key_perm_t kperm, mask;
 	int ret;
 
+	switch (need_perm) {
+	default:
+		WARN_ON(1);
+		return -EACCES;
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
+		goto lsm;
+
+	case KEY_NEED_VIEW:	mask = KEY_OTH_VIEW;	break;
+	case KEY_NEED_READ:	mask = KEY_OTH_READ;	break;
+	case KEY_NEED_WRITE:	mask = KEY_OTH_WRITE;	break;
+	case KEY_NEED_SEARCH:	mask = KEY_OTH_SEARCH;	break;
+	case KEY_NEED_LINK:	mask = KEY_OTH_LINK;	break;
+	case KEY_NEED_SETATTR:	mask = KEY_OTH_SETATTR;	break;
+	}
+
 	key = key_ref_to_ptr(key_ref);
 
 	/* use the second 8-bits of permissions for keys the caller owns */
@@ -64,13 +82,12 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 	if (is_key_possessed(key_ref))
 		kperm |= key->perm >> 24;
 
-	kperm = kperm & perm & KEY_NEED_ALL;
-
-	if (kperm != perm)
+	if ((kperm & mask) != mask)
 		return -EACCES;
 
 	/* let LSM be the final arbiter */
-	return security_key_permission(key_ref, cred, perm);
+lsm:
+	return security_key_permission(key_ref, cred, need_perm);
 }
 EXPORT_SYMBOL(key_task_permission);
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 09541de31f2f..7e0232db1707 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -609,7 +609,7 @@ bool lookup_user_key_possessed(const struct key *key,
  * returned key reference.
  */
 key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
-			  key_perm_t perm)
+			  enum key_need_perm need_perm)
 {
 	struct keyring_search_context ctx = {
 		.match_data.cmp		= lookup_user_key_possessed,
@@ -773,35 +773,33 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 
 	/* unlink does not use the nominated key in any way, so can skip all
 	 * the permission checks as it is only concerned with the keyring */
-	if (lflags & KEY_LOOKUP_FOR_UNLINK) {
-		ret = 0;
-		goto error;
-	}
-
-	if (!(lflags & KEY_LOOKUP_PARTIAL)) {
-		ret = wait_for_key_construction(key, true);
-		switch (ret) {
-		case -ERESTARTSYS:
-			goto invalid_key;
-		default:
-			if (perm)
+	if (need_perm != KEY_NEED_UNLINK) {
+		if (!(lflags & KEY_LOOKUP_PARTIAL)) {
+			ret = wait_for_key_construction(key, true);
+			switch (ret) {
+			case -ERESTARTSYS:
+				goto invalid_key;
+			default:
+				if (need_perm != KEY_AUTHTOKEN_OVERRIDE &&
+				    need_perm != KEY_DEFER_PERM_CHECK)
+					goto invalid_key;
+			case 0:
+				break;
+			}
+		} else if (need_perm != KEY_DEFER_PERM_CHECK) {
+			ret = key_validate(key);
+			if (ret < 0)
 				goto invalid_key;
-		case 0:
-			break;
 		}
-	} else if (perm) {
-		ret = key_validate(key);
-		if (ret < 0)
+
+		ret = -EIO;
+		if (!(lflags & KEY_LOOKUP_PARTIAL) &&
+		    key_read_state(key) == KEY_IS_UNINSTANTIATED)
 			goto invalid_key;
 	}
 
-	ret = -EIO;
-	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
-	    key_read_state(key) == KEY_IS_UNINSTANTIATED)
-		goto invalid_key;
-
 	/* check the permissions */
-	ret = key_task_permission(key_ref, ctx.cred, perm);
+	ret = key_task_permission(key_ref, ctx.cred, need_perm);
 	if (ret < 0)
 		goto invalid_key;
 
diff --git a/security/security.c b/security/security.c
index c73334ab2882..af32d4cd0462 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2398,10 +2398,10 @@ void security_key_free(struct key *key)
 	call_void_hook(key_free, key);
 }
 
-int security_key_permission(key_ref_t key_ref,
-			    const struct cred *cred, unsigned perm)
+int security_key_permission(key_ref_t key_ref, const struct cred *cred,
+			    enum key_need_perm need_perm)
 {
-	return call_int_hook(key_permission, 0, key_ref, cred, perm);
+	return call_int_hook(key_permission, 0, key_ref, cred, need_perm);
 }
 
 int security_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b77..0de048705f0e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6541,20 +6541,43 @@ static void selinux_key_free(struct key *k)
 
 static int selinux_key_permission(key_ref_t key_ref,
 				  const struct cred *cred,
-				  unsigned perm)
+				  enum key_need_perm need_perm)
 {
 	struct key *key;
 	struct key_security_struct *ksec;
-	u32 sid;
+	u32 perm, sid;
 
-	/* if no specific permissions are requested, we skip the
-	   permission check. No serious, additional covert channels
-	   appear to be created. */
-	if (perm == 0)
+	switch (need_perm) {
+	case KEY_NEED_VIEW:
+		perm = KEY__VIEW;
+		break;
+	case KEY_NEED_READ:
+		perm = KEY__READ;
+		break;
+	case KEY_NEED_WRITE:
+		perm = KEY__WRITE;
+		break;
+	case KEY_NEED_SEARCH:
+		perm = KEY__SEARCH;
+		break;
+	case KEY_NEED_LINK:
+		perm = KEY__LINK;
+		break;
+	case KEY_NEED_SETATTR:
+		perm = KEY__SETATTR;
+		break;
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
 		return 0;
+	default:
+		WARN_ON(1);
+		return -EPERM;
 
-	sid = cred_sid(cred);
+	}
 
+	sid = cred_sid(cred);
 	key = key_ref_to_ptr(key_ref);
 	ksec = key->security;
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8c61d175e195..0d6bb53efe74 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4230,13 +4230,14 @@ static void smack_key_free(struct key *key)
  * smack_key_permission - Smack access on a key
  * @key_ref: gets to the object
  * @cred: the credentials to use
- * @perm: requested key permissions
+ * @need_perm: requested key permission
  *
  * Return 0 if the task has read and write to the object,
  * an error code otherwise
  */
 static int smack_key_permission(key_ref_t key_ref,
-				const struct cred *cred, unsigned perm)
+				const struct cred *cred,
+				enum key_need_perm need_perm)
 {
 	struct key *keyp;
 	struct smk_audit_info ad;
@@ -4247,8 +4248,26 @@ static int smack_key_permission(key_ref_t key_ref,
 	/*
 	 * Validate requested permissions
 	 */
-	if (perm & ~KEY_NEED_ALL)
+	switch (need_perm) {
+	case KEY_NEED_READ:
+	case KEY_NEED_SEARCH:
+	case KEY_NEED_VIEW:
+		request |= MAY_READ;
+		break;
+	case KEY_NEED_WRITE:
+	case KEY_NEED_LINK:
+	case KEY_NEED_SETATTR:
+		request |= MAY_WRITE;
+		break;
+	case KEY_NEED_UNSPECIFIED:
+	case KEY_NEED_UNLINK:
+	case KEY_SYSADMIN_OVERRIDE:
+	case KEY_AUTHTOKEN_OVERRIDE:
+	case KEY_DEFER_PERM_CHECK:
+		return 0;
+	default:
 		return -EINVAL;
+	}
 
 	keyp = key_ref_to_ptr(key_ref);
 	if (keyp == NULL)
@@ -4273,10 +4292,6 @@ static int smack_key_permission(key_ref_t key_ref,
 	ad.a.u.key_struct.key = keyp->serial;
 	ad.a.u.key_struct.key_desc = keyp->description;
 #endif
-	if (perm & (KEY_NEED_READ | KEY_NEED_SEARCH | KEY_NEED_VIEW))
-		request |= MAY_READ;
-	if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
-		request |= MAY_WRITE;
 	rc = smk_access(tkp, keyp->security, request, &ad);
 	rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
 	return rc;