| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <004ec27a-cc3a-c75c-952b-ff371b82b8d1@schaufler-ca.com>
Date: Wed, 13 May 2020 08:25:55 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: David Howells <dhowells@...hat.com>, stephen.smalley.work@...il.com
Cc: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
Paul Moore <paul@...l-moore.com>, keyrings@...r.kernel.org,
selinux@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org,
Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a
mask
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@...hat.com>
> cc: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> cc: Paul Moore <paul@...l-moore.com>
> cc: Stephen Smalley <stephen.smalley.work@...il.com>
> cc: Casey Schaufler <casey@...aufler-ca.com>
> cc: keyrings@...r.kernel.org
> cc: selinux@...r.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;
>
>
Powered by blists - more mailing lists