| Message ID | 158932282880.2885325.2688622278854566047.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | keys: Make the KEY_NEED_* perms an enum rather than a mask | expand |
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;
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.
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; > >
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
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
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
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
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?
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.
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;
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(-)