KEYS: Replace uid/gid/perm permissions checking with ACL
Subsystems (10)
David Howells
maintainerRFC PATCH 1/1 KEYS: Replace uid/gid/perm permissions checking with ACL
Hi Eric,
Here's a patch that partially implements ACLs on keys. I haven't yet done KEYCTL_[GS]ET_ACL, but KEYCTL_SETATTR, KEYCTL_DESCRIBE and /proc/keys have been modified to deal with it.
Most of the keyutils testsuite passes, just the permissions tests that don't because there isn't a fully compatible mapping from SETATTR -> ACL -> DESCRIBE.
David
---
KEYS: Replace uid/gid/perm permissions checking with ACL
Replace the uid/gid/perm permissions checking on a key with an ACL to allow the SETATTR permission to be split. The problem is that SETATTR covers a slew of things, not all of which should be grouped together. This includes:
(1) Changing the key ownership.
(2) Changing the security information.
(3) Keyring restriction.
(4) Expiry time.
(5) Revocation.
and it has also been proposed to add:
(6) Invalidation.
The above can be divided into three groups: Controlling access (1), (2) and (3), managing the content at construction time (4) and managing the key (5) and (6).
Add an ACL set & get functions that use a list of elements of the following form:
struct key_ace {
union {
uid_t uid;
gid_t gid;
unsigned int special_id;
};
unsigned int mask;
};where the top four bits of mask indicate the subject being checked:
KEY_ACE_SPECIAL A specific value
KEY_ACE_UID A nominated UID
KEY_ACE_GID A nominated GID
and if SPECIAL is specified, special_id is one of the following:
KEY_ACE_EVERYONE Everyone, including owner and group
KEY_ACE_GROUP The key's group
KEY_ACE_OWNER The owner of the key
KEY_ACE_POSSESSOR Any process that possesses the key
KEY_ACE_ROOT The user namespace root user
KEY_ACE_SYS_ADMIN Anyone with CAP_SYS_ADMIN
KEY_ACE_NET_ADMIN Anyone with CAP_NET_ADMIN
Each special ID may only occur once in an ACL.
The bottom 28 bits of mask indicate the permissions being granted:
KEY_ACE_VIEW Can view the key metadata
KEY_ACE_READ Can read the key content
KEY_ACE_WRITE Can update/modify the key content
KEY_ACE_SEARCH Can find the key by searching/requesting
KEY_ACE_LINK Can make a link to the key
KEY_ACE_INVAL Can invalidate
KEY_ACE_REVOKE Can revoke
KEY_ACE_SET_SECURITY Can set security
KEY_ACE_JOIN Can join this keyring
KEY_ACE_CLEAR Can clear this keyring
The ACEs are purely additive; all ACEs that apply are considered. I figure that it's unlikely that you'll give the key's Group permissions that you wouldn't give the Owner and Other permissions that you wouldn't give to both Group and Owner. Possessor permissions are already additive.
The functions are:
keyctl(KEYCTL_GET_ACL, key_serial_t key,
struct key_ace *acl_buffer, unsigned int buflen);
keyctl(KEYCTL_SET_ACL, key_serial_t key,
const struct key_ace *acl, unsigned int len);
The KEYCTL_SETATTR function is then deprecated. If called, it will construct an ACL to reflect the mask it is given, using possessor, owner, group and other ACE's as appropriate if any of those elements are granted any permissions. SETATTR permission turns on all of INVAL, REVOKE and SET_SECURITY. WRITE permission turns on WRITE, REVOKE and, if a keyring, CLEAR. JOIN is turned on if a keyring is being altered. KEYCTL_SETATTR will return an error if SETACL has been called on a key.
The KEYCTL_DESCRIBE function then creates a permissions mask to return depending on possessor, owner, group and other ACEs, indicating SETATTR if any of INVAL, REVOKE and SET_SECURITY are set and indicating WRITE if any of WRITE, REVOKE or CLEAR are set.
Note that the value subsequently returned by KEYCTL_DESCRIBE may not match the value set with KEYCTL_SETATTR - but this is already true because keys that lack ->read() can't have READ set and keys that lack ->write() can't have WRITE set.
The KEYCTL_SET_TIMEOUT function then is permitted if WRITE or SETSEC is set, or if the caller has a valid instantiation auth token.
The KEYCTL_INVALIDATE function then requires INVAL.
The KEYCTL_REVOKE function then requires REVOKE.
The KEYCTL_JOIN_SESSION_KEYRING function then requires JOIN to join an existing keyring.
The JOIN permission is enabled by default for session keyrings and manually created keyrings only.
---
certs/blacklist.c +28 -5
@@ -23,6 +23,32 @@static struct key *blacklist_keyring; +static struct key_acl blacklist_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_READ ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; + +static struct key_acl blacklist_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; +/* * The description must be a type prefix, a colon and then an even number of * hex digits. The hash is kept in the description.
@@ -93,8 +119,7 @@ int mark_hash_blacklisted(const char *hash) hash,NULL ,0 , - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW ), + &blacklist_key_acl,KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);if (IS_ERR (key)) {
@@ -153,9 +178,7 @@ static int __init blacklist_init(void)keyring_alloc (".blacklist" ,KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - (KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ | -KEY_USR_SEARCH , + &blacklist_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA |KEY_FLAG_KEEP ,NULL ,NULL );
certs/system_keyring.c +30 -9
@@ -27,6 +27,33 @@ static struct key *secondary_trusted_keys;extern __initconst const u8 system_certificate_list[];extern __initconst const unsigned long system_certificate_list_size; +static struct key_acl trusted_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_READ ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; + +static struct key_acl trusted_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask = (KEY_ACE_SPECIAL | +KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_OWNER , + }, +}; +/** * restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA *
@@ -99,9 +126,7 @@ static __init int system_trusted_keyring_init(void) builtin_trusted_keys keyring_alloc(".builtin_trusted_keys",KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ |KEY_USR_SEARCH ), -KEY_ALLOC_NOT_IN_QUOTA , + &trusted_key_acl,KEY_ALLOC_NOT_IN_QUOTA , NULL, NULL);if (IS_ERR (builtin_trusted_keys))panic ("Can't allocate builtin trusted keyring\n" );
@@ -110,10 +135,7 @@ static __init int system_trusted_keyring_init(void) secondary_trusted_keys keyring_alloc(".secondary_trusted_keys",KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ |KEY_USR_SEARCH | -KEY_USR_WRITE ), -KEY_ALLOC_NOT_IN_QUOTA , + &trusted_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,get_builtin_and_secondary_restriction (), NULL);if (IS_ERR (secondary_trusted_keys))
@@ -163,8 +185,7 @@ static __init int load_system_certificate_list(void)NULL , p, plen, - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ ), + &trusted_key_acl,KEY_ALLOC_NOT_IN_QUOTA |KEY_ALLOC_BUILT_IN | KEY_ALLOC_BYPASS_RESTRICTION);
fs/afs/security.c +1 -1
@@ -28,7 +28,7 @@ struct key *afs_request_key(struct afs_cell *cell)_debug ("key %s" , cell->anonymous_key->description); key =request_key (&key_type_rxrpc, cell->anonymous_key->description, -NULL ); +NULL ,NULL );if (IS_ERR (key)) {if (PTR_ERR (key) != -ENOKEY ) {_leave (" = %ld" ,PTR_ERR (key));
fs/cifs/cifs_spnego.c +37 -4
65 lines not on this page · raw message
fs/cifs/cifsacl.c +39 -5
74 lines not on this page · raw message
fs/cifs/connect.c +2 -2
@@ -2469,7 +2469,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses) }cifs_dbg (FYI ,"%s: desc=%s\n" , __func__, desc); - key =request_key (&key_type_logon, desc,"" ); + key =request_key (&key_type_logon, desc,"" ,NULL );if (IS_ERR (key)) {if (!ses->domainName) {cifs_dbg (FYI ,"domainName is NULL\n" );
@@ -2480,7 +2480,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses)/* didn't work, try to find a domain key */ sprintf (desc,"cifs:d:%s" , ses->domainName);cifs_dbg (FYI ,"%s: desc=%s\n" , __func__, desc); - key =request_key (&key_type_logon, desc,"" ); + key =request_key (&key_type_logon, desc,"" ,NULL );if (IS_ERR (key)) { rc =PTR_ERR (key); goto out_err;
fs/fscache/object-list.c +1 -1
@@ -323,7 +323,7 @@ static void fscache_objlist_config(struct fscache_objlist_data *data)const char *buf;int len; - key =request_key (&key_type_user,"fscache:objlist" ,NULL ); + key =request_key (&key_type_user,"fscache:objlist" ,NULL ,NULL );if (IS_ERR (key)) goto no_config;
fs/nfs/nfs4idmap.c +38 -9
82 lines not on this page · raw message
include/linux/key.h +41 -22
151 lines not on this page · raw message
include/linux/keyctl.h new file +3 -0
@@ -0,0 +1,3 @@ +#define key_ace user_key_ace +#include <uapi/linux/keyctl.h> +#undef key_ace
include/uapi/linux/keyctl.h +36 -0
@@ -14,6 +14,40 @@#include <linux/types.h> +/* Keyring ACL definitions */ +struct key_ace { +unsigned int mask; +#define KEY_ACE_VIEW 0x00000001/* Can describe the key */ +#define KEY_ACE_READ 0x00000002/* Can read the key content */ +#define KEY_ACE_WRITE 0x00000004/* Can update/modify the key content */ +#define KEY_ACE_SEARCH 0x00000008/* Can find the key by search */ +#define KEY_ACE_LINK 0x00000010/* Can make a link to the key */ +#define KEY_ACE_INVAL 0x00000020/* Can invalidate the key */ +#define KEY_ACE_REVOKE 0x00000040/* Can revoke the key */ +#define KEY_ACE_SET_SECURITY 0x00000080/* Can set owner, group, ACL */ +#define KEY_ACE_JOIN 0x00000100/* Can join keyring */ +#define KEY_ACE_CLEAR 0x00000200/* Can clear keyring */ +#define KEY_ACE__ORDINARY 0x0000001f/* Ordinary permissions */ +#define KEY_ACE__PERMS 0x0fffffff +#define KEY_ACE_SPECIAL 0x10000000/* A specific subject */ +#define KEY_ACE_UID 0x20000000/* A nominated UID */ +#define KEY_ACE_GID 0x30000000/* A nominated GID */ +#define KEY_ACE__IDENTITY 0xf0000000 + +union { +uid_t uid; +gid_t gid; +unsigned int special_id; +#define KEY_ACE_EVERYONE 1/* Everyone, including owner and group */ +#define KEY_ACE_GROUP 2/* The key's group */ +#define KEY_ACE_OWNER 3/* The owner of the key */ +#define KEY_ACE_POSSESSOR 4/* Any process that possesses of the key */ +#define KEY_ACE_ROOT 5/* Anyone with user namespace root */ +#define KEY_ACE_SYS_ADMIN 6/* Anyone with CAP_SYS_ADMIN */ +#define KEY_ACE_NET_ADMIN 7/* Anyone with CAP_NET_ADMIN */ + }; +}; +/* special process keyring shortcut IDs */ #define KEY_SPEC_THREAD_KEYRING -1/* - key ID for thread-specific keyring */ #define KEY_SPEC_PROCESS_KEYRING -2/* - key ID for process-specific keyring */
@@ -61,6 +95,8 @@#define KEYCTL_GET_PERSISTENT 22/* get a user's persistent keyring */ #define KEYCTL_DH_COMPUTE 23/* Compute Diffie-Hellman values */ #define KEYCTL_RESTRICT_KEYRING 29/* Restrict keys allowed to link to a keyring */ +#define KEYCTL_GET_ACL 30/* Get a key's ACL */ +#define KEYCTL_SET_ACL 31/* Set a key's ACL */ /* keyctl structures */ struct keyctl_dh_params {
net/dns_resolver/dns_key.c +18 -3
@@ -45,6 +45,23 @@ const struct cred *dns_resolver_cache;#define DNS_ERRORNO_OPTION "dnserror" +static struct key_acl dns_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =3 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_SEARCH |KEY_ACE_WRITE , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, + .aces[2 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_CLEAR , + .special_id =KEY_ACE_NET_ADMIN , + }, +}; +/* * Preparse instantiation data for a dns_resolver key. *
@@ -279,8 +296,7 @@ static int __init init_dns_resolver(void) keyring =keyring_alloc (".dns_resolver" ,GLOBAL_ROOT_UID ,GLOBAL_ROOT_GID , cred, - (KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ , + &dns_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL ,NULL );if (IS_ERR (keyring)) { ret =PTR_ERR (keyring);
@@ -293,7 +309,6 @@ static int __init init_dns_resolver(void)/* instruct request_key() to use this special keyring as a cache for * the results it looks up */ -set_bit (KEY_FLAG_ROOT_CAN_CLEAR , &keyring->flags); cred->thread_keyring = keyring; cred->jit_keyring =KEY_REQKEY_DEFL_THREAD_KEYRING ; dns_resolver_cache = cred;
net/dns_resolver/dns_query.c +18 -4
@@ -46,6 +46,23 @@#include "internal.h" +static struct key_acl dns_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =3 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE__PERMS , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, + .aces[2 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_INVAL , + .special_id =KEY_ACE_NET_ADMIN , + }, +}; +/** * dns_query - Query the DNS * @type: Query type (or NULL for straight host->IP lookup)
@@ -122,7 +139,7 @@ int dns_query(const char *type, const char *name, size_t namelen,* add_key() to preinstall malicious redirections */ saved_cred =override_creds (dns_resolver_cache); - rkey =request_key (&key_type_dns_resolver, desc, options); + rkey =request_key (&key_type_dns_resolver, desc, options, &dns_key_acl);revert_creds (saved_cred);kfree (desc);if (IS_ERR (rkey)) {
@@ -131,9 +148,6 @@ int dns_query(const char *type, const char *name, size_t namelen, }down_read (&rkey->sem); -set_bit (KEY_FLAG_ROOT_CAN_INVAL , &rkey->flags); - rkey->perm |=KEY_USR_VIEW ; - ret =key_validate (rkey);if (ret <0 ) goto put;
net/rxrpc/key.c +12 -3
@@ -27,6 +27,15 @@#include <keys/user-type.h> #include "ar-internal.h" +static struct key_acl rxrpc_null_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =1 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_SEARCH , + .special_id =KEY_ACE_POSSESSOR , + }, +}; +static int rxrpc_vet_description_s (const char *);static int rxrpc_preparse (struct key_preparsed_payload *);static int rxrpc_preparse_s (struct key_preparsed_payload *);
@@ -912,7 +921,7 @@ int rxrpc_request_key(struct rxrpc_sock *rx, char __user *optval, int optlen)if (IS_ERR (description))return PTR_ERR (description); - key =request_key (&key_type_rxrpc, description,NULL ); + key =request_key (&key_type_rxrpc, description,NULL ,NULL );if (IS_ERR (key)) {kfree (description);_leave (" = %ld" ,PTR_ERR (key));
@@ -943,7 +952,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval,if (IS_ERR (description))return PTR_ERR (description); - key =request_key (&key_type_keyring, description,NULL ); + key =request_key (&key_type_keyring, description,NULL ,NULL );if (IS_ERR (key)) {kfree (description);_leave (" = %ld" ,PTR_ERR (key));
@@ -1024,7 +1033,7 @@ struct key *rxrpc_get_null_key(const char *keyname) key =key_alloc (&key_type_rxrpc, keyname,GLOBAL_ROOT_UID ,GLOBAL_ROOT_GID , cred, -KEY_POS_SEARCH ,KEY_ALLOC_NOT_IN_QUOTA ,NULL ); + &rxrpc_null_key_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL );if (IS_ERR (key))return key;
security/keys/encrypted-keys/encrypted.c +1 -1
@@ -303,7 +303,7 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_kconst struct user_key_payload *upayload;struct key *ukey; - ukey =request_key (&key_type_user, master_desc,NULL ); + ukey =request_key (&key_type_user, master_desc,NULL ,NULL );if (IS_ERR (ukey)) goto error;
security/keys/encrypted-keys/masterkey_trusted.c +1 -1
@@ -34,7 +34,7 @@ struct key *request_trusted_key(const char *trusted_desc,struct trusted_key_payload *tpayload;struct key *tkey; - tkey =request_key (&key_type_trusted, trusted_desc,NULL ); + tkey =request_key (&key_type_trusted, trusted_desc,NULL ,NULL );if (IS_ERR (tkey)) goto error;
security/keys/gc.c +1 -1
@@ -155,6 +155,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)atomic_dec (&key->user->nikeys);key_user_put (key->user); +key_put_acl (key->acl);kfree (key->description);
@@ -225,7 +226,6 @@ static void key_garbage_collector(struct work_struct *work)if (key->type = key_gc_dead_keytype) { gc_state |=KEY_GC_FOUND_DEAD_KEY ;set_bit (KEY_FLAG_DEAD , &key->flags); - key->perm =0 ; goto skip_dead_key; }else if (key->type = &key_type_keyring && key->restrict_link) {
security/keys/internal.h +8 -4
@@ -88,7 +88,8 @@ extern struct rb_root key_serial_tree;extern spinlock_t key_serial_lock;extern struct mutex key_construction_mutex;extern wait_queue_head_t request_key_conswq; - +extern struct key_acl default_key_acl; +extern struct key_acl joinable_keyring_acl;extern struct key_type *key_type_lookup (const char *type);extern void key_type_put (struct key_type *ktype);
@@ -153,13 +154,14 @@ extern struct key *request_key_and_link(struct key_type *type,const void *callout_info, size_t callout_len, void *aux, + struct key_acl *acl, struct key *dest_keyring, unsigned long flags);extern bool lookup_user_key_possessed (const struct key *key,const struct key_match_data *match_data);extern key_ref_t lookup_user_key (key_serial_t id,unsigned long flags, -key_perm_t perm); +u32 desired_perm);#define KEY_LOOKUP_CREATE 0x01#define KEY_LOOKUP_PARTIAL 0x02#define KEY_LOOKUP_FOR_UNLINK 0x04
@@ -178,7 +180,9 @@ 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); +u32 desired_perm); +extern unsigned int key_acl_to_perm (const struct key_acl *acl); +extern void key_put_acl (struct key_acl *acl);/* * Check to see whether permission is granted to use a key in the desired way.
@@ -234,7 +238,7 @@ extern long keyctl_keyring_search(key_serial_t, const char __user *, const char __user *, key_serial_t);extern long keyctl_read_key (key_serial_t ,char __user *,size_t );extern long keyctl_chown_key (key_serial_t ,uid_t ,gid_t ); -extern long keyctl_setperm_key (key_serial_t ,key_perm_t ); +extern long keyctl_setperm_key (key_serial_t ,unsigned int );extern long keyctl_instantiate_key (key_serial_t ,const void __user *,size_t ,key_serial_t );extern long keyctl_negate_key (key_serial_t ,unsigned ,key_serial_t );
security/keys/key.c +8 -16
@@ -227,7 +227,7 @@ static inline void key_alloc_serial(struct key *key)*/ struct key *key_alloc (struct key_type *type,const char *desc,kuid_t uid,kgid_t gid,const struct cred *cred, -key_perm_t perm,unsigned long flags, +struct key_acl *acl,unsigned long flags,struct key_restriction *restrict_link) {struct key_user *user =NULL ;
@@ -295,7 +295,8 @@ struct key *key_alloc(struct key_type *type, const char *desc, key->datalen = type->def_datalen; key->uid = uid; key->gid = gid; - key->perm = perm; +refcount_inc (&acl->usage); +rcu_assign_pointer (key->acl, acl); key->restrict_link = restrict_link;if (!(flags &KEY_ALLOC_NOT_IN_QUOTA ))
@@ -798,7 +799,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, const char *description, const void *payload, size_t plen, - key_perm_t perm, + struct key_acl *acl, unsigned long flags) { struct keyring_index_key index_key = {
@@ -889,22 +890,13 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, goto found_matching_key; } -/* if the client doesn't provide, decide on the permissions we want */ -if (perm =KEY_PERM_UNDEF ) { - perm =KEY_POS_VIEW |KEY_POS_SEARCH |KEY_POS_LINK |KEY_POS_SETATTR ; - perm |=KEY_USR_VIEW ; - -if (index_key.type->read) - perm |=KEY_POS_READ ; - -if (index_key.type = &key_type_keyring || - index_key.type->update) - perm |=KEY_POS_WRITE ; - } +/* If the caller doesn't provide, provide default permissions. */ +if (!acl) + acl = &default_key_acl;/* allocate a new key */ key =key_alloc (index_key.type, index_key.description, - cred->fsuid, cred->fsgid, cred, perm, flags,NULL ); + cred->fsuid, cred->fsgid, cred, acl, flags,NULL );if (IS_ERR (key)) { key_ref =ERR_CAST (key); goto error_link_end;
security/keys/keyctl.c +67 -51
243 lines not on this page · raw message
security/keys/keyring.c +2 -2
@@ -493,7 +493,7 @@ static long keyring_read(const struct key *keyring,* Allocate a keyring and link into the destination keyring. */ struct key *keyring_alloc (const char *description,kuid_t uid,kgid_t gid, -const struct cred *cred,key_perm_t perm, +const struct cred *cred,struct key_acl *acl,unsigned long flags,struct key_restriction *restrict_link,struct key *dest)
@@ -502,7 +502,7 @@ struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,int ret; keyring =key_alloc (&key_type_keyring, description, - uid, gid, cred, perm, flags, restrict_link); + uid, gid, cred, acl, flags, restrict_link);if (!IS_ERR (keyring)) { ret =key_instantiate_and_link (keyring,NULL ,0 , dest,NULL );if (ret <0 ) {
security/keys/permission.c +163 -38
232 lines not on this page · raw message
security/keys/proc.c +19 -6
@@ -176,13 +176,15 @@ static void proc_keys_stop(struct seq_file *p, void *v)static int proc_keys_show (struct seq_file *m,void *v) { +const struct key_acl *acl;struct rb_node *_p = v; struct key *key = rb_entry(_p, struct key, serial_node);struct timespec now;unsigned long timo;key_ref_t key_ref, skey_ref;char xbuf[16 ]; -int rc; +bool check_pos; +int i, rc;struct keyring_search_context ctx = { .index_key.type = key->type,
@@ -194,12 +196,25 @@ static int proc_keys_show(struct seq_file *m, void *v) .flags = KEYRING_SEARCH_NO_STATE_CHECK, }; - key_ref =make_key_ref (key,0 ); +rcu_read_lock (); + + acl =rcu_dereference (key->acl); + check_pos = false; +for (i =0 ; i < acl->nr_ace; i++) { +const struct key_ace *ace = &acl->aces[i]; +if (ace->special_id =KEY_ACE_POSSESSOR ) { +if ((ace->mask & (KEY_ACE__IDENTITY |KEY_ACE_VIEW )) = + (KEY_ACE_SPECIAL |KEY_ACE_VIEW )) + check_pos = true; +break ; + } + }/* determine if the key is possessed by this process (a test we can * skip if the key does not indicate the possessor can view it */ -if (key->perm &KEY_POS_VIEW ) { + key_ref =make_key_ref (key,0 ); +if (check_pos) { skey_ref =search_my_process_keyrings (&ctx);if (!IS_ERR (skey_ref)) {key_ref_put (skey_ref);
@@ -214,8 +229,6 @@ static int proc_keys_show(struct seq_file *m, void *v) now =current_kernel_time (); -rcu_read_lock (); -/* come up with a suitable timeout value */ if (key->expiry =0 ) {memcpy (xbuf,"perm" ,5 );
@@ -250,7 +263,7 @@ static int proc_keys_show(struct seq_file *m, void *v)showflag (key,'i' ,KEY_FLAG_INVALIDATED ),refcount_read (&key->usage), xbuf, - key->perm, +key_acl_to_perm (acl),from_kuid_munged (seq_user_ns (m), key->uid),from_kgid_munged (seq_user_ns (m), key->gid), key->type->name);
security/keys/process_keys.c +60 -20
161 lines not on this page · raw message
security/keys/request_key.c +25 -22
163 lines not on this page · raw message
security/selinux/hooks.c +15 -1
@@ -6157,6 +6157,7 @@ static int selinux_key_permission(key_ref_t key_ref, {struct key *key;struct key_security_struct *ksec; +unsigned oldstyle_perm;u32 sid;/* if no specific permissions are requested, we skip the
@@ -6165,12 +6166,25 @@ static int selinux_key_permission(key_ref_t key_ref,if (perm =0 )return 0 ; + oldstyle_perm = perm & (KEY_NEED_VIEW |KEY_NEED_READ |KEY_NEED_WRITE | +KEY_NEED_SEARCH |KEY_NEED_LINK ); +if (perm &KEY_NEED_INVAL ) + oldstyle_perm |=KEY_NEED_SEARCH ; +if (perm &KEY_NEED_REVOKE ) + oldstyle_perm |=KEY_NEED_WRITE ; +if (perm &KEY_NEED_SETSEC ) + oldstyle_perm |=0x20 ; +if (perm &KEY_NEED_JOIN ) + oldstyle_perm |=KEY_NEED_LINK ; +if (perm &KEY_NEED_CLEAR ) + oldstyle_perm |=KEY_NEED_WRITE ; + sid =cred_sid (cred); key =key_ref_to_ptr (key_ref); ksec = key->security; -return avc_has_perm (sid, ksec->sid,SECCLASS_KEY , perm,NULL ); +return avc_has_perm (sid, ksec->sid,SECCLASS_KEY , oldstyle_perm,NULL ); }static int selinux_key_getsecurity (struct key *key,char **_buffer)
security/smack/smack_lsm.c +2 -1
@@ -4377,7 +4377,8 @@ static int smack_key_permission(key_ref_t key_ref,#endif if (perm &KEY_NEED_READ ) request =MAY_READ ; -if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETATTR )) +if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETSEC | +KEY_NEED_INVAL |KEY_NEED_REVOKE |KEY_NEED_CLEAR )) request =MAY_WRITE ; rc =smk_access (tkp, keyp->security, request, &ad); rc =smk_bu_note ("key access" , tkp, keyp->security, request, rc);
David Howells
maintainerQuoted 7 lines · David Howells
Here's a patch that partially implements ACLs on keys. I haven't yet done KEYCTL_[GS]ET_ACL, but
KEYCTL_SETATTR,KEYCTL_DESCRIBEand/proc/keyshave been modified to deal with it.Most of the keyutils testsuite passes, just the permissions tests that don't because there isn't a fully compatible mapping from SETATTR -> ACL -> DESCRIBE.
Here's an updated patch. It still doesn't implement GET/SET_ACL, but it passes almost all the tests in the keyutils testsuite, barring:
(1) tests/keyctl/dh_compute/badargs: The first wrong-key-type test now returns EOPNOTSUPP rather than ENOKEY as READ permission isn't removed if the type doesn't have ->read(). You still can't actually read the key.
(2) tests/keyctl/permitting/valid: The view-other-permissions test doesn't work as Other has been replaced with Everyone in the ACL.
All other tests still work.
---
commit 2b5fa6ac075006f129b376886a5e8b41d47efad1
Author: David Howells <[email protected]> Date: Tue Sep 26 09:45:26 2017 +0100
KEYS: Replace uid/gid/perm permissions checking with ACL
Replace the uid/gid/perm permissions checking on a key with an ACL to allow the SETATTR permission to be split. The problem is that SETATTR covers a slew of things, not all of which should be grouped together. This includes:
(1) Changing the key ownership.
(2) Changing the security information.
(3) Keyring restriction.
(4) Expiry time.
(5) Revocation.
and it has also been proposed to add:
(6) Invalidation.
The above can be divided into three groups: Controlling access (1), (2) and (3), managing the content at construction time (4) and managing the key (5) and (6).
Add an ACL set & get functions that use a list of elements of the following form:
struct key_ace {
union {
uid_t uid;
gid_t gid;
unsigned int special_id;
};
unsigned int mask;
};
where the top four bits of mask indicate the subject being checked:
KEY_ACE_SPECIAL A specific value
KEY_ACE_UID A nominated UID
KEY_ACE_GID A nominated GID
and if SPECIAL is specified, special_id is one of the following:
KEY_ACE_POSSESSOR The possessor of the key
KEY_ACE_OWNER The owner of the key
KEY_ACE_GROUP The key's group
KEY_ACE_OTHER Everyone else
Each special ID may only occur once in an ACL.
The bottom 28 bits of mask indicate the permissions being granted:
KEY_ACE_VIEW Can view the key metadata
KEY_ACE_READ Can read the key content
KEY_ACE_WRITE Can update/modify the key content
KEY_ACE_SEARCH Can find the key by searching/requesting
KEY_ACE_LINK Can make a link to the key
KEY_ACE_INVAL Can invalidate
KEY_ACE_REVOKE Can revoke
KEY_ACE_SET_SECURITY Can set security
KEY_ACE_JOIN Can join this keyring
KEY_ACE_CLEAR Can clear this keyring
The ACEs are purely additive; all ACEs that apply are considered. I figure that it's unlikely that you'll give the key's Group permissions that you wouldn't give the Owner and Other permissions that you wouldn't give to both Group and Owner. Possessor permissions are already additive.
The functions are:
keyctl(KEYCTL_GET_ACL, key_serial_t key,
struct key_ace *acl_buffer, unsigned int buflen);
keyctl(KEYCTL_SET_ACL, key_serial_t key,
const struct key_ace *acl, unsigned int len);
The KEYCTL_SETATTR function is then deprecated. If called, it will construct an ACL to reflect the mask it is given, using possessor, owner, group and other ACE's as appropriate if any of those elements are granted any permissions. SETATTR permission turns on all of INVAL, REVOKE and SET_SECURITY. WRITE permission turns on WRITE, REVOKE and, if a keyring, CLEAR. JOIN is turned on if a keyring is being altered. KEYCTL_SETATTR will return an error if SETACL has been called on a key.
The KEYCTL_DESCRIBE function then creates a permissions mask to return depending on possessor, owner, group and other ACEs, indicating SETATTR if any of INVAL, REVOKE and SET_SECURITY are set and indicating WRITE if any of WRITE, REVOKE or CLEAR are set.
Note that the value subsequently returned by KEYCTL_DESCRIBE may not match the value set with KEYCTL_SETATTR - but this is already true because keys that lack ->read() can't have READ set and keys that lack ->write() can't have WRITE set.
The KEYCTL_SET_TIMEOUT function then is permitted if WRITE or SETSEC is set, or if the caller has a valid instantiation auth token.
The KEYCTL_INVALIDATE function then requires INVAL.
The KEYCTL_REVOKE function then requires REVOKE.
The KEYCTL_JOIN_SESSION_KEYRING function then requires JOIN to join an existing keyring.
The JOIN permission is enabled by default for session keyrings and manually created keyrings only.
certs/blacklist.c +28 -5
@@ -23,6 +23,32 @@static struct key *blacklist_keyring; +static struct key_acl blacklist_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_READ ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; + +static struct key_acl blacklist_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; +/* * The description must be a type prefix, a colon and then an even number of * hex digits. The hash is kept in the description.
@@ -93,8 +119,7 @@ int mark_hash_blacklisted(const char *hash) hash,NULL ,0 , - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW ), + &blacklist_key_acl,KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);if (IS_ERR (key)) {
@@ -153,9 +178,7 @@ static int __init blacklist_init(void)keyring_alloc (".blacklist" ,KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - (KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ | -KEY_USR_SEARCH , + &blacklist_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA |KEY_FLAG_KEEP ,NULL ,NULL );
certs/system_keyring.c +30 -9
@@ -27,6 +27,33 @@ static struct key *secondary_trusted_keys;extern __initconst const u8 system_certificate_list[];extern __initconst const unsigned long system_certificate_list_size; +static struct key_acl trusted_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_READ ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +}; + +static struct key_acl trusted_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask = (KEY_ACE_SPECIAL | +KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH |KEY_ACE_WRITE ), + .special_id =KEY_ACE_OWNER , + }, +}; +/** * restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA *
@@ -99,9 +126,7 @@ static __init int system_trusted_keyring_init(void) builtin_trusted_keys keyring_alloc(".builtin_trusted_keys",KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ |KEY_USR_SEARCH ), -KEY_ALLOC_NOT_IN_QUOTA , + &trusted_key_acl,KEY_ALLOC_NOT_IN_QUOTA , NULL, NULL);if (IS_ERR (builtin_trusted_keys))panic ("Can't allocate builtin trusted keyring\n" );
@@ -110,10 +135,7 @@ static __init int system_trusted_keyring_init(void) secondary_trusted_keys keyring_alloc(".secondary_trusted_keys",KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ |KEY_USR_SEARCH | -KEY_USR_WRITE ), -KEY_ALLOC_NOT_IN_QUOTA , + &trusted_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,get_builtin_and_secondary_restriction (), NULL);if (IS_ERR (secondary_trusted_keys))
@@ -163,8 +185,7 @@ static __init int load_system_certificate_list(void)NULL , p, plen, - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ ), + &trusted_key_acl,KEY_ALLOC_NOT_IN_QUOTA |KEY_ALLOC_BUILT_IN | KEY_ALLOC_BYPASS_RESTRICTION);
fs/afs/security.c +1 -1
@@ -28,7 +28,7 @@ struct key *afs_request_key(struct afs_cell *cell)_debug ("key %s" , cell->anonymous_key->description); key =request_key (&key_type_rxrpc, cell->anonymous_key->description, -NULL ); +NULL ,NULL );if (IS_ERR (key)) {if (PTR_ERR (key) != -ENOKEY ) {_leave (" = %ld" ,PTR_ERR (key));
fs/cifs/cifs_spnego.c +37 -4
65 lines not on this page · raw message
fs/cifs/cifsacl.c +39 -5
74 lines not on this page · raw message
fs/cifs/connect.c +2 -2
@@ -2469,7 +2469,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses) }cifs_dbg (FYI ,"%s: desc=%s\n" , __func__, desc); - key =request_key (&key_type_logon, desc,"" ); + key =request_key (&key_type_logon, desc,"" ,NULL );if (IS_ERR (key)) {if (!ses->domainName) {cifs_dbg (FYI ,"domainName is NULL\n" );
@@ -2480,7 +2480,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses)/* didn't work, try to find a domain key */ sprintf (desc,"cifs:d:%s" , ses->domainName);cifs_dbg (FYI ,"%s: desc=%s\n" , __func__, desc); - key =request_key (&key_type_logon, desc,"" ); + key =request_key (&key_type_logon, desc,"" ,NULL );if (IS_ERR (key)) { rc =PTR_ERR (key); goto out_err;
fs/fscache/object-list.c +1 -1
@@ -323,7 +323,7 @@ static void fscache_objlist_config(struct fscache_objlist_data *data)const char *buf;int len; - key =request_key (&key_type_user,"fscache:objlist" ,NULL ); + key =request_key (&key_type_user,"fscache:objlist" ,NULL ,NULL );if (IS_ERR (key)) goto no_config;
fs/nfs/nfs4idmap.c +38 -9
82 lines not on this page · raw message
include/linux/key.h +41 -22
151 lines not on this page · raw message
include/linux/keyctl.h new file +3 -0
@@ -0,0 +1,3 @@ +#define key_ace user_key_ace +#include <uapi/linux/keyctl.h> +#undef key_ace
include/uapi/linux/keyctl.h +36 -0
@@ -14,6 +14,40 @@#include <linux/types.h> +/* Keyring ACL definitions */ +struct key_ace { +unsigned int mask; +#define KEY_ACE_VIEW 0x00000001/* Can describe the key */ +#define KEY_ACE_READ 0x00000002/* Can read the key content */ +#define KEY_ACE_WRITE 0x00000004/* Can update/modify the key content */ +#define KEY_ACE_SEARCH 0x00000008/* Can find the key by search */ +#define KEY_ACE_LINK 0x00000010/* Can make a link to the key */ +#define KEY_ACE_INVAL 0x00000020/* Can invalidate the key */ +#define KEY_ACE_REVOKE 0x00000040/* Can revoke the key */ +#define KEY_ACE_SET_SECURITY 0x00000080/* Can set owner, group, ACL */ +#define KEY_ACE_JOIN 0x00000100/* Can join keyring */ +#define KEY_ACE_CLEAR 0x00000200/* Can clear keyring */ +#define KEY_ACE__ORDINARY 0x0000001f/* Ordinary permissions */ +#define KEY_ACE__PERMS 0x0fffffff +#define KEY_ACE_SPECIAL 0x10000000/* A specific subject */ +#define KEY_ACE_UID 0x20000000/* A nominated UID */ +#define KEY_ACE_GID 0x30000000/* A nominated GID */ +#define KEY_ACE__IDENTITY 0xf0000000 + +union { +uid_t uid; +gid_t gid; +unsigned int special_id; +#define KEY_ACE_EVERYONE 1/* Everyone, including owner and group */ +#define KEY_ACE_GROUP 2/* The key's group */ +#define KEY_ACE_OWNER 3/* The owner of the key */ +#define KEY_ACE_POSSESSOR 4/* Any process that possesses of the key */ +#define KEY_ACE_ROOT 5/* The user namespace root user */ +#define KEY_ACE_SYS_ADMIN 6/* Anyone with CAP_SYS_ADMIN */ +#define KEY_ACE_NET_ADMIN 7/* Anyone with CAP_NET_ADMIN */ + }; +}; +/* special process keyring shortcut IDs */ #define KEY_SPEC_THREAD_KEYRING -1/* - key ID for thread-specific keyring */ #define KEY_SPEC_PROCESS_KEYRING -2/* - key ID for process-specific keyring */
@@ -61,6 +95,8 @@#define KEYCTL_GET_PERSISTENT 22/* get a user's persistent keyring */ #define KEYCTL_DH_COMPUTE 23/* Compute Diffie-Hellman values */ #define KEYCTL_RESTRICT_KEYRING 29/* Restrict keys allowed to link to a keyring */ +#define KEYCTL_GET_ACL 30/* Get a key's ACL */ +#define KEYCTL_SET_ACL 31/* Set a key's ACL */ /* keyctl structures */ struct keyctl_dh_params {
net/dns_resolver/dns_key.c +18 -3
@@ -45,6 +45,23 @@ const struct cred *dns_resolver_cache;#define DNS_ERRORNO_OPTION "dnserror" +static struct key_acl dns_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =3 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_SEARCH |KEY_ACE_WRITE , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, + .aces[2 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_CLEAR , + .special_id =KEY_ACE_NET_ADMIN , + }, +}; +/* * Preparse instantiation data for a dns_resolver key. *
@@ -279,8 +296,7 @@ static int __init init_dns_resolver(void) keyring =keyring_alloc (".dns_resolver" ,GLOBAL_ROOT_UID ,GLOBAL_ROOT_GID , cred, - (KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ , + &dns_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL ,NULL );if (IS_ERR (keyring)) { ret =PTR_ERR (keyring);
@@ -293,7 +309,6 @@ static int __init init_dns_resolver(void)/* instruct request_key() to use this special keyring as a cache for * the results it looks up */ -set_bit (KEY_FLAG_ROOT_CAN_CLEAR , &keyring->flags); cred->thread_keyring = keyring; cred->jit_keyring =KEY_REQKEY_DEFL_THREAD_KEYRING ; dns_resolver_cache = cred;
net/dns_resolver/dns_query.c +18 -4
@@ -46,6 +46,23 @@#include "internal.h" +static struct key_acl dns_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =3 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE__PERMS , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, + .aces[2 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_INVAL , + .special_id =KEY_ACE_NET_ADMIN , + }, +}; +/** * dns_query - Query the DNS * @type: Query type (or NULL for straight host->IP lookup)
@@ -122,7 +139,7 @@ int dns_query(const char *type, const char *name, size_t namelen,* add_key() to preinstall malicious redirections */ saved_cred =override_creds (dns_resolver_cache); - rkey =request_key (&key_type_dns_resolver, desc, options); + rkey =request_key (&key_type_dns_resolver, desc, options, &dns_key_acl);revert_creds (saved_cred);kfree (desc);if (IS_ERR (rkey)) {
@@ -131,9 +148,6 @@ int dns_query(const char *type, const char *name, size_t namelen, }down_read (&rkey->sem); -set_bit (KEY_FLAG_ROOT_CAN_INVAL , &rkey->flags); - rkey->perm |=KEY_USR_VIEW ; - ret =key_validate (rkey);if (ret <0 ) goto put;
net/rxrpc/key.c +12 -3
@@ -27,6 +27,15 @@#include <keys/user-type.h> #include "ar-internal.h" +static struct key_acl rxrpc_null_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =1 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_SEARCH , + .special_id =KEY_ACE_POSSESSOR , + }, +}; +static int rxrpc_vet_description_s (const char *);static int rxrpc_preparse (struct key_preparsed_payload *);static int rxrpc_preparse_s (struct key_preparsed_payload *);
@@ -912,7 +921,7 @@ int rxrpc_request_key(struct rxrpc_sock *rx, char __user *optval, int optlen)if (IS_ERR (description))return PTR_ERR (description); - key =request_key (&key_type_rxrpc, description,NULL ); + key =request_key (&key_type_rxrpc, description,NULL ,NULL );if (IS_ERR (key)) {kfree (description);_leave (" = %ld" ,PTR_ERR (key));
@@ -943,7 +952,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval,if (IS_ERR (description))return PTR_ERR (description); - key =request_key (&key_type_keyring, description,NULL ); + key =request_key (&key_type_keyring, description,NULL ,NULL );if (IS_ERR (key)) {kfree (description);_leave (" = %ld" ,PTR_ERR (key));
@@ -1024,7 +1033,7 @@ struct key *rxrpc_get_null_key(const char *keyname) key =key_alloc (&key_type_rxrpc, keyname,GLOBAL_ROOT_UID ,GLOBAL_ROOT_GID , cred, -KEY_POS_SEARCH ,KEY_ALLOC_NOT_IN_QUOTA ,NULL ); + &rxrpc_null_key_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL );if (IS_ERR (key))return key;
security/keys/encrypted-keys/encrypted.c +1 -1
@@ -303,7 +303,7 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_kconst struct user_key_payload *upayload;struct key *ukey; - ukey =request_key (&key_type_user, master_desc,NULL ); + ukey =request_key (&key_type_user, master_desc,NULL ,NULL );if (IS_ERR (ukey)) goto error;
security/keys/encrypted-keys/masterkey_trusted.c +1 -1
@@ -34,7 +34,7 @@ struct key *request_trusted_key(const char *trusted_desc,struct trusted_key_payload *tpayload;struct key *tkey; - tkey =request_key (&key_type_trusted, trusted_desc,NULL ); + tkey =request_key (&key_type_trusted, trusted_desc,NULL ,NULL );if (IS_ERR (tkey)) goto error;
security/keys/gc.c +1 -1
@@ -155,6 +155,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)atomic_dec (&key->user->nikeys);key_user_put (key->user); +key_put_acl (key->acl);kfree (key->description);
@@ -225,7 +226,6 @@ static void key_garbage_collector(struct work_struct *work)if (key->type = key_gc_dead_keytype) { gc_state |=KEY_GC_FOUND_DEAD_KEY ;set_bit (KEY_FLAG_DEAD , &key->flags); - key->perm =0 ; goto skip_dead_key; }else if (key->type = &key_type_keyring && key->restrict_link) {
security/keys/internal.h +8 -4
@@ -88,7 +88,8 @@ extern struct rb_root key_serial_tree;extern spinlock_t key_serial_lock;extern struct mutex key_construction_mutex;extern wait_queue_head_t request_key_conswq; - +extern struct key_acl default_key_acl; +extern struct key_acl joinable_keyring_acl;extern struct key_type *key_type_lookup (const char *type);extern void key_type_put (struct key_type *ktype);
@@ -153,13 +154,14 @@ extern struct key *request_key_and_link(struct key_type *type,const void *callout_info, size_t callout_len, void *aux, + struct key_acl *acl, struct key *dest_keyring, unsigned long flags);extern bool lookup_user_key_possessed (const struct key *key,const struct key_match_data *match_data);extern key_ref_t lookup_user_key (key_serial_t id,unsigned long flags, -key_perm_t perm); +u32 desired_perm);#define KEY_LOOKUP_CREATE 0x01#define KEY_LOOKUP_PARTIAL 0x02#define KEY_LOOKUP_FOR_UNLINK 0x04
@@ -178,7 +180,9 @@ 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); +u32 desired_perm); +extern unsigned int key_acl_to_perm (const struct key_acl *acl); +extern void key_put_acl (struct key_acl *acl);/* * Check to see whether permission is granted to use a key in the desired way.
@@ -234,7 +238,7 @@ extern long keyctl_keyring_search(key_serial_t, const char __user *, const char __user *, key_serial_t);extern long keyctl_read_key (key_serial_t ,char __user *,size_t );extern long keyctl_chown_key (key_serial_t ,uid_t ,gid_t ); -extern long keyctl_setperm_key (key_serial_t ,key_perm_t ); +extern long keyctl_setperm_key (key_serial_t ,unsigned int );extern long keyctl_instantiate_key (key_serial_t ,const void __user *,size_t ,key_serial_t );extern long keyctl_negate_key (key_serial_t ,unsigned ,key_serial_t );
security/keys/key.c +10 -19
@@ -199,7 +199,7 @@ static inline void key_alloc_serial(struct key *key)* @uid: The owner of the new key. * @gid: The group ID for the new key's group permissions. * @cred: The credentials specifying UID namespace. -* @perm: The permissions mask of the new key. +* @acl: The ACL to attach to the new key. * @flags: Flags specifying quota properties. * @restrict_link: Optional link restriction for new keyrings. *
@@ -227,7 +227,7 @@ static inline void key_alloc_serial(struct key *key)*/ struct key *key_alloc (struct key_type *type,const char *desc,kuid_t uid,kgid_t gid,const struct cred *cred, -key_perm_t perm,unsigned long flags, +struct key_acl *acl,unsigned long flags,struct key_restriction *restrict_link) {struct key_user *user =NULL ;
@@ -250,6 +250,9 @@ struct key *key_alloc(struct key_type *type, const char *desc, desclen =strlen (desc); quotalen = desclen +1 + type->def_datalen; +if (!acl) + acl = &default_key_acl; +/* get hold of the key tracking for this user */ user =key_user_lookup (uid);if (!user)
@@ -295,7 +298,8 @@ struct key *key_alloc(struct key_type *type, const char *desc, key->datalen = type->def_datalen; key->uid = uid; key->gid = gid; - key->perm = perm; +refcount_inc (&acl->usage); +rcu_assign_pointer (key->acl, acl); key->restrict_link = restrict_link;if (!(flags &KEY_ALLOC_NOT_IN_QUOTA ))
@@ -775,7 +779,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,* @description: The searchable description for the key. * @payload: The data to use to instantiate or update the key. * @plen: The length of @payload. -* @perm: The permissions mask for a new key. +* @acl: The ACL to attach if a key is created. * @flags: The quota flags for a new key. * * Search the destination keyring for a key of the same description and if one
@@ -798,7 +802,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, const char *description, const void *payload, size_t plen, - key_perm_t perm, + struct key_acl *acl, unsigned long flags) { struct keyring_index_key index_key = {
@@ -889,22 +893,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, goto found_matching_key; } -/* if the client doesn't provide, decide on the permissions we want */ -if (perm =KEY_PERM_UNDEF ) { - perm =KEY_POS_VIEW |KEY_POS_SEARCH |KEY_POS_LINK |KEY_POS_SETATTR ; - perm |=KEY_USR_VIEW ; - -if (index_key.type->read) - perm |=KEY_POS_READ ; - -if (index_key.type = &key_type_keyring || - index_key.type->update) - perm |=KEY_POS_WRITE ; - } -/* allocate a new key */ key =key_alloc (index_key.type, index_key.description, - cred->fsuid, cred->fsgid, cred, perm, flags,NULL ); + cred->fsuid, cred->fsgid, cred, acl, flags,NULL );if (IS_ERR (key)) { key_ref =ERR_CAST (key); goto error_link_end;
security/keys/keyctl.c +67 -51
243 lines not on this page · raw message
security/keys/keyring.c +12 -4
@@ -489,11 +489,19 @@ static long keyring_read(const struct key *keyring,return ctx.count; } -/* -* Allocate a keyring and link into the destination keyring. +/** +* keyring_alloc - Allocate a keyring and link into the destination +* @description: The key description to allow the key to be searched out. +* @uid: The owner of the new key. +* @gid: The group ID for the new key's group permissions. +* @cred: The credentials specifying UID namespace. +* @acl: The ACL to attach to the new key. +* @flags: Flags specifying quota properties. +* @restrict_link: Optional link restriction for new keyrings. +* @dest: Destination keyring. */ struct key *keyring_alloc (const char *description,kuid_t uid,kgid_t gid, -const struct cred *cred,key_perm_t perm, +const struct cred *cred,struct key_acl *acl,unsigned long flags,struct key_restriction *restrict_link,struct key *dest)
@@ -502,7 +510,7 @@ struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,int ret; keyring =key_alloc (&key_type_keyring, description, - uid, gid, cred, perm, flags, restrict_link); + uid, gid, cred, acl, flags, restrict_link);if (!IS_ERR (keyring)) { ret =key_instantiate_and_link (keyring,NULL ,0 , dest,NULL );if (ret <0 ) {
security/keys/permission.c +173 -38
242 lines not on this page · raw message
security/keys/persistent.c +29 -4
@@ -16,6 +16,33 @@unsigned persistent_keyring_expiry =3 *24 *3600 ;/* Expire after 3 days of non-use */ +static struct key_acl persistent_register_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_SEARCH |KEY_ACE_WRITE , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW |KEY_ACE_READ , + .special_id =KEY_ACE_OWNER , + }, +}; + +static struct key_acl persistent_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE__ORDINARY |KEY_ACE_CLEAR | +KEY_ACE_INVAL ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW |KEY_ACE_READ , + .special_id =KEY_ACE_OWNER , + }, +}; +/* * Create the persistent keyring register for the current user namespace. *
@@ -26,8 +53,7 @@ static int key_create_persistent_register(struct user_namespace *ns)struct key *reg =keyring_alloc (".persistent_register" ,KUIDT_INIT (0 ),KGIDT_INIT (0 ),current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ ), + &persistent_register_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL ,NULL );if (IS_ERR (reg))return PTR_ERR (reg);
@@ -60,8 +86,7 @@ static key_ref_t key_create_persistent(struct user_namespace *ns, kuid_t uid, persistent =keyring_alloc (index_key->description, uid,INVALID_GID ,current_cred (), - ((KEY_POS_ALL & ~KEY_POS_SETATTR ) | -KEY_USR_VIEW |KEY_USR_READ ), + &persistent_keyring_acl,KEY_ALLOC_NOT_IN_QUOTA ,NULL , ns->persistent_keyring_register);if (IS_ERR (persistent))
security/keys/proc.c +19 -6
@@ -176,13 +176,15 @@ static void proc_keys_stop(struct seq_file *p, void *v)static int proc_keys_show (struct seq_file *m,void *v) { +const struct key_acl *acl;struct rb_node *_p = v; struct key *key = rb_entry(_p, struct key, serial_node);struct timespec now;unsigned long timo;key_ref_t key_ref, skey_ref;char xbuf[16 ]; -int rc; +bool check_pos; +int i, rc;struct keyring_search_context ctx = { .index_key.type = key->type,
@@ -194,12 +196,25 @@ static int proc_keys_show(struct seq_file *m, void *v) .flags = KEYRING_SEARCH_NO_STATE_CHECK, }; - key_ref =make_key_ref (key,0 ); +rcu_read_lock (); + + acl =rcu_dereference (key->acl); + check_pos = false; +for (i =0 ; i < acl->nr_ace; i++) { +const struct key_ace *ace = &acl->aces[i]; +if (ace->special_id =KEY_ACE_POSSESSOR ) { +if ((ace->mask & (KEY_ACE__IDENTITY |KEY_ACE_VIEW )) = + (KEY_ACE_SPECIAL |KEY_ACE_VIEW )) + check_pos = true; +break ; + } + }/* determine if the key is possessed by this process (a test we can * skip if the key does not indicate the possessor can view it */ -if (key->perm &KEY_POS_VIEW ) { + key_ref =make_key_ref (key,0 ); +if (check_pos) { skey_ref =search_my_process_keyrings (&ctx);if (!IS_ERR (skey_ref)) {key_ref_put (skey_ref);
@@ -214,8 +229,6 @@ static int proc_keys_show(struct seq_file *m, void *v) now =current_kernel_time (); -rcu_read_lock (); -/* come up with a suitable timeout value */ if (key->expiry =0 ) {memcpy (xbuf,"perm" ,5 );
@@ -250,7 +263,7 @@ static int proc_keys_show(struct seq_file *m, void *v)showflag (key,'i' ,KEY_FLAG_INVALIDATED ),refcount_read (&key->usage), xbuf, - key->perm, +key_acl_to_perm (acl),from_kuid_munged (seq_user_ns (m), key->uid),from_kgid_munged (seq_user_ns (m), key->gid), key->type->name);
security/keys/process_keys.c +60 -20
161 lines not on this page · raw message
security/keys/request_key.c +25 -22
163 lines not on this page · raw message
security/keys/request_key_auth.c +15 -2
@@ -29,6 +29,19 @@ static void request_key_auth_revoke(struct key *);static void request_key_auth_destroy (struct key *);static long request_key_auth_read (const struct key *,char __user *,size_t ); +static struct key_acl request_key_auth_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_SEARCH , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, +}; +/* * The request-key authorisation key type definition. */
@@ -203,8 +216,8 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info, authkey =key_alloc (&key_type_request_key_auth, desc, cred->fsuid, cred->fsgid, cred, -KEY_POS_VIEW |KEY_POS_READ |KEY_POS_SEARCH | -KEY_USR_VIEW ,KEY_ALLOC_NOT_IN_QUOTA ,NULL ); + &request_key_auth_acl, +KEY_ALLOC_NOT_IN_QUOTA ,NULL );if (IS_ERR (authkey)) { ret =PTR_ERR (authkey); goto error_free_rka;
security/selinux/hooks.c +15 -1
@@ -6157,6 +6157,7 @@ static int selinux_key_permission(key_ref_t key_ref, {struct key *key;struct key_security_struct *ksec; +unsigned oldstyle_perm;u32 sid;/* if no specific permissions are requested, we skip the
@@ -6165,12 +6166,25 @@ static int selinux_key_permission(key_ref_t key_ref,if (perm =0 )return 0 ; + oldstyle_perm = perm & (KEY_NEED_VIEW |KEY_NEED_READ |KEY_NEED_WRITE | +KEY_NEED_SEARCH |KEY_NEED_LINK ); +if (perm &KEY_NEED_INVAL ) + oldstyle_perm |=KEY_NEED_SEARCH ; +if (perm &KEY_NEED_REVOKE ) + oldstyle_perm |=KEY_NEED_WRITE ; +if (perm &KEY_NEED_SETSEC ) + oldstyle_perm |=0x20 ; +if (perm &KEY_NEED_JOIN ) + oldstyle_perm |=KEY_NEED_LINK ; +if (perm &KEY_NEED_CLEAR ) + oldstyle_perm |=KEY_NEED_WRITE ; + sid =cred_sid (cred); key =key_ref_to_ptr (key_ref); ksec = key->security; -return avc_has_perm (sid, ksec->sid,SECCLASS_KEY , perm,NULL ); +return avc_has_perm (sid, ksec->sid,SECCLASS_KEY , oldstyle_perm,NULL ); }static int selinux_key_getsecurity (struct key *key,char **_buffer)
security/smack/smack_lsm.c +2 -1
@@ -4377,7 +4377,8 @@ static int smack_key_permission(key_ref_t key_ref,#endif if (perm &KEY_NEED_READ ) request =MAY_READ ; -if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETATTR )) +if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETSEC | +KEY_NEED_INVAL |KEY_NEED_REVOKE |KEY_NEED_CLEAR )) request =MAY_WRITE ; rc =smk_access (tkp, keyp->security, request, &ad); rc =smk_bu_note ("key access" , tkp, keyp->security, request, rc);
Hi David,
Quoted 16 lines · David Howells
Replace the uid/gid/perm permissions checking on a key with an ACL to allow the SETATTR permission to be split. The problem is that SETATTR covers a slew of things, not all of which should be grouped together. This includes:
(1) Changing the key ownership.
(2) Changing the security information.
(3) Keyring restriction.
(4) Expiry time.
(5) Revocation.
Quoted 7 lines
and it has also been proposed to add:
(6) Invalidation.
The above can be divided into three groups: Controlling access (1), (2) and (3), managing the content at construction time (4) and managing the key (5) and (6).
This is interesting work, though it adds complexity and makes a lot of subtle (and potentially breaking) changes to which permissions are required for various things. First I think you need to start out with a better statement of the problems you are trying to solve. The patch does much more than simply split up the SETATTR permission --- for example, it also adds the ability to assign permissions to specific uids, gids, and capabilities. Who is planning to use those features and why?
Quoted 1 line
The
KEYCTL_SETATTRfunction is then deprecated. If called, it will
KEYCTL_SETPERM
Quoted 5 lines
construct an ACL to reflect the mask it is given, using possessor, owner, group and other ACE's as appropriate if any of those elements are granted any permissions. SETATTR permission turns on all of INVAL, REVOKE and
SET_SECURITY. WRITE permission turns on WRITE, REVOKE and, if a keyring, CLEAR. JOIN is turned on if a keyring is being altered.
The proposed changes to keyctl_setperm_key() actually never enable INVAL at all, which doesn't match the description here. Also, all breaking changes need to be justified. If keyctl_setperm(key, KEY_*_SEARCH) is no longer going to allow the key to be invalidated (as I had proposed earlier), that is really its own change which needs its own justification; it shouldn't be hidden in a larger patch.
Quoted 1 line
will return an error if SETACL has been called on a key.
That is simplest, but it doesn't match the behavior of POSIX ACLs, for example. With POSIX ACLs you can still chmod() a file that has an ACL.
Quoted 4 lines
The
KEYCTL_DESCRIBEfunction then creates a permissions mask to return depending on possessor, owner, group and other ACEs, indicating SETATTR if any of INVAL, REVOKE andSET_SECURITYare set and indicating WRITE if any of WRITE, REVOKE or CLEAR are set.
Ignoring ACEs for specific users, groups, and capabilities may be problematic because the returned mask will under-estimate rather than over-estimate the permissions that have been granted. With POSIX ACLs, for example, the union of all permissions that have been granted to any subjects other than the regular ones is reflected in the group entry. I believe that's generally considered better from a security perspective, because then no permissions are "hidden" from a listing of the regular (non-ACL) permissions only.
Quoted 4 lines
Note that the value subsequently returned by
KEYCTL_DESCRIBEmay not match the value set withKEYCTL_SETATTR- but this is already true because keys that lack ->read() can't have READ set and keys that lack ->write() can't have WRITE set.
Not true; you *can* set READ on a key that lacks ->read() and WRITE on a key that lacks ->update(). They are only omitted from the default permissions.
Quoted 2 lines
The
KEYCTL_SET_TIMEOUTfunction then is permitted if WRITE or SETSEC is set, or if the caller has a valid instantiation auth token.
This doesn't match the code, which asks for WRITE permission only. It's also a breaking change which needs to be justified on its own. Also I'm not sure that WRITE permission actually makes sense, given that KEYCTL_SET_TIMEOUT doesn't modify the payload of the key.
certs/blacklist.c line 37
Quoted 12 lines
certs/blacklist.c +12
-0+static struct key_acl blacklist_key_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_SEARCH |KEY_ACE_READ ), + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL | (KEY_ACE_VIEW |KEY_ACE_SEARCH ), + .special_id =KEY_ACE_OWNER , + }, +};
Designators into flexible arrays are a gcc extension which doesn't work with clang. Use this instead:
.aces = {
{
.mask = KEY_ACE_SPECIAL | (KEY_ACE_SEARCH | KEY_ACE_READ),
.special_id = KEY_ACE_POSSESSOR,
},
{
.mask = KEY_ACE_SPECIAL | (KEY_ACE_VIEW | KEY_ACE_SEARCH),
.special_id = KEY_ACE_OWNER,
},
},It's also difficult to read these lists of ACEs. An ACE should read as "who", then "what". But now it reads as "part of who", then "what", then "the rest of who". It may be helpful to define some macros to create the entries more concisely:
.aces = {
KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_READ),
KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_SEARCH),
}include/linux/keyctl.h line 3
Quoted 9 lines
include/linux/keyctl.h new file +3
-0@@ -0,0 +1,3 @@ +#define key_ace user_key_ace +#include <uapi/linux/keyctl.h> +#undef key_ace
This is ugly. How about using kernel_key_ace for the internal kernel version?
Quoted 1 line
+#define
KEY_ACE_SPECIAL0x10000000/* A specific subject */
What is a "specific subject"?
Quoted 1 line
+#define
KEY_ACE_ROOT5 /* The user namespace root user */
Which user namespace?
include/linux/keyctl.h
Quoted 2 lines
include/linux/keyctl.h new file +2
-0+#define KEY_ACE_SYS_ADMIN 6/* Anyone with CAP_SYS_ADMIN */ +#define KEY_ACE_NET_ADMIN 7/* Anyone with CAP_NET_ADMIN */
In what user namespace?
include/linux/keyctl.h
Quoted 2 lines
include/linux/keyctl.h new file +2
-0+#define KEYCTL_GET_ACL 30/* Get a key's ACL */ +#define KEYCTL_SET_ACL 31/* Set a key's ACL */
The implementations of these don't seem to be included in the patch.
Quoted 12 lines
include/linux/keyctl.h new file +1
-5@@ -461,31 +441,17 @@ long keyctl_keyring_clear(key_serial_t ringid)struct key *keyring;long ret; - keyring_ref =lookup_user_key (ringid,KEY_LOOKUP_CREATE ,KEY_NEED_WRITE ); + keyring_ref =lookup_user_key (ringid,KEY_LOOKUP_CREATE ,KEY_NEED_CLEAR );if (IS_ERR (keyring_ref)) { ret =PTR_ERR (keyring_ref); - -/* Root is permitted to invalidate certain special keyrings */ -if (capable (CAP_SYS_ADMIN )) { - keyring_ref =lookup_user_key (ringid,0 ,0 );
include/linux/keyctl.h long keyctl_keyring_clear(key_serial_t ringid) · line 448
Quoted 10 lines
include/linux/keyctl.h new file +0
-8-if (IS_ERR (keyring_ref)) - goto error; -if (test_bit (KEY_FLAG_ROOT_CAN_CLEAR , - &key_ref_to_ptr (keyring_ref)->flags)) - goto clear; - goto error_put; - } - goto error; }
The CLEAR permission is weird because WRITE is a superset of it. (Clearing a keyring is equivalent to unlinking all keys in it.) Permissions should be orthogonal. Did you consider instead having one permission for adding links to a keyring and one for deleting links? I'm thinking about use cases similar to that which the sticky bit on files is used for...
include/linux/keyctl.h long keyctl_keyring_clear(key_serial_t ringid) · line 456
Quoted 8 lines
include/linux/keyctl.h new file +8
-0+for (i =0 ; i <4 ; i++) { +struct key_ace *ace = &acl->aces[j]; +unsigned int subset = (perm >> (i *8 )) &0x3f ; + +if (!subset) +continue ; + ace->special_id =KEY_ACE_EVERYONE + i; + ace->mask =KEY_ACE_SPECIAL | (subset &0x1f );
No magic numbers please. What are 0x3f and 0x1f?
Also, this code is assuming that the subject values are numbered in a certain way. There should be some BUILD_BUG_ON()s to verify that.
include/linux/keyctl.h long keyctl_keyring_clear(key_serial_t ringid)
Quoted 6 lines
include/linux/keyctl.h new file +6
-0+if (subset & (KEY_OTH_WRITE |KEY_OTH_SETATTR )) + ace->mask |=KEY_ACE_REVOKE ; +if (subset &KEY_OTH_SETATTR ) + ace->mask |=KEY_ACE_SET_SECURITY ; +if (key->type = &key_type_keyring) { + ace->mask |=KEY_ACE_JOIN ;
Why is JOIN permission always given to keyrings? If someone has JOIN permission (or LINK permission, for that matter) on a keyring than they can acquire the possessor permissions. So always giving JOIN defeats the point of having all the different non-possessor permissions...
include/linux/keyctl.h
Quoted 3 lines
include/linux/keyctl.h new file +3
-0+if (subset &KEY_OTH_WRITE ) + ace->mask |=KEY_ACE_CLEAR ; + }
Granting INVAL permission is missing.
include/linux/keyctl.h long keyctl_set_timeout(key_serial_t id, unsigned timeout) · line 1352
Quoted 6 lines
include/linux/keyctl.h new file +1
-1@@ -1333,7 +1349,7 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout)long ret; key_ref =lookup_user_key (id,KEY_LOOKUP_CREATE |KEY_LOOKUP_PARTIAL , -KEY_NEED_SETATTR ); +KEY_NEED_WRITE );
As mentioned earlier, this is a breaking change which needs to be justified on its own.
Quoted 16 lines
include/linux/keyctl.h new file +16
-0+rcu_read_lock (); + + acl =rcu_dereference (key->acl); +if (!acl || acl->nr_ace =0 ) + goto no_access_rcu; + +for (i =0 ; i < acl->nr_ace; i++) { +const struct key_ace *ace = &acl->aces[i]; + +switch (ace->mask &KEY_ACE__IDENTITY ) { +case KEY_ACE_SPECIAL : +switch (ace->special_id) { +case KEY_ACE_POSSESSOR : +if (is_key_possessed (key_ref)) + allow |= ace->mask; +break ;
include/linux/keyctl.h long keyctl_set_timeout(key_serial_t id, unsigned timeout)
Quoted 10 lines
include/linux/keyctl.h new file +10
-0+case KEY_ACE_OWNER : +if (uid_eq (key->uid, cred->fsuid)) + allow |= ace->mask; +break ; +case KEY_ACE_GROUP : +if (gid_valid (key->gid) && +gid_eq (key->gid, cred->fsgid)) + allow |= ace->mask; +else if (groups_search (cred->group_info, key->gid)) + allow |= ace->mask;
Why call groups_search() when the key gid isn't valid?
include/linux/keyctl.h
Quoted 4 lines
include/linux/keyctl.h new file +4
-0+case KEY_ACE_ROOT : +if (uid_eq (key->uid, cred->user_ns->owner)) + allow |= ace->mask; +break ;
Not sure I understand this. So if someone owns the key, then they actually have all the "root" privileges as well, since they can create and enter a new user namespace where they are "root"? That may not be what people expect.
include/linux/keyctl.h
Quoted 4 lines
include/linux/keyctl.h new file +4
-0+case KEY_ACE_SYS_ADMIN : +if (ns_capable (&init_user_ns,CAP_SYS_ADMIN )) + allow |= ace->mask; +break ;
Also known as capable(CAP_SYS_ADMIN). But also, why does it handle namespaces differently from KEY_ACE_ROOT?
include/linux/keyctl.h
Quoted 6 lines
include/linux/keyctl.h new file +6
-0+case KEY_ACE_NET_ADMIN : +if (ns_capable (&init_user_ns,CAP_NET_ADMIN )) + allow |= ace->mask; +break ; + } +break ;
capable(CAP_NET_ADMIN), and see above.
include/linux/keyctl.h
Quoted 2 lines
include/linux/keyctl.h new file +2
-0+if (!(allow & desired_perm)) + goto no_access;
This is wrong because 'desired_perm' may contain multiple permissions --- and if *any* are denied, then the permission check should fail. It should be:
if (desired_perm & ~allow) goto no_access;
Quoted 14 lines
include/linux/keyctl.h new file +14
-0+unsigned int key_acl_to_perm (const struct key_acl *acl) +{ +unsigned int perm =0 ; +int i; + +if (!acl || acl->nr_ace =0 ) +return 0 ; + +for (i =0 ; i < acl->nr_ace; i++) { +const struct key_ace *ace = &acl->aces[i]; +unsigned int mask = ace->mask &KEY_ACE__PERMS ; + +switch (ace->mask &KEY_ACE__IDENTITY ) { +case KEY_ACE_SPECIAL :
include/linux/keyctl.h
Quoted 10 lines
include/linux/keyctl.h new file +10
-0+switch (ace->special_id) { +case KEY_ACE_POSSESSOR : + perm |= (mask &0x1f ) <<24 ; +if (mask & (KEY_ACE_INVAL |KEY_ACE_SET_SECURITY )) + perm |=KEY_POS_SETATTR ; +if (mask &KEY_ACE_CLEAR ) + perm |=KEY_POS_WRITE ; +if ((mask &KEY_ACE_REVOKE ) && !(perm &KEY_POS_SETATTR )) + perm |=KEY_POS_WRITE ; +break ;
The handling for REVOKE is weird, since it makes it possible that by *adding* permissions, it can appear that WRITE permission was removed.
Also, why isn't JOIN handled here?
Quoted 18 lines
include/linux/keyctl.h new file +18
-0+case KEY_ACE_OWNER : + perm |= (mask &0x1f ) <<16 ; +if (mask & (KEY_ACE_INVAL |KEY_ACE_SET_SECURITY )) + perm |=KEY_USR_SETATTR ; +if (mask &KEY_ACE_CLEAR ) + perm |=KEY_USR_WRITE ; +if ((mask &KEY_ACE_REVOKE ) && !(perm &KEY_USR_SETATTR )) + perm |=KEY_USR_WRITE ; +break ; +case KEY_ACE_GROUP : + perm |= (mask &0x1f ) <<8 ; +if (mask & (KEY_ACE_INVAL |KEY_ACE_SET_SECURITY )) + perm |=KEY_GRP_SETATTR ; +if (mask &KEY_ACE_CLEAR ) + perm |=KEY_GRP_WRITE ; +if ((mask &KEY_ACE_REVOKE ) && !(perm &KEY_GRP_SETATTR )) + perm |=KEY_GRP_WRITE ; +break ;
include/linux/keyctl.h
Quoted 10 lines
include/linux/keyctl.h new file +10
-0+case KEY_ACE_EVERYONE : + perm |= (mask &0x1f ); +if (mask & (KEY_ACE_INVAL |KEY_ACE_SET_SECURITY )) + perm |=KEY_OTH_SETATTR ; +if (mask &KEY_ACE_CLEAR ) + perm |=KEY_OTH_WRITE ; +if ((mask &KEY_ACE_REVOKE ) && !(perm &KEY_OTH_SETATTR )) + perm |=KEY_OTH_WRITE ; +break ; + }
Again, ignoring the "non-special" ACEs will cause the returned permissions mask to be an under-estimate rather than an over-estimate. I'm not sure that's a good idea.
include/linux/keyctl.h
Quoted 13 lines
include/linux/keyctl.h new file +13
-0+static void key_acl_rcu (struct rcu_head *rcu) +{ +kfree (container_of(rcu ,struct key_acl ,rcu )); +} + +/* +* Destroy a key's ACL. +*/ +void key_put_acl (struct key_acl *acl) +{ +if (refcount_dec_and_test (&acl->usage)) +call_rcu (&acl->rcu, key_acl_rcu); +}
Use kfree_rcu().
include/linux/keyctl.h
Quoted 13 lines
include/linux/keyctl.h new file +13
-0+rcu_read_lock (); + + acl =rcu_dereference (key->acl); + check_pos = false; +for (i =0 ; i < acl->nr_ace; i++) { +const struct key_ace *ace = &acl->aces[i]; +if (ace->special_id =KEY_ACE_POSSESSOR ) { +if ((ace->mask & (KEY_ACE__IDENTITY |KEY_ACE_VIEW )) = + (KEY_ACE_SPECIAL |KEY_ACE_VIEW )) + check_pos = true; +break ; + } + }
Bug: this will break from the loop if it encounters an ACE for uid or gid 4 (the value of KEY_ACE_POSSESSOR). It needs to check for KEY_ACE_SPECIAL before special_id = KEY_ACE_POSSESSOR can be considered meaningful.
include/linux/keyctl.h
Quoted 12 lines
include/linux/keyctl.h new file +12
-0+static struct key_acl tp_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE__PERMS , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW , + .special_id =KEY_ACE_OWNER , + }, +};
It's not obvious what 'tp' means. How about:
static struct key_acl thread_keyring_acl = {
...
};#define process_keyring_acl thread_keyring_acl
Quoted 13 lines
include/linux/keyctl.h new file +13
-0+ +static struct key_acl session_keyring_acl = { + .usage =REFCOUNT_INIT (1 ), + .nr_ace =2 , + .aces[0 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE__PERMS , + .special_id =KEY_ACE_POSSESSOR , + }, + .aces[1 ] = { + .mask =KEY_ACE_SPECIAL |KEY_ACE_VIEW |KEY_ACE_READ |KEY_ACE_JOIN , + .special_id =KEY_ACE_OWNER , + }, +};
...
include/linux/keyctl.h
Quoted 7 lines
include/linux/keyctl.h new file +1
-2keyring =keyring_alloc ("_ses" , cred->uid, cred->gid, cred, -KEY_POS_ALL |KEY_USR_VIEW |KEY_USR_READ , - flags,NULL ,NULL ); + &session_keyring_acl, flags,NULL ,NULL );if (IS_ERR (keyring))return PTR_ERR (keyring);
Why give JOIN permission to anonymous session keyrings? They shouldn't be joinable --- and they weren't prior to this patch, since they were *not* given KEY_USR_SEARCH permission.
include/linux/keyctl.h long join_session_keyring(const char *name) · line 851
Quoted 13 lines
include/linux/keyctl.h new file +1
-2@@ -800,8 +836,7 @@ long join_session_keyring(const char *name)if (PTR_ERR (keyring) = -ENOKEY ) {/* not found - try and create a new one */ keyring =keyring_alloc ( - name, old->uid, old->gid, old, -KEY_POS_ALL |KEY_USR_VIEW |KEY_USR_READ |KEY_USR_LINK , + name, old->uid, old->gid, old, &joinable_keyring_acl,KEY_ALLOC_IN_QUOTA ,NULL ,NULL );if (IS_ERR (keyring)) { ret =PTR_ERR (keyring);@@ -815,6 +850,11 @@ long join_session_keyring(const char *name) goto error3; }
This is also a behavior change which needs to be explained and justified on its own. A named keyring created with keyctl_join_session_keyring(name) was not previously joinable by default, but after this patch it is.
include/linux/keyctl.h long join_session_keyring(const char *name) · line 856
Quoted 6 lines
include/linux/keyctl.h new file +5
-0+ ret =key_task_permission (make_key_ref (keyring, false), old, +KEY_NEED_JOIN ); +if (ret <0 ) + goto error3; +
find_keyring_by_name() also checks for SEARCH permission. Now this checks for JOIN as well. Is it intentional that two different permissions are being checked?
Quoted 13 lines
security/selinux/hooks.c +1
-0@@ -6157,6 +6157,7 @@ static int selinux_key_permission(key_ref_t key_ref, {struct key *key;struct key_security_struct *ksec; +unsigned oldstyle_perm;u32 sid;/* if no specific permissions are requested, we skip the
security/selinux/hooks.c static int selinux_key_permission(key_ref_t key_ref, · line 6172
Quoted 7 lines
security/selinux/hooks.c +4
-0@@ -6165,12 +6166,25 @@ static int selinux_key_permission(key_ref_t key_ref,if (perm =0 )return 0 ; + oldstyle_perm = perm & (KEY_NEED_VIEW |KEY_NEED_READ |KEY_NEED_WRITE | +KEY_NEED_SEARCH |KEY_NEED_LINK ); +if (perm &KEY_NEED_INVAL ) + oldstyle_perm |=KEY_NEED_SEARCH ;
Isn't this going to need to be SETATTR rather than SEARCH? Even if we can't update SELinux to be aware of the new-style permissions we still need to fix the "search also means delete" bug. Otherwise it will remain impossible to use SELinux to enforce a read-only view of a key hierarchy.
security/selinux/hooks.c static int selinux_key_permission(key_ref_t key_ref, · line 6174
Quoted 2 lines
security/selinux/hooks.c +2
-0+if (perm &KEY_NEED_REVOKE ) + oldstyle_perm |=KEY_NEED_WRITE ;
Another breaking change which needs to be explained and justified. Before either the write or setattr SELinux key permissions was sufficient for revocation, but now only write is.
security/selinux/hooks.c static int selinux_key_permission(key_ref_t key_ref, · line 6176
Quoted 2 lines
security/selinux/hooks.c +2
-0+if (perm &KEY_NEED_SETSEC ) + oldstyle_perm |=0x20 ;
Magic number.
security/selinux/hooks.c static int selinux_key_permission(key_ref_t key_ref, · line 6178
Quoted 2 lines
security/selinux/hooks.c +2
-0+if (perm &KEY_NEED_JOIN ) + oldstyle_perm |=KEY_NEED_LINK ;
The old-style permission for joining keyrings was SEARCH, not LINK. Probably it *should* have been LINK, but it's still a breaking change which needs to be justified on its own...
security/selinux/hooks.c static int smack_key_permission(key_ref_t key_ref, · line 4384
Quoted 10 lines
security/selinux/hooks.c +2
-1@@ -4377,7 +4377,8 @@ static int smack_key_permission(key_ref_t key_ref,#endif if (perm &KEY_NEED_READ ) request =MAY_READ ; -if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETATTR )) +if (perm & (KEY_NEED_WRITE |KEY_NEED_LINK |KEY_NEED_SETSEC | +KEY_NEED_INVAL |KEY_NEED_REVOKE |KEY_NEED_CLEAR )) request =MAY_WRITE ; rc =smk_access (tkp, keyp->security, request, &ad); rc =smk_bu_note ("key access" , tkp, keyp->security, request, rc);
Why does Smack ignore requests for SEARCH permission?
Eric
David Howells
maintainerQuoted 7 lines · Eric Biggers
This is interesting work, though it adds complexity and makes a lot of subtle (and potentially breaking) changes to which permissions are required for various things. First I think you need to start out with a better statement of the problems you are trying to solve. The patch does much more than simply split up the SETATTR permission --- for example, it also adds the ability to assign permissions to specific uids, gids, and capabilities. Who is planning to use those features and why?
I should split out the additional subject types (uid, gid, cap-based) into a separate patch, but having them in one patch helped design it.
Quoted 2 lines
The proposed changes to
keyctl_setperm_key()actually never enable INVAL at all
Fixed. Now done if KEY_xxx_SEARCH is given.
Quoted 4 lines
Quoted 1 line
will return an error if SETACL has been called on a key.
That is simplest, but it doesn't match the behavior of POSIX ACLs, for example. With POSIX ACLs you can still
chmod()a file that has an ACL.
Does chmod() remove a POSIX ACL from a file?
Quoted 8 lines
Quoted 4 lines
The
KEYCTL_DESCRIBEfunction then creates a permissions mask to return depending on possessor, owner, group and other ACEs, indicating SETATTR if any of INVAL, REVOKE andSET_SECURITYare set and indicating WRITE if any of WRITE, REVOKE or CLEAR are set.Ignoring ACEs for specific users, groups, and capabilities may be problematic because the returned mask will under-estimate rather than over-estimate the permissions that have been granted.
You have a point, but KEYCTL_DESCRIBE doesn't return the set of permissions that has been granted to the caller. The KEYCTL_DESCRIBE interface can't be made to accurately describe the ACL.
Quoted 2 lines
With POSIX ACLs, for example, the union of all permissions that have been granted to any subjects other than the regular ones
Define 'regular ones'. For the keys case, do you mean possessor, user, group and other?
Quoted 3 lines
is reflected in the group entry. I believe that's generally considered better from a security perspective, because then no permissions are "hidden" from a listing of the regular (non-ACL) permissions only.
I'm not sure how that helps. If there's a group ID set, then displaying extra permissions for it because, say, there's a matching UID-specific ACE present would be giving a false picture of the permissions set.
Quoted 7 lines
Quoted 4 lines
Note that the value subsequently returned by
KEYCTL_DESCRIBEmay not match the value set withKEYCTL_SETATTR- but this is already true because keys that lack ->read() can't have READ set and keys that lack ->write() can't have WRITE set.Not true; you *can* set READ on a key that lacks ->read() and WRITE on a key that lacks ->update(). They are only omitted from the default permissions.
You are right. This should be fixed, assuming we don't move to some other permissions model.
Quoted 7 lines
Quoted 2 lines
The
KEYCTL_SET_TIMEOUTfunction then is permitted if WRITE or SETSEC is set, or if the caller has a valid instantiation auth token.This doesn't match the code, which asks for WRITE permission only. It's also a breaking change which needs to be justified on its own. Also I'm not sure that WRITE permission actually makes sense, given that
KEYCTL_SET_TIMEOUTdoesn't modify the payload of the key.
I'm not sure I agree. Whilst it doesn't modify the payload of a key, it is *about* the lifetime of that payload IMO.
However, given that add_key() grants WRITE and SET_SECURITY/SETATTR permission to the caller (assuming they stick the new key in a keyring they 'possess') and request_key() grants permission directly to the upcall to set the timeout, I wonder how much that matters. It's just that it's not in the same class as changing the key permissions.
Quoted 2 lines
Designators into flexible arrays are a gcc extension which doesn't work with clang.
Fix clang? gcc is the standard. ;-)
Quoted 3 lines
It's also difficult to read these lists of ACEs. An ACE should read as "who", then "what". But now it reads as "part of who", then "what", then "the rest of who".
True. That makes it occupy less space, though. I could also flip the order of .mask and .special_id.
One thing I was wondering about is whether I should make the core ACLs R/O and skip the inc/dec on the refcount if is_kernel_rodata() on the ACL is true.
Quoted 4 lines
.aces = {
KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_READ),
KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_SEARCH),
}Yeah, that looks nicer.
Quoted 3 lines
Quoted 1 line
+#define
KEY_ACE_SPECIAL0x10000000/* A specific subject */What is a "specific subject"?
'Subject' as in the entity performing the action. 'Specific subject' as specified by .special_id. I'm not sure 'specific' is the right word, or 'special' for that matter, but it's how I specify 'macro' things like 'possessor' or 'this key's owner'.
Maybe KEY_ACE_FIXED_SUBJECT or KEY_ACE_MACRO_SUBJECT? I guess KEY_ACE_UID and KEY_ACE_GID more fit 'specific subject'.
Quoted 8 lines
Quoted 1 line
+#define
KEY_ACE_ROOT5 /* The user namespace root user */Which user namespace?
Review
Quoted 2 lines
(quoted diff) +2
-0+#define KEY_ACE_SYS_ADMIN 6 /* Anyone with CAP_SYS_ADMIN */ +#define KEY_ACE_NET_ADMIN 7 /* Anyone with CAP_NET_ADMIN */In what user namespace?
cred->user_ns as per the cred passed to key_task_permission().
Having dicussed these a bit with Eric Biederman, I'm thinking I probably want to rejig this, perhaps in favour of specifying the owner of a particular user namespace. However, I do think, as mentioned above, I need to split this bit out into a subsequent patch.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#define KEYCTL_GET_ACL 30 /* Get a key's ACL */ +#define KEYCTL_SET_ACL 31 /* Set a key's ACL */The implementations of these don't seem to be included in the patch.
As I stated. I do have implementations of these now, which I've attached to the bottom of this message.
Quoted 3 lines
The CLEAR permission is weird because WRITE is a superset of it. (Clearing a keyring is equivalent to unlinking all keys in it.) Permissions should be orthogonal.
I know, but I want to be able to grant just the ability to clear a keyring to the admin.
Quoted 3 lines
Did you consider instead having one permission for adding links to a keyring and one for deleting links? I'm thinking about use cases similar to that which the sticky bit on files is used for...
Actually, that would've been better - and possibly could've avoided the need for invalidate - though invalidate is applied to a specific key, not a keyring.
However, what do you do about key replacement where a link to an old key is replaced by add_key() with a link to a new key? Do you have to have both permits? Or a third REPLACE permit?
Quoted 1 line
No magic numbers please. What are
0x3fand0x1f?
Things I don't have symbols for. I would like to avoid using KEY_OTH_xxx when referring to values that weren't originally 'other'. I didn't succeed, though, as I'm sure you noticed, so I could just use combinations of KEY_OTH_xxx, I guess.
Quoted 2 lines
Also, this code is assuming that the subject values are numbered in a certain way.
The UAPI is defined so.
Further, key_task_permission() in current Linus will fail if this is not so.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (key->type = &key_type_keyring) { + ace->mask |= KEY_ACE_JOIN;Why is JOIN permission always given to keyrings?
It isn't, except if one calls KEYCTL_SETPERM or if a keyring is created by join_session_keyring() or as a normal session keyrign. add_key() creates keyrings with default_key_acl.
Quoted 4 lines
If someone has JOIN permission (or LINK permission, for that matter) on a keyring than they can acquire the possessor permissions. So always giving JOIN defeats the point of having all the different non-possessor permissions...
Without this patch, join_session_keyring() doesn't impose any permissions on joining a keyring, though find_keyring_by_name() requires SEARCH permission on a named keyring to be able to find it (and still does - though this should perhaps be switched to JOIN also).
Yes, LINK permission can be used to 'possess' a keyring - but only if you're granted it.
I can alter KEYCTL_SETPERM to only give KEY_ACE_JOIN if KEY_ACE_SEARCH.
The problem is that I have to supply JOIN to be backwardly compatible, which makes it arguable that I need to add it to default_key_acl also.
I should probably make it widely available in the main patch and add a separate patch limiting its availability.
Quoted 1 line
Granting INVAL permission is missing.
Fixed. Setting SEARCH sets INVAL in KEYCTL_SETPERM.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +1
-1key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL, - KEY_NEED_SETATTR); + KEY_NEED_WRITE);As mentioned earlier, this is a breaking change which needs to be justified on its own.
I can undo this and defer the thought to a later patch.
Quoted 1 line
Why call
groups_search()when the key gid isn't valid?
Fixed.
Quoted 1 line
This is wrong because '
desired_perm' may contain multiple permissions ---
No, it can't. I've fixed the comment.
It was never specified what was meant by specifying multiple permissions and I don't think I ever used that directly. The nearest I came was to make multiple calls to key_{,task_}permission() so that it was obvious in the caller what was meant.
Quoted 2 lines
The handling for REVOKE is weird, since it makes it possible that by *adding* permissions, it can appear that WRITE permission was removed.
I know. But there's no way to render an ACL down to an old-style permissions mask accurately. Revocation permission was enabled by having either SETATTR or WRITE permission, but WRITE permission was disabled under some circumstances if ->write() didn't exist.
Quoted 1 line
Also, why isn't JOIN handled here?
What would you represent it as?
Currently, the nearest thing to a JOIN permission is SEARCH, since you need that to find the keyring by name in the first place; beyond that, there is no permission needed to join a keyring.
I can make JOIN enable SEARCH, but if you pass it back to KEYCTL_SETPERM, you're going to enable SEARCH and JOIN, even if you didn't have SEARCH before.
There isn't an easy answer.
Quoted 3 lines
Again, ignoring the "non-special" ACEs will cause the returned permissions mask to be an under-estimate rather than an over-estimate. I'm not sure that's a good idea.
There isn't really a viable alternative. perm isn't what you, the caller, have available to you.
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+void key_put_acl(struct key_acl *acl) +{ + if (refcount_dec_and_test(&acl->usage)) + call_rcu(&acl->rcu, key_acl_rcu); +}Use
kfree_rcu().
Good idea. I was thinking that could only deal with objects in which the rcu_head was first, but that doesn't appear to be the case.
Quoted 3 lines
Bug: this will break from the loop if it encounters an ACE for uid or gid 4 (the value of
KEY_ACE_POSSESSOR). It needs to check forKEY_ACE_SPECIALbeforespecial_id=KEY_ACE_POSSESSORcan be considered meaningful.
Fixed.
Quoted 7 lines
It's not obvious what 'tp' means. How about:
static struct key_acl thread_keyring_acl = { ... };#define
process_keyring_aclthread_keyring_acl
I dislike that. I've changed it to:
static struct key_acl thread_and_process_keyring_acl = ...
Quoted 3 lines
Why give JOIN permission to anonymous session keyrings? They shouldn't be joinable --- and they weren't prior to this patch, since they were *not* given
KEY_USR_SEARCHpermission.
Point. Fixed.
Quoted 3 lines
This is also a behavior change which needs to be explained and justified on its own. A named keyring created with
keyctl_join_session_keyring(name)was not previously joinable by default, but after this patch it is.
Yeah - this should be its own patch. If you create a named keyring with KEYCTL_JOIN_SESSION_KEYRING, doing this again in another process with the same name ought to join the first one if it's owned by you and lets you find it.
Quoted 3 lines
find_keyring_by_name()also checks for SEARCH permission. Now this checks for JOIN as well. Is it intentional that two different permissions are being checked?
As previously mentioned, just JOIN should probably be used. In effect, SEARCH is also being split from JOIN.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (perm & KEY_NEED_INVAL) + oldstyle_perm |= KEY_NEED_SEARCH;Isn't this going to need to be SETATTR rather than SEARCH?
No. Current code requires SEARCH (or CAP_SYS_ADMIN + KEY_FLAG_ROOT_CAN_INVAL), not SETATTR. It was *your* proposal to make it use SETATTR instead.
Quoted 4 lines
Even if we can't update SELinux to be aware of the new-style permissions we still need to fix the "search also means delete" bug. Otherwise it will remain impossible to use SELinux to enforce a read-only view of a key hierarchy.
SELinux will need fixing if you want to be able to do that. I need to discuss how to do this with the SELinux people.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (perm & KEY_NEED_REVOKE) + oldstyle_perm |= KEY_NEED_WRITE;Another breaking change which needs to be explained and justified. Before either the write or setattr SELinux key permissions was sufficient for revocation, but now only write is.
Fixed.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (perm & KEY_NEED_SETSEC) + oldstyle_perm |= 0x20;Magic number.
The symbol no longer exists. I've #defined OLD_KEY_NEED_SETATTR to it in hooks.c.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (perm & KEY_NEED_JOIN) + oldstyle_perm |= KEY_NEED_LINK;The old-style permission for joining keyrings was SEARCH, not LINK. Probably it *should* have been LINK, but it's still a breaking change which needs to be justified on its own...
Changed to SEARCH.
Quoted 1 line
Why does Smack ignore requests for SEARCH permission?
No idea.
David
---
security/keys/compat.c +6 -0
@@ -141,6 +141,12 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,return keyctl_restrict_keyring (arg2,compat_ptr (arg3),compat_ptr (arg4)); +case KEYCTL_GET_ACL : +return keyctl_get_acl (arg2,compat_ptr (arg3), arg4); + +case KEYCTL_SET_ACL : +return keyctl_set_acl (arg2,compat_ptr (arg3), arg4); +default :return -EOPNOTSUPP ; }
security/keys/internal.h +8 -0
@@ -302,6 +302,14 @@ static inline long compat_keyctl_dh_compute(#endif #endif +extern long keyctl_get_acl (key_serial_t keyid, +struct user_key_ace __user *_acl, +size_t nr_elem); +extern long keyctl_set_acl (key_serial_t keyid, +const struct user_key_ace __user *_acl, +size_t nr_elem); + +/* * Debugging key validation */
security/keys/keyctl.c +12 -0
@@ -961,6 +961,8 @@ long keyctl_setperm_key(key_serial_t id, unsigned int perm) ace->mask |=KEY_ACE_REVOKE ;if (subset &KEY_OTH_SETATTR ) ace->mask |=KEY_ACE_SET_SECURITY ; +if (subset &KEY_OTH_SEARCH ) + ace->mask |=KEY_ACE_INVAL ;if (key->type = &key_type_keyring) { ace->mask |=KEY_ACE_JOIN ;if (subset &KEY_OTH_WRITE )
@@ -1768,6 +1770,16 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3, (const char __user *) arg3, (const char __user *) arg4); +case KEYCTL_GET_ACL : +return keyctl_get_acl ((key_serial_t )arg2, + (struct user_key_ace __user *)arg3, + (size_t )arg4); + +case KEYCTL_SET_ACL : +return keyctl_set_acl ((key_serial_t )arg2, + (const struct user_key_ace __user *)arg3, + (size_t )arg4); +default :return -EOPNOTSUPP ; }
security/keys/permission.c +182 -0
191 lines not on this page · raw message
David Howells
maintainerRFC PATCH 1/1 keyutils: Add key/keyring ACL support
In reply to David HowellsHere's a patch to make keyutils support ACLs on keys.
[root@andromeda ~]# LD_LIBRARY_PATH=~/keyutils ~/keyutils/keyctl setacl $k all:all u.4:r pos:0x37f root:I net:c [root@andromeda ~]# LD_LIBRARY_PATH=~/keyutils ~/keyutils/keyctl getacl $k
IDENTITY ID PERMS
===== ===== =========all - 0fffffff cjSRIlswrv
uid 4 00000002 --------r-
possessor - 0000037f cj-RIlswrv root - 00000020 ----I----- net - 00000200 c---------
David
---
keyctl.c +227 -1
@@ -71,6 +71,9 @@ static nr void act_keyctl_dh_compute(int argc, char *argv[]); static nr void act_keyctl_dh_compute_kdf(int argc, char *argv[]); static nr void act_keyctl_dh_compute_kdf_oi(int argc, char *argv[]); static nr void act_keyctl_restrict_keyring(int argc, char *argv[]); +static nr void act_keyctl_getacl(int argc, char *argv[]); +static nr void act_keyctl_rgetacl(int argc, char *argv[]); +static nr void act_keyctl_setacl(int argc, char *argv[]);const struct command commands[] = { { act_keyctl___version,"--version" ,"" },
@@ -82,9 +85,10 @@ const struct command commands[] = { { act_keyctl_dh_compute, "dh_compute", "<private> <prime> <base>" }, { act_keyctl_dh_compute_kdf, "dh_compute_kdf", "<private> <prime> <base> <len> <hash_name>" }, { act_keyctl_dh_compute_kdf_oi, "dh_compute_kdf_oi", "<private> <prime> <base> <len> <hash_name>" }, + { act_keyctl_getacl, "getacl", "<key>" }, + { act_keyctl_get_persistent, "get_persistent", "<keyring> [<uid>]" }, { act_keyctl_instantiate, "instantiate","<key> <data> <keyring>" }, { act_keyctl_invalidate,"invalidate", "<key>" }, - { act_keyctl_get_persistent, "get_persistent", "<keyring> [<uid>]" }, { act_keyctl_link, "link", "<key> <keyring>" }, { act_keyctl_list, "list", "<keyring>" }, { act_keyctl_negate, "negate", "<key> <timeout> <keyring>" },
@@ -107,12 +111,14 @@ const struct command commands[] = { { act_keyctl_request2, "request2", "<type> <desc> <info> [<dest_keyring>]" }, { act_keyctl_restrict_keyring, "restrict_keyring", "<keyring> [<type> [<restriction>]]" }, { act_keyctl_revoke, "revoke", "<key>" }, + { act_keyctl_rgetacl, "rgetacl", "<key>" }, { act_keyctl_rlist, "rlist", "<keyring>" }, { act_keyctl_search, "search", "<keyring> <type> <desc> [<dest_keyring>]" }, { act_keyctl_security, "security", "<key>" }, { act_keyctl_session, "session", "" }, { NULL, "session", "- [<prog> <arg1> <arg2> ...]" }, { NULL, "session", "<name> [<prog> <arg1> <arg2> ...]" }, + { act_keyctl_setacl, "setacl", "<key> [<ace> <ace> ...]" }, { act_keyctl_setperm, "setperm", "<key> <mask>" }, { act_keyctl_show, "show", "[-x] [<keyring>]" }, { act_keyctl_timeout, "timeout", "<key> <timeout>" },
@@ -1841,6 +1847,226 @@ static void act_keyctl_restrict_keyring(int argc, char *argv[])exit (0 ); } +static const char *special_ace_ids[] = { + [0 ] ="?0" , + [KEY_ACE_EVERYONE ] ="all" , + [KEY_ACE_GROUP ] ="group" , + [KEY_ACE_OWNER ] ="owner" , + [KEY_ACE_POSSESSOR ] ="possessor" , + [KEY_ACE_ROOT ] ="root" , + [KEY_ACE_SYS_ADMIN ] ="sys" , + [KEY_ACE_NET_ADMIN ] ="net" , +}; + +/* +* Display ACL. +*/ +static void display_acl (const struct key_ace *acl,int nr_ace) +{ +int i; + +printf ("IDENTITY ID PERMS\n" ); +printf ("===== ===== ==========\n" ); + +for (i =0 ; i < nr_ace; i++) { +const struct key_ace *ace = &acl[i]; + +switch (ace->mask &KEY_ACE__IDENTITY ) { +case KEY_ACE_SPECIAL : +if (ace->special_id <=KEY_ACE_NET_ADMIN ) +printf ("%-9.9s - " , special_ace_ids[ace->special_id]); +else +printf ("?special %10u" , ace->special_id); +break ; +case KEY_ACE_UID : +printf ("uid %10u" , ace->uid); +break ; +case KEY_ACE_GID : +printf ("gid %10u" , ace->gid); +break ; +default : +printf ("?%x %10u" , + (ace->mask &KEY_ACE__IDENTITY ) >>28 , + ace->special_id); +break ; + } + +printf (" %08x " , ace->mask &KEY_ACE__PERMS ); +printf ("%c%c%c%c%c%c%c%c%c%c\n" , + ace->mask &KEY_ACE_CLEAR ?'c' :'-' , + ace->mask &KEY_ACE_JOIN ?'j' :'-' , + ace->mask &KEY_ACE_SET_SECURITY ?'S' :'-' , + ace->mask &KEY_ACE_REVOKE ?'R' :'-' , + ace->mask &KEY_ACE_INVAL ?'I' :'-' , + ace->mask &KEY_ACE_LINK ?'l' :'-' , + ace->mask &KEY_ACE_SEARCH ?'s' :'-' , + ace->mask &KEY_ACE_WRITE ?'w' :'-' , + ace->mask &KEY_ACE_READ ?'r' :'-' , + ace->mask &KEY_ACE_VIEW ?'v' :'-' ); + } +} + +/* +* Get a key's ACL. +*/ +static void act_keyctl_getacl (int argc,char *argv[]) +{ +struct key_ace *acl; +key_serial_t key; +int ret, nr_ace; + +if (argc !=2 ) +format (); + + key =get_key_id (argv[1 ]); + + ret =keyctl_get_acl_alloc (key, &acl); +if (ret <0 ) +error ("keyctl_get_acl_alloc" ); + nr_ace = ret /sizeof (*acl); + +display_acl (acl, nr_ace); +exit (0 ); +} + +/* +* Get a key's ACL in raw hex form. +*/ +static void act_keyctl_rgetacl (int argc,char *argv[]) +{ +struct key_ace *acl; +key_serial_t key; +int ret, nr_ace, i; + +if (argc !=2 ) +format (); + + key =get_key_id (argv[1 ]); + + ret =keyctl_get_acl_alloc (key, &acl); +if (ret <0 ) +error ("keyctl_get_acl_alloc" ); + nr_ace = ret /sizeof (*acl); + +for (i =0 ; i < nr_ace; i++) { +if (i !=0 ) +putchar (';' ); +printf ("%x:%x" , acl[i].mask, acl[i].special_id); + } + +putchar ('\n' ); +exit (0 ); +} + +/* +* Set a key's ACL. +*/ +static void act_keyctl_setacl (int argc,char *argv[]) +{ +struct key_ace *acl =NULL , *ace; +key_serial_t key; +unsigned ltmp; +unsigned tmp, len; +char *id, *colon, *perm, *p; +int nr_ace, i; + +if (argc <2 ) +format (); + + key =get_key_id (argv[1 ]); + nr_ace = argc -2 ; + +if (nr_ace >0 ) { + acl =alloca (sizeof (*acl) * nr_ace); +memset (acl,0 ,sizeof (*acl) * nr_ace); + +for (i =0 ; i < nr_ace; i++) { + ace = &acl[i]; + id = argv[i +2 ]; + colon =strchr (id,':' ); +if (!colon || colon = id) + goto invalid_ace; + + *colon =0 ; +if (strcmp (id,"all" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_EVERYONE ; + }else if (strcmp (id,"grp" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_GROUP ; + }else if (strcmp (id,"own" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_OWNER ; + }else if (strcmp (id,"pos" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_POSSESSOR ; + }else if (strcmp (id,"root" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_ROOT ; + }else if (strcmp (id,"sys" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_SYS_ADMIN ; + }else if (strcmp (id,"net" ) =0 ) { + ace->mask =KEY_ACE_SPECIAL ; + ace->special_id =KEY_ACE_NET_ADMIN ; + }else if (strncmp (id,"u." ,2 ) =0 ) { + ace->mask =KEY_ACE_UID ; +if (sscanf (id +2 ,"%u%n" , &tmp, &len) !=1 ) + goto invalid_ace_colon; +if (id +2 + len != colon) + goto invalid_ace_colon; + ace->uid = tmp; + }else if (strncmp (id,"g." ,2 ) =0 ) { + ace->mask =KEY_ACE_GID ; +if (sscanf (id +2 ,"%u%n" , &tmp, &len) !=1 ) + goto invalid_ace_colon; +if (id +2 + len != colon) + goto invalid_ace_colon; + ace->gid = tmp; + }else { + goto invalid_ace_colon; + } + + perm = colon +1 ; +if (isdigit (*perm)) { + ltmp =strtoul (perm, &p,0 ); +if (ltmp & ~(unsigned long )KEY_ACE__PERMS ) + goto invalid_ace_colon; + ace->mask |= ltmp; + }else if (strcmp (perm,"all" ) =0 ) { + ace->mask |=KEY_ACE__PERMS ; + }else { +for (; *perm; perm++) { +switch (*perm) { +case 'v' : ace->mask |=KEY_ACE_VIEW ;break ; +case 'r' : ace->mask |=KEY_ACE_READ ;break ; +case 'w' : ace->mask |=KEY_ACE_WRITE ;break ; +case 's' : ace->mask |=KEY_ACE_SEARCH ;break ; +case 'l' : ace->mask |=KEY_ACE_LINK ;break ; +case 'I' : ace->mask |=KEY_ACE_INVAL ;break ; +case 'R' : ace->mask |=KEY_ACE_REVOKE ;break ; +case 'S' : ace->mask |=KEY_ACE_SET_SECURITY ;break ; +case 'j' : ace->mask |=KEY_ACE_JOIN ;break ; +case 'c' : ace->mask |=KEY_ACE_CLEAR ;break ; +default : goto invalid_ace_colon; + } + } + } + } + } + +if (keyctl_set_acl (key, acl, nr_ace *sizeof (*acl)) <0 ) +error ("keyctl_set_acl" ); + +exit (0 ); + +invalid_ace_colon: + *colon =':' ; +invalid_ace: +fprintf (stderr,"Invalid ACE: '%s'\n" , id); +exit (2 ); +} +/*****************************************************************************/ /* * parse a key identifier
keyutils.c +45 -0
57 lines not on this page · raw message
keyutils.h +41 -0
65 lines not on this page · raw message
tests/keyctl/dh_compute/bad-args/runtest.sh +1 -1
@@ -72,7 +72,7 @@ expect_keyid logonid marker "CHECK WRONG KEY TYPE" dh_compute --fail $privateid $primeid $logonid -expect_error ENOKEY +expect_error EOPNOTSUPP dh_compute --fail $privateid $primeid @s expect_error EOPNOTSUPP
tests/keyctl/permitting/valid/runtest.sh +4 -5
@@ -72,12 +72,11 @@ set_key_perm $keyid 0x00201f00 describe_key --fail $keyid expect_error EACCES -# check that we can't use other perms instead of user perms to view the key -# (our UID matches that of the key) -marker "VIEW OTHER PERMISSIONS" +# check that permissions for everyone allow the user to view the key, +# even if our user perms don't. +marker "VIEW EVERYONE PERMISSIONS" set_key_perm $keyid 0x0020001f -describe_key --fail $keyid -expect_error EACCES +describe_key $keyid # check that taking away setattr permission renders the key immune to setperm marker "REMOVE SETATTR"