On Wed, Aug 16, 2017 at 2:14 PM, Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Cc: linux-cifs@xxxxxxxxxxxxxxx
> Cc: linux-fscrypt@xxxxxxxxxxxxxxx
> Cc: linux-nfs@xxxxxxxxxxxxxxx
> Cc: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> Cc: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> Cc: Paul Crowley <paulcrowley@xxxxxxxxxx>
> Cc: Richard Weinberger <richard@xxxxxx>
> Cc: Ryo Hashimoto <hashimoto@xxxxxxxxxx>
> Cc: Tyler Hicks <tyhicks@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v3.5+
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html