On 1/23/20 10:46 AM, Stephen Smalley wrote:

On 1/23/20 10:12 AM, David Howells wrote:

quoted

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@google.com/T/#u (local)
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@tycho.nsa.gov/ (local)

Then future policies can declare the policy capability when they are 
ready to start using the new permissions instead of the old.

quoted

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.