* SELinux issue with 'keys-acl' patch in kernel.org's 'linux-next' tree @ 2020-01-23 10:28 Richard Haines 2020-01-23 15:12 ` SELinux: How to split permissions for keys? David Howells 0 siblings, 1 reply; 8+ messages in thread From: Richard Haines @ 2020-01-23 10:28 UTC (permalink / raw) To: dhowells; +Cc: keyrings, selinux I see the 'keys-acl' [1] patch is now back in kernel.org's 'linux-next' tree. After some investigation, I have a query on this that I've attempted to explain below. The keyutils tests work with this patch on standard Fedora policy because the policy gives most domains access to all key permissions. Also the polcy is built using 'hide_broken_symptoms' that adds 'allow domain domain:key { link search };', therefore calls made by these always pass: keys/keyring.c - find_keyring_by_name(): Originally required: KEY_NEED_SEARCH Now requires: KEY_NEED_JOIN keys/keyctl.c - keyctl_session_to_parent(): Originally required: KEY_NEED_LINK Now requires: KEY_NEED_JOIN However if (as in the selinux-testsuite - test/keys), 'domain' is replaced by a macro that excludes the { link search }, and allows each permission to be tested, two tests fail. This is because: 1) keyctl_session_to_parent() requires KEY_NEED_JOIN translated to KEY_NEED_LINK permission. 2) find_keyring_by_name() requires KEY_NEED_JOIN translated to KEY_NEED_SEARCH permission. The patch always translates KEY_NEED_JOIN to KEY_NEED_SEARCH Any views on this as it seems a regression. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/security/selinux?h=next-20200122&id=a862a799537490b74a81e14a62623af502bdb25d ^ permalink raw reply [flat|nested] 8+ messages in thread
* SELinux: How to split permissions for keys? 2020-01-23 10:28 SELinux issue with 'keys-acl' patch in kernel.org's 'linux-next' tree Richard Haines @ 2020-01-23 15:12 ` David Howells 2020-01-23 15:46 ` Stephen Smalley 0 siblings, 1 reply; 8+ messages in thread From: David Howells @ 2020-01-23 15:12 UTC (permalink / raw) To: Stephen Smalley Cc: dhowells, Richard Haines, keyrings, selinux, linux-security-module, linux-kernel 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? 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; + sid = cred_sid(cred); key = key_ref_to_ptr(key_ref); ksec = key->security; return avc_has_perm(&selinux_state, - sid, ksec->sid, SECCLASS_KEY, perm, NULL); + sid, ksec->sid, SECCLASS_KEY, oldstyle_perm, NULL); } static int selinux_key_getsecurity(struct key *key, char **_buffer) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-01-23 15:12 ` SELinux: How to split permissions for keys? David Howells @ 2020-01-23 15:46 ` Stephen Smalley 2020-01-23 20:35 ` Stephen Smalley 0 siblings, 1 reply; 8+ messages in thread From: Stephen Smalley @ 2020-01-23 15:46 UTC (permalink / raw) To: David Howells Cc: Richard Haines, keyrings, selinux, linux-security-module, linux-kernel 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@google.com/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@tycho.nsa.gov/ 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. > sid = cred_sid(cred); > > key = key_ref_to_ptr(key_ref); > ksec = key->security; > > return avc_has_perm(&selinux_state, > - sid, ksec->sid, SECCLASS_KEY, perm, NULL); > + sid, ksec->sid, SECCLASS_KEY, oldstyle_perm, NULL); > } > > static int selinux_key_getsecurity(struct key *key, char **_buffer) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-01-23 15:46 ` Stephen Smalley @ 2020-01-23 20:35 ` Stephen Smalley 2020-02-02 19:30 ` Richard Haines 0 siblings, 1 reply; 8+ messages in thread From: Stephen Smalley @ 2020-01-23 20:35 UTC (permalink / raw) To: David Howells Cc: Richard Haines, keyrings, selinux, linux-security-module, linux-kernel 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@google.com/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@tycho.nsa.gov/ > > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-01-23 20:35 ` Stephen Smalley @ 2020-02-02 19:30 ` Richard Haines 2020-02-03 13:13 ` Stephen Smalley 0 siblings, 1 reply; 8+ messages in thread From: Richard Haines @ 2020-02-02 19:30 UTC (permalink / raw) To: Stephen Smalley, David Howells, paul Cc: keyrings, selinux, linux-security-module, linux-kernel 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@google.com/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@tycho.nsa.gov/ > > > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-02-02 19:30 ` Richard Haines @ 2020-02-03 13:13 ` Stephen Smalley 2020-02-03 14:03 ` Richard Haines 0 siblings, 1 reply; 8+ messages in thread From: Stephen Smalley @ 2020-02-03 13:13 UTC (permalink / raw) To: Richard Haines, David Howells, paul Cc: keyrings, selinux, linux-security-module, linux-kernel On 2/2/20 2:30 PM, Richard Haines wrote: > 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@google.com/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@tycho.nsa.gov/ >>> >>> 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 Was that kernel patch ever posted to selinux list and/or the selinux kernel maintainers? I don't recall seeing it. If not, please send it to the selinux list for review; at least one selinux maintainer should ack it before it gets accepted into any other tree. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-02-03 13:13 ` Stephen Smalley @ 2020-02-03 14:03 ` Richard Haines 2020-02-03 14:48 ` Stephen Smalley 0 siblings, 1 reply; 8+ messages in thread From: Richard Haines @ 2020-02-03 14:03 UTC (permalink / raw) To: Stephen Smalley, David Howells, paul Cc: keyrings, selinux, linux-security-module, linux-kernel On Mon, 2020-02-03 at 08:13 -0500, Stephen Smalley wrote: > On 2/2/20 2:30 PM, Richard Haines wrote: > > 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@google.com/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@tycho.nsa.gov/ > > > > > > > > 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 > > Was that kernel patch ever posted to selinux list and/or the selinux > kernel maintainers? I don't recall seeing it. If not, please send > it > to the selinux list for review; at least one selinux maintainer > should > ack it before it gets accepted into any other tree. > > Not formally. I did post it in a discussion about keys in [2]. Since then it's been modified to support the split permissions. I've extracted the patch from [1] and will post that to list for comments. [2] https://lore.kernel.org/selinux/35455b30b5185780628e92c98ec8191c70f39bde.camel@btinternet.com/ > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SELinux: How to split permissions for keys? 2020-02-03 14:03 ` Richard Haines @ 2020-02-03 14:48 ` Stephen Smalley 0 siblings, 0 replies; 8+ messages in thread From: Stephen Smalley @ 2020-02-03 14:48 UTC (permalink / raw) To: Richard Haines, David Howells, paul Cc: keyrings, selinux, linux-security-module, linux-kernel On 2/3/20 9:03 AM, Richard Haines wrote: > On Mon, 2020-02-03 at 08:13 -0500, Stephen Smalley wrote: >> Was that kernel patch ever posted to selinux list and/or the selinux >> kernel maintainers? I don't recall seeing it. If not, please send >> it >> to the selinux list for review; at least one selinux maintainer >> should >> ack it before it gets accepted into any other tree. >> >> > > Not formally. I did post it in a discussion about keys in [2]. Since > then it's been modified to support the split permissions. Yes, that doesn't count since a) it wasn't the final version of the patch which changed significantly afterward and b) even it had been the final version, there was no acked-by or reviewed-by from a selinux maintainer, just some suggestions. A non-trivial patch that modifies security/selinux needs to be at least acked by a selinux maintainer and often should go through the upstream selinux maintainer (Paul). > I've extracted the patch from [1] and will post that to list for > comments. Thanks. > [2] > https://lore.kernel.org/selinux/35455b30b5185780628e92c98ec8191c70f39bde.camel@btinternet.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-03 14:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-23 10:28 SELinux issue with 'keys-acl' patch in kernel.org's 'linux-next' tree Richard Haines 2020-01-23 15:12 ` SELinux: How to split permissions for keys? David Howells 2020-01-23 15:46 ` Stephen Smalley 2020-01-23 20:35 ` Stephen Smalley 2020-02-02 19:30 ` Richard Haines 2020-02-03 13:13 ` Stephen Smalley 2020-02-03 14:03 ` Richard Haines 2020-02-03 14:48 ` Stephen Smalley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).