From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Richey Subject: Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission Date: Wed, 16 Aug 2017 15:08:05 -0700 Message-ID: References: <20170816211403.121920-1-ebiggers3@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20170816211403.121920-1-ebiggers3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eric Biggers Cc: keyrings-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, David Howells , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fscrypt-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Gwendal Grignou , Jaegeuk Kim , Paul Crowley , Richard Weinberger , Ryo Hashimoto , Tyler Hicks , stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-api@vger.kernel.org On Wed, Aug 16, 2017 at 2:14 PM, Eric Biggers wrote: > From: Eric Biggers > > 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 > > 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 > Cc: Jaegeuk Kim > Cc: Paul Crowley > Cc: Richard Weinberger > Cc: Ryo Hashimoto > Cc: Tyler Hicks > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.5+ > Signed-off-by: Eric Biggers > --- > 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