[PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Eric Biggers]

From: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Currently, a process only needs Search permission on a key to invalidate
it with keyctl_invalidate(), which has an effect similar to unlinking it
from all keyrings.  Unfortunately, this significantly compromises the
keys permission system because as a result, there is no way to grant
read-only access to keys without also granting de-facto "delete" access.
Even worse, processes may invalidate entire _keyrings_, given only
permission to "search" them.  It is not even possible to block this
using SELinux, because SELinux is likewise only asked for Search
permission, which needs to be allowed for read-only access.

Key invalidation seems to have intended for cases where keyrings are
used as caches, and keys can be re-requested at any time by an upcall to
/sbin/request-key.  However, keyrings are not always used in this way.
For example, users of filesystem-level encryption (EXT4, F2FS, and/or
UBIFS encryption) usually wish to grant many differently-privileged
processes access to the same keys, set up in a shared keyring ahead of
time.  Permitting everyone to invalidate keys in this scenario enables a
trivial denial-of-service.  And even users of keyrings as "just caches"
may wish to restrict invalidation because it may require significant
work or user interaction to regenerate keys.

This patch fixes the flaw by requiring both Search and Setattr
permission to invalidate a key rather than just Search permission.
Requiring Setattr permission is appropriate because Setattr permission
also allows revoking (via keyctl_revoke()) or expiring (via
keyctl_set_timeout()) the key, which also make the key inaccessible
regardless of how many keyrings it is in.  Continuing to require Search
permission ensures we do not decrease the level of permissions required.

Alternatively, the problem could be solved by requiring Write
permission.  However, that would be less appropriate because Write
permission is ostensibly meant to deal with the key payload, not the key
itself.  A new "Invalidate" permission would also work, but that would
require keyctl_setperm() users to start including a new permission which
never existed before, which would be much more likely to break users of
keyctl_invalidate().  Finally, we could only allow invalidating keys
which the kernel has explicitly marked as invalidatible, e.g. the
"id_resolver" keys created for NFSv4 ID mapping.  However, that likewise
would be much more likely to break users.

This is a user-visible API change.  But we don't seem to have much
choice, and any breakage will be limited to users who override the
default key permissions using keyctl_setperm() to remove Setattr
permission, then later call keyctl_invalidate().  There are probably not
many such users, possibly even none at all.  In fact, the only users of
keyctl_invalidate() I could find are nfsidmap and the Chromium OS
cryptohome daemon.  nfsidmap will be unaffected because it runs as root
and actually relies on the "root is permitted to invalidate certain
special keys" exception (KEY_FLAG_ROOT_CAN_INVAL) rather than the actual
key permissions.  cryptohomed will be unaffected because it already owns
the keys and sets KEY_USR_SETATTR permission on them.

Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-fscrypt-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Gwendal Grignou <gwendal-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jaegeuk Kim <jaegeuk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Paul Crowley <paulcrowley-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
Cc: Ryo Hashimoto <hashimoto-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Tyler Hicks <tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.5+
Signed-off-by: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 Documentation/security/keys/core.rst | 4 ++--
 security/keys/keyctl.c               | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 1648fa80b3bf..c8030f628810 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -815,8 +815,8 @@ The keyctl syscall functions are:
      immediately, though they are still visible in /proc/keys until deleted
      (they're marked with an 'i' flag).
 
-     A process must have search permission on the key for this function to be
-     successful.
+     A process must have Search and Setattr permission on the key for this
+     function to be successful.
 
   *  Compute a Diffie-Hellman shared secret or public key::
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ab0b337c84b4..0739e7934e74 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -400,7 +400,7 @@ long keyctl_revoke_key(key_serial_t id)
 /*
  * Invalidate a key.
  *
- * The key must be grant the caller Invalidate permission for this to work.
+ * The key must grant the caller Search and Setattr permission for this to work.
  * The key and any links to the key will be automatically garbage collected
  * immediately.
  *
@@ -416,7 +416,7 @@ long keyctl_invalidate_key(key_serial_t id)
 
 	kenter("%d", id);
 
-	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
+	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH | KEY_NEED_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 
-- 
2.14.1.480.gb18f417b89-goog

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 6892 bytes --]

On 08/16/2017 04:14 PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> Currently, a process only needs Search permission on a key to invalidate
> it with keyctl_invalidate(), which has an effect similar to unlinking it
> from all keyrings.  Unfortunately, this significantly compromises the
> keys permission system because as a result, there is no way to grant
> read-only access to keys without also granting de-facto "delete" access.
> Even worse, processes may invalidate entire _keyrings_, given only
> permission to "search" them.  It is not even possible to block this
> using SELinux, because SELinux is likewise only asked for Search
> permission, which needs to be allowed for read-only access.
> 
> Key invalidation seems to have intended for cases where keyrings are
> used as caches, and keys can be re-requested at any time by an upcall to
> /sbin/request-key.  However, keyrings are not always used in this way.
> For example, users of filesystem-level encryption (EXT4, F2FS, and/or
> UBIFS encryption) usually wish to grant many differently-privileged
> processes access to the same keys, set up in a shared keyring ahead of
> time.  Permitting everyone to invalidate keys in this scenario enables a
> trivial denial-of-service.  And even users of keyrings as "just caches"
> may wish to restrict invalidation because it may require significant
> work or user interaction to regenerate keys.
> 
> This patch fixes the flaw by requiring both Search and Setattr
> permission to invalidate a key rather than just Search permission.
> Requiring Setattr permission is appropriate because Setattr permission
> also allows revoking (via keyctl_revoke()) or expiring (via
> keyctl_set_timeout()) the key, which also make the key inaccessible
> regardless of how many keyrings it is in.  Continuing to require Search
> permission ensures we do not decrease the level of permissions required.
> 
> Alternatively, the problem could be solved by requiring Write
> permission.  However, that would be less appropriate because Write
> permission is ostensibly meant to deal with the key payload, not the key
> itself.  A new "Invalidate" permission would also work, but that would
> require keyctl_setperm() users to start including a new permission which
> never existed before, which would be much more likely to break users of
> keyctl_invalidate().  Finally, we could only allow invalidating keys
> which the kernel has explicitly marked as invalidatible, e.g. the
> "id_resolver" keys created for NFSv4 ID mapping.  However, that likewise
> would be much more likely to break users.
> 
> This is a user-visible API change.  But we don't seem to have much
> choice, and any breakage will be limited to users who override the
> default key permissions using keyctl_setperm() to remove Setattr
> permission, then later call keyctl_invalidate().  There are probably not
> many such users, possibly even none at all.  In fact, the only users of
> keyctl_invalidate() I could find are nfsidmap and the Chromium OS
> cryptohome daemon.  nfsidmap will be unaffected because it runs as root
> and actually relies on the "root is permitted to invalidate certain
> special keys" exception (KEY_FLAG_ROOT_CAN_INVAL) rather than the actual
> key permissions.  cryptohomed will be unaffected because it already owns
> the keys and sets KEY_USR_SETATTR permission on them.

It makes sense to me to require something more than SEARCH permission to
invalidate a key and SETATTR does seem to be a good fit. Since you
mentioned WRITE as another possibility, I'll point out this blurb from
the original KEYCTL_INVALIDATE commit message (fd75815):

  To invalidate a key the caller must be granted SEARCH permission by
  the key. This may be too strict. It may be better to also permit
  invalidation if the caller has any of READ, WRITE or SETATTR
  permission.

So, (SEARCH | SETATTR) || (SEARCH | WRITE) could also be a possibility
to reduce the chance of breaking existing code. READ doesn't seem
appropriate to me.

I searched for KEYCTL_INVALIDATE in https://codesearch.debian.net/ and
didn't see anything outside of nfsidmap in Debian, which you already
investigated. I can confirm that ecryptfs-utils definitely isn't
affected by this change.

Tyler

> 
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-fscrypt-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Gwendal Grignou <gwendal-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Jaegeuk Kim <jaegeuk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Paul Crowley <paulcrowley-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
> Cc: Ryo Hashimoto <hashimoto-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Tyler Hicks <tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.5+
> Signed-off-by: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/security/keys/core.rst | 4 ++--
>  security/keys/keyctl.c               | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 1648fa80b3bf..c8030f628810 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -815,8 +815,8 @@ The keyctl syscall functions are:
>       immediately, though they are still visible in /proc/keys until deleted
>       (they're marked with an 'i' flag).
>  
> -     A process must have search permission on the key for this function to be
> -     successful.
> +     A process must have Search and Setattr permission on the key for this
> +     function to be successful.
>  
>    *  Compute a Diffie-Hellman shared secret or public key::
>  
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab0b337c84b4..0739e7934e74 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -400,7 +400,7 @@ long keyctl_revoke_key(key_serial_t id)
>  /*
>   * Invalidate a key.
>   *
> - * The key must be grant the caller Invalidate permission for this to work.
> + * The key must grant the caller Search and Setattr permission for this to work.
>   * The key and any links to the key will be automatically garbage collected
>   * immediately.
>   *
> @@ -416,7 +416,7 @@ long keyctl_invalidate_key(key_serial_t id)
>  
>  	kenter("%d", id);
>  
> -	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
> +	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH | KEY_NEED_SETATTR);
>  	if (IS_ERR(key_ref)) {
>  		ret = PTR_ERR(key_ref);
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Joe Richey]

On Wed, Aug 16, 2017 at 2:14 PM, Eric Biggers <ebiggers3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> Currently, a process only needs Search permission on a key to invalidate
> it with keyctl_invalidate(), which has an effect similar to unlinking it
> from all keyrings.  Unfortunately, this significantly compromises the
> keys permission system because as a result, there is no way to grant
> read-only access to keys without also granting de-facto "delete" access.
> Even worse, processes may invalidate entire _keyrings_, given only
> permission to "search" them.  It is not even possible to block this
> using SELinux, because SELinux is likewise only asked for Search
> permission, which needs to be allowed for read-only access.
>
> Key invalidation seems to have intended for cases where keyrings are
> used as caches, and keys can be re-requested at any time by an upcall to
> /sbin/request-key.  However, keyrings are not always used in this way.
> For example, users of filesystem-level encryption (EXT4, F2FS, and/or
> UBIFS encryption) usually wish to grant many differently-privileged
> processes access to the same keys, set up in a shared keyring ahead of
> time.  Permitting everyone to invalidate keys in this scenario enables a
> trivial denial-of-service.  And even users of keyrings as "just caches"
> may wish to restrict invalidation because it may require significant
> work or user interaction to regenerate keys.

This patch is great for our work on EXT4 encryption. Currently, if User
A wishes to decrypt a file and have it shared with User B, the
encryption keys must be in a keyring searchable by both users. However,
this also means that User B can just invalidate this shared keyring,
causing User A to lose access to their files.

Fixing this security issue will let us have a global keyring containing
all the EXT4 encryption keys (with type logon).

-- Joe Richey <joerichey-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

>
> This patch fixes the flaw by requiring both Search and Setattr
> permission to invalidate a key rather than just Search permission.
> Requiring Setattr permission is appropriate because Setattr permission
> also allows revoking (via keyctl_revoke()) or expiring (via
> keyctl_set_timeout()) the key, which also make the key inaccessible
> regardless of how many keyrings it is in.  Continuing to require Search
> permission ensures we do not decrease the level of permissions required.
>
> Alternatively, the problem could be solved by requiring Write
> permission.  However, that would be less appropriate because Write
> permission is ostensibly meant to deal with the key payload, not the key
> itself.  A new "Invalidate" permission would also work, but that would
> require keyctl_setperm() users to start including a new permission which
> never existed before, which would be much more likely to break users of
> keyctl_invalidate().  Finally, we could only allow invalidating keys
> which the kernel has explicitly marked as invalidatible, e.g. the
> "id_resolver" keys created for NFSv4 ID mapping.  However, that likewise
> would be much more likely to break users.
>
> This is a user-visible API change.  But we don't seem to have much
> choice, and any breakage will be limited to users who override the
> default key permissions using keyctl_setperm() to remove Setattr
> permission, then later call keyctl_invalidate().  There are probably not
> many such users, possibly even none at all.  In fact, the only users of
> keyctl_invalidate() I could find are nfsidmap and the Chromium OS
> cryptohome daemon.  nfsidmap will be unaffected because it runs as root
> and actually relies on the "root is permitted to invalidate certain
> special keys" exception (KEY_FLAG_ROOT_CAN_INVAL) rather than the actual
> key permissions.  cryptohomed will be unaffected because it already owns
> the keys and sets KEY_USR_SETATTR permission on them.
>
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-fscrypt-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Gwendal Grignou <gwendal-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Jaegeuk Kim <jaegeuk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Paul Crowley <paulcrowley-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
> Cc: Ryo Hashimoto <hashimoto-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Tyler Hicks <tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.5+
> Signed-off-by: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/security/keys/core.rst | 4 ++--
>  security/keys/keyctl.c               | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 1648fa80b3bf..c8030f628810 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -815,8 +815,8 @@ The keyctl syscall functions are:
>       immediately, though they are still visible in /proc/keys until deleted
>       (they're marked with an 'i' flag).
>
> -     A process must have search permission on the key for this function to be
> -     successful.
> +     A process must have Search and Setattr permission on the key for this
> +     function to be successful.
>
>    *  Compute a Diffie-Hellman shared secret or public key::
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab0b337c84b4..0739e7934e74 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -400,7 +400,7 @@ long keyctl_revoke_key(key_serial_t id)
>  /*
>   * Invalidate a key.
>   *
> - * The key must be grant the caller Invalidate permission for this to work.
> + * The key must grant the caller Search and Setattr permission for this to work.
>   * The key and any links to the key will be automatically garbage collected
>   * immediately.
>   *
> @@ -416,7 +416,7 @@ long keyctl_invalidate_key(key_serial_t id)
>
>         kenter("%d", id);
>
> -       key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
> +       key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH | KEY_NEED_SETATTR);
>         if (IS_ERR(key_ref)) {
>                 ret = PTR_ERR(key_ref);
>
> --
> 2.14.1.480.gb18f417b89-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[David Howells]

Eric Biggers <ebiggers3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This patch fixes the flaw by requiring both Search and Setattr permission to
> invalidate a key rather than just Search permission.

I'm not sure I agree.  The problem is that you then have to grant someone
Setattr permission for them to be able to do this, which then opens up a whole
host of other things they can also do.

> Requiring Setattr permission is appropriate because Setattr permission also
> allows revoking (via keyctl_revoke()) or expiring (via keyctl_set_timeout())
> the key, which also make the key inaccessible regardless of how many
> keyrings it is in.

Note that setting the expiry time is not really equivalent to revokation in
this regard as setting the expiry time is something you do when setting up a
key, whereas revokation is something you do later to kill a key off.

> Alternatively, the problem could be solved by requiring Write permission.

That's not really a viable solution as things stand.  You can't turn Write
permission on on a key whose type doesn't have an ->update() method (though
this could be changed) and it also opens up a key to being modified.

How about another solution:

 (1) I add a flag to a key to say that it can be invalidated and a keyctl to
     change that flag.

 (2) I add a new key type op called ->allow_invalidation() that allows key
     types to govern separately who is allowed to invalidate keys of that
     type.

     So, for instance, DNS record invalidation would require CAP_NET_ADMIN.

 (3) Allow keyrings to be cleared by users who don't have Write permission but
     do have other permission, such as CAP_NET_ADMIN.  This would need to be
     granted on a per-keyring basis.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Eric Biggers]

Hi David,

On Tue, Sep 05, 2017 at 10:54:55AM +0100, David Howells wrote:
> Eric Biggers <ebiggers3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > This patch fixes the flaw by requiring both Search and Setattr permission to
> > invalidate a key rather than just Search permission.
> 
> I'm not sure I agree.  The problem is that you then have to grant someone
> Setattr permission for them to be able to do this, which then opens up a whole
> host of other things they can also do.
> 

True, but Setattr permission has already been overloaded to allow several
different types of modifications, and it makes *much* more sense than Search
permission which should not allow any modifications.  And in practice I expect
people care more about whether modifications are permitted or not, than the
details of the finer-grained permissions.

> > Requiring Setattr permission is appropriate because Setattr permission also
> > allows revoking (via keyctl_revoke()) or expiring (via keyctl_set_timeout())
> > the key, which also make the key inaccessible regardless of how many
> > keyrings it is in.
> 
> Note that setting the expiry time is not really equivalent to revokation in
> this regard as setting the expiry time is something you do when setting up a
> key, whereas revokation is something you do later to kill a key off.
> 

Sort of, but actually keyctl_set_timeout() can be called at any time, and the
timeout can be set to as little as 1 second.  So I don't see how keyctl_revoke()
is that much different, fundamentally.

> How about another solution:
> 
>  (1) I add a flag to a key to say that it can be invalidated and a keyctl to
>      change that flag.

And who would have permission to change that flag?  It seems to be the same
problem again.

> 
>  (2) I add a new key type op called ->allow_invalidation() that allows key
>      types to govern separately who is allowed to invalidate keys of that
>      type.
> 
>      So, for instance, DNS record invalidation would require CAP_NET_ADMIN.

What would the behavior be if ->allow_invalidation() was not supplied?  In other
words, would the purpose of this be to lock down invalidation of dns_resolver
keys, or to restrict invalidation to *only* dns_resolver keys?

> 
>  (3) Allow keyrings to be cleared by users who don't have Write permission but
>      do have other permission, such as CAP_NET_ADMIN.  This would need to be
>      granted on a per-keyring basis.

Granted by who, and how?  And do you mean keyctl_clear(), or
keyctl_invalidate()?

- Eric

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> True, but Setattr permission has already been overloaded to allow several
> different types of modifications,

Perhaps therein lies the problem.  Setattr is too overloaded and really needs
splitting along the following lines:

 (1) Security: ownership, access, security label, restrictions.

 (2) Content: alter/update, timeout.

 (3) Revocation and invalidation.

Maybe I should create some sort of ACL for keys and map setperm onto that.

I wonder if this could also intersect with user namespaces in some way so that
you can make an ACL that has users from multiple namespaces - might be tricky,
though.

> and it makes *much* more sense than Search permission which should not allow
> any modifications.  And in practice I expect people care more about whether
> modifications are permitted or not, than the details of the finer-grained
> permissions.

I disagree still.  To allow users to invalidate a key you would *also* have to
give them the ability to muck around with the permissions.

> Sort of, but actually keyctl_set_timeout() can be called at any time, and
> the timeout can be set to as little as 1 second.  So I don't see how
> keyctl_revoke() is that much different, fundamentally.

The timeout is a property of the key content and revoked is one of the states
the key can be in.

> >  (1) I add a flag to a key to say that it can be invalidated and a keyctl to
> >      change that flag.
> 
> And who would have permission to change that flag?  It seems to be the same
> problem again.

No.  Setperm would be required to change the flag, but not to apply the
invalidate operation if the flag is set.  Think of Invalidate as being a
mass-unlink operation effected by the garbage collector.

Kernel-created DNS keys, for example, don't grant Setperm to anyone.

> What would the behavior be if ->allow_invalidation() was not supplied?

The obvious would be to check that you're the owner of the key.

> In other words, would the purpose of this be to lock down invalidation of
> dns_resolver keys, or to restrict invalidation to *only* dns_resolver keys?

To restrict who could invalidate keys of a particular type.  Actually, it
wants to be per-key not per-key-type.  Hmmm...

> Granted by who, and how?  And do you mean keyctl_clear(), or
> keyctl_invalidate()?

keyctl_clear().  Empty the keyring, not render it unusable.


David

Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Eric Biggers]

Hi David,

On Fri, Sep 08, 2017 at 04:41:40PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > True, but Setattr permission has already been overloaded to allow several
> > different types of modifications,
> 
> Perhaps therein lies the problem.  Setattr is too overloaded and really needs
> splitting along the following lines:
> 
>  (1) Security: ownership, access, security label, restrictions.
> 
>  (2) Content: alter/update, timeout.
> 
>  (3) Revocation and invalidation.
> 
> Maybe I should create some sort of ACL for keys and map setperm onto that.
> 
> I wonder if this could also intersect with user namespaces in some way so that
> you can make an ACL that has users from multiple namespaces - might be tricky,
> though.

There would be a *lot* of changes needed to implement ACLs for keys.  Setting
them, getting them, evaluating them, updating all the LSMs to understand any new
permissions, deciding how the existing keyctl operations affect the ACL,
deciding what ACL to use by default, etc...  And it would make the keyrings
permissions system even more complicated, when people already have trouble
understanding it.  Complicated permissions systems are difficult to use
correctly.

Also I suppose that Invalidate would be implied by the old "Search" permission,
so users would need to use a new keyctl to remove Invalidate permission from the
ACL to get sane behavior?  That would be a pain, in comparison to having the bug
fixed by default.

> 
> > and it makes *much* more sense than Search permission which should not allow
> > any modifications.  And in practice I expect people care more about whether
> > modifications are permitted or not, than the details of the finer-grained
> > permissions.
> 
> I disagree still.  To allow users to invalidate a key you would *also* have to
> give them the ability to muck around with the permissions.
> 

True, though actually keyctl_setperm() also requires that the user own the key
or be root.

And while finer-grained permissions are preferable in theory, out of curiosity
do you have an example of an actual user who needs to be able to invalidate, or
perhaps in plainer language, "delete" keys without otherwise being able to
modify the keys or their containing keyring(s)?

Meanwhile, users of filesystem-level encryption need to set up keys that can be
searched by non-root users but not deleted.  This *should* be a normal, very
boring use case, since being able to grant read-only access should be a
fundamental part of any access control system.  But not with the keyrings
permission system, apparently :-)

> > Sort of, but actually keyctl_set_timeout() can be called at any time, and
> > the timeout can be set to as little as 1 second.  So I don't see how
> > keyctl_revoke() is that much different, fundamentally.
> 
> The timeout is a property of the key content and revoked is one of the states
> the key can be in.
> 
> > >  (1) I add a flag to a key to say that it can be invalidated and a keyctl to
> > >      change that flag.
> > 
> > And who would have permission to change that flag?  It seems to be the same
> > problem again.
> 
> No.  Setperm would be required to change the flag, but not to apply the
> invalidate operation if the flag is set.  Think of Invalidate as being a
> mass-unlink operation effected by the garbage collector.
> 
> Kernel-created DNS keys, for example, don't grant Setperm to anyone.
> 

Perhaps keyctl_invalidate() should by default require Setattr permission, or
perhaps owning the key --- but after a call to a new keyctl_set_invalidatable()
which would require Setattr permission, just Search permission would be
required?  It seems that would address your concern about needing to grant
Setattr permission to invalidate keys.

> 
> > What would the behavior be if ->allow_invalidation() was not supplied?
> 
> The obvious would be to check that you're the owner of the key.
> 
> > In other words, would the purpose of this be to lock down invalidation of
> > dns_resolver keys, or to restrict invalidation to *only* dns_resolver keys?
> 
> To restrict who could invalidate keys of a particular type.  Actually, it
> wants to be per-key not per-key-type.  Hmmm...
> 
> > Granted by who, and how?  And do you mean keyctl_clear(), or
> > keyctl_invalidate()?
> 
> keyctl_clear().  Empty the keyring, not render it unusable.
> 
> 
> David

If there is a "keyctl_set_invalidatable()" being added too, I don't really see
the point of adding an ->allow_invalidation() method and mucking around with
keyctl_clear() permissions as well.

Eric