On Thu, 2020-01-23 at 15:35 -0500, Stephen Smalley wrote: > On 1/23/20 10:46 AM, Stephen Smalley wrote: > > On 1/23/20 10:12 AM, David Howells wrote: > > > Hi Stephen, > > > > > > I have patches to split the permissions that are used for keys to > > > make > > > them a > > > bit finer grained and easier to use - and also to move to ACLs > > > rather > > > than > > > fixed masks. See patch "keys: Replace uid/gid/perm permissions > > > checking with > > > an ACL" here: > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-acl > > > > > > > > > > > > However, I may not have managed the permission mask > > > transformation inside > > > SELinux correctly. Could you lend an eyeball? The change to > > > the > > > permissions > > > model is as follows: > > > > > > The SETATTR permission is split to create two new > > > permissions: > > > (1) SET_SECURITY - which allows the key's owner, group and > > > ACL > > > to be > > > changed and a restriction to be placed on a keyring. > > > (2) REVOKE - which allows a key to be revoked. > > > The SEARCH permission is split to create: > > > (1) SEARCH - which allows a keyring to be search and a key > > > to be > > > found. > > > (2) JOIN - which allows a keyring to be joined as a > > > session > > > keyring. > > > (3) INVAL - which allows a key to be invalidated. > > > The WRITE permission is also split to create: > > > (1) WRITE - which allows a key's content to be altered and > > > links > > > to be > > > added, removed and replaced in a keyring. > > > (2) CLEAR - which allows a keyring to be cleared > > > completely. > > > This is > > > split out to make it possible to give just this to an > > > administrator. > > > (3) REVOKE - see above. > > > > > > The change to SELinux is attached below. > > > > > > Should the split be pushed down into the SELinux policy rather > > > than > > > trying to > > > calculate it? > > > > My understanding is that you must provide full backward > > compatibility > > with existing policies; hence, you must ensure that you always > > check the > > same SELinux permission(s) for the same operation when using an > > existing > > policy. > > > > In order to support finer-grained distinctions in SELinux with > > future > > policies, you can define a new SELinux policy capability along with > > the > > new permissions, and if the policy capability is enabled in the > > policy, > > check the new permissions rather than the old ones. A recent > > example of > > adding a new policy capability and using it can be seen in: > > https://lore.kernel.org/selinux/20200116194530.8696-1-jeffv@xxxxxxxxxx/T/#u > > although that patch was rejected for other reasons. > > > > Another example was when we introduced fine-grained distinctions > > for all > > network address families, commit > > da69a5306ab92e07224da54aafee8b1dccf024f6. > > > > The new policy capability also needs to be defined in libsepol for > > use > > by the policy compiler; an example can be seen in: > > https://lore.kernel.org/selinux/20170714164801.6346-1-sds@xxxxxxxxxxxxx/ > > > > Then future policies can declare the policy capability when they > > are > > ready to start using the new permissions instead of the old. > > > > > Thanks, > > > David > > > --- > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 116b4d644f68..c8db5235b01f 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -6556,6 +6556,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 > > > @@ -6564,13 +6565,26 @@ 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_SETSEC) > > > + oldstyle_perm |= OLD_KEY_NEED_SETATTR; > > > + if (perm & KEY_NEED_INVAL) > > > + oldstyle_perm |= KEY_NEED_SEARCH; > > > + if (perm & KEY_NEED_REVOKE && !(perm & > > > OLD_KEY_NEED_SETATTR)) > > > + oldstyle_perm |= KEY_NEED_WRITE; > > > + if (perm & KEY_NEED_JOIN) > > > + oldstyle_perm |= KEY_NEED_SEARCH; > > > + if (perm & KEY_NEED_CLEAR) > > > + oldstyle_perm |= KEY_NEED_WRITE; > > > + > > > > I don't know offhand if this ensures that the same SELinux > > permission is > > always checked as it would have been previously for the same > > operation+arguments. That's what you have to preserve for > > existing > > policies. > > As Richard pointed out in his email, your key-acl series replaces > two > different old permissions (LINK, SEARCH) with a single permission > (JOIN) > in different callers, so by the time we reach the SELinux hook we > cannot > map it back unambiguously and provide full backward > compatibility. The > REVOKE case also seems fragile although there you seem to distinguish > by > sometimes passing in OLD_KEY_NEED_SETATTR and sometimes not? You'll > have to fix the JOIN case to avoid userspace breakage. > > You may want to go ahead and explicitly translate all of the > KEY_NEED > permissions to SELinux permissions rather than passing the key > permissions directly here to avoid requiring that the values always > match. The SELinux permission symbols are of the form > CLASS__PERMISSION > (NB double underscore), e.g. KEY__SETATTR, generated automatically > from > the security/selinux/include/classmap.h tables to the > security/selinux/av_permissions.h generated header. Most hooks > perform > such translation, e.g. file_mask_to_av(). You will almost certainly > need to do this if/when you introduce support for the new permissions > to > SELinux. This problem has now been fixed in [1]. It passes the current selinux-test-suite (except test/fs_filesystem regression). As the fix now includes a new 'key_perms' policy capability to allow use of the extended key permissions, I've updated libsepol and the selinux-testsuite test/keys to test these. I'll submit two RFC patches that will allow [1] to be tested with 'key_perms' true or false. [1] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next
