* security/selinux: Add support for new key permissions
@ 2020-02-03 14:13 Richard Haines
2020-02-03 15:29 ` Stephen Smalley
0 siblings, 1 reply; 5+ messages in thread
From: Richard Haines @ 2020-02-03 14:13 UTC (permalink / raw)
To: selinux; +Cc: sds, paul, dhowells, Richard Haines
Add a new 'key_perms' policy capability and support for the additional
key permissions: inval, revoke, join, clear
Also fixes JOIN -> LINK permission translation when policy
capability 'keys_perms' = 0;
The current "setattr" perm name remains and is used for KEY_NEED_SETSEC.
This gives the following permissions for the 'key' class:
create Create a key or keyring.
view View attributes.
read Read contents.
write Update or modify.
search Search (keyring) or find (key).
link Link a key into the keyring.
setattr kernel < 5.X Change permissions on a keyring.
kernel >= 5.X Set owner, group, ACL.
inval Invalidate key.
revoke Revoke key.
join Join keyring as session.
clear Clear a keyring.
Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 3 +-
security/keys/keyctl.c | 3 +-
security/selinux/hooks.c | 73 ++++++++++++++++++++++++++++++-------
security/selinux/include/classmap.h | 2 +-
security/selinux/include/security.h | 8 ++++
security/selinux/ss/services.c | 3 +-
6 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 24c69457783f..ddfc0709569b 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -435,7 +435,8 @@ extern void key_free_user_ns(struct user_namespace *);
#define KEY_NEED_REVOKE 0x080 /* Require permission to revoke key */
#define KEY_NEED_JOIN 0x100 /* Require permission to join keyring as session */
#define KEY_NEED_CLEAR 0x200 /* Require permission to clear a keyring */
-#define KEY_NEED_ALL 0x3ff
+#define KEY_NEED_PARENT_JOIN 0x400 /* Require permission to impose keyring on parent */
+#define KEY_NEED_ALL 0x7ff
#define OLD_KEY_NEED_SETATTR 0x20 /* Used to be Require permission to change attributes */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index a0c97d4d8251..65e1c0c3feb1 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
struct cred *cred;
int ret;
- keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_JOIN);
+ keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
+ KEY_NEED_PARENT_JOIN);
if (IS_ERR(keyring_r))
return PTR_ERR(keyring_r);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c8db5235b01f..a499bd7d9777 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6556,7 +6556,8 @@ static int selinux_key_permission(key_ref_t key_ref,
{
struct key *key;
struct key_security_struct *ksec;
- unsigned oldstyle_perm;
+ unsigned int key_perm = 0, switch_perm = 0;
+ int bit = 1, count = KEY_NEED_ALL;
u32 sid;
/* if no specific permissions are requested, we skip the
@@ -6565,18 +6566,62 @@ 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;
+ if (selinux_policycap_key_perms()) {
+ while (count) {
+ switch_perm = bit & perm;
+ switch (switch_perm) {
+ case KEY_NEED_VIEW:
+ key_perm |= KEY__VIEW;
+ break;
+ case KEY_NEED_READ:
+ key_perm |= KEY__READ;
+ break;
+ case KEY_NEED_WRITE:
+ key_perm |= KEY__WRITE;
+ break;
+ case KEY_NEED_SEARCH:
+ key_perm |= KEY__SEARCH;
+ break;
+ case KEY_NEED_LINK:
+ key_perm |= KEY__LINK;
+ break;
+ case KEY_NEED_SETSEC:
+ key_perm |= KEY__SETATTR;
+ break;
+ case KEY_NEED_INVAL:
+ key_perm |= KEY__INVAL;
+ break;
+ case KEY_NEED_REVOKE:
+ key_perm |= KEY__REVOKE;
+ break;
+ case KEY_NEED_JOIN:
+ case KEY_NEED_PARENT_JOIN:
+ key_perm |= KEY__JOIN;
+ break;
+ case KEY_NEED_CLEAR:
+ key_perm |= KEY__CLEAR;
+ break;
+ }
+ bit <<= 1;
+ count >>= 1;
+ }
+ } else {
+ key_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
+ KEY_NEED_WRITE | KEY_NEED_SEARCH |
+ KEY_NEED_LINK);
+ if (perm & KEY_NEED_PARENT_JOIN)
+ key_perm |= KEY_NEED_LINK;
+ if (perm & KEY_NEED_SETSEC)
+ key_perm |= OLD_KEY_NEED_SETATTR;
+ if (perm & KEY_NEED_INVAL)
+ key_perm |= KEY_NEED_SEARCH;
+ if (perm & KEY_NEED_REVOKE && !(perm & OLD_KEY_NEED_SETATTR))
+ key_perm |= KEY_NEED_WRITE;
+ if (perm & KEY_NEED_JOIN)
+ key_perm |= KEY_NEED_SEARCH;
+ if (perm & KEY_NEED_CLEAR)
+ key_perm |= KEY_NEED_WRITE;
+ }
sid = cred_sid(cred);
@@ -6584,7 +6629,7 @@ static int selinux_key_permission(key_ref_t key_ref,
ksec = key->security;
return avc_has_perm(&selinux_state,
- sid, ksec->sid, SECCLASS_KEY, oldstyle_perm, NULL);
+ sid, ksec->sid, SECCLASS_KEY, key_perm, NULL);
}
static int selinux_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 7db24855e12d..038374bef526 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -159,7 +159,7 @@ struct security_class_mapping secclass_map[] = {
{ "send", "recv", "relabelto", "forward_in", "forward_out", NULL } },
{ "key",
{ "view", "read", "write", "search", "link", "setattr", "create",
- NULL } },
+ "inval", "revoke", "join", "clear", NULL } },
{ "dccp_socket",
{ COMMON_SOCK_PERMS,
"node_bind", "name_connect", NULL } },
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index ae840634e3c7..6b264b6d9d31 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -79,6 +79,7 @@ enum {
POLICYDB_CAPABILITY_ALWAYSNETWORK,
POLICYDB_CAPABILITY_CGROUPSECLABEL,
POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
+ POLICYDB_CAPABILITY_KEYPERMS,
__POLICYDB_CAPABILITY_MAX
};
#define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
@@ -178,6 +179,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void)
return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION];
}
+static inline bool selinux_policycap_key_perms(void)
+{
+ struct selinux_state *state = &selinux_state;
+
+ return state->policycap[POLICYDB_CAPABILITY_KEYPERMS];
+}
+
int security_mls_enabled(struct selinux_state *state);
int security_load_policy(struct selinux_state *state,
void *data, size_t len);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index a5813c7629c1..b31dcd54cde9 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -73,7 +73,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
"extended_socket_class",
"always_check_network",
"cgroup_seclabel",
- "nnp_nosuid_transition"
+ "nnp_nosuid_transition",
+ "key_perms"
};
static struct selinux_ss selinux_ss;
--
cgit 1.2-0.3.lf.el7
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: security/selinux: Add support for new key permissions
2020-02-03 14:13 security/selinux: Add support for new key permissions Richard Haines
@ 2020-02-03 15:29 ` Stephen Smalley
2020-02-03 15:42 ` Richard Haines
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Smalley @ 2020-02-03 15:29 UTC (permalink / raw)
To: Richard Haines, selinux; +Cc: paul, dhowells
On 2/3/20 9:13 AM, Richard Haines wrote:
> Add a new 'key_perms' policy capability and support for the additional
> key permissions: inval, revoke, join, clear
For future reference, subject line should have [PATCH] prefix; git
send-email will do this for you automatically. Also, the subsystem
prefix would conventionally be "keys,selinux:" to indicates that it
touches the keys and selinux subsystems, no need for "security/".
>
> Also fixes JOIN -> LINK permission translation when policy
> capability 'keys_perms' = 0;
>
> The current "setattr" perm name remains and is used for KEY_NEED_SETSEC.
> This gives the following permissions for the 'key' class:
>
> create Create a key or keyring.
> view View attributes.
> read Read contents.
> write Update or modify.
> search Search (keyring) or find (key).
> link Link a key into the keyring.
> setattr kernel < 5.X Change permissions on a keyring.
> kernel >= 5.X Set owner, group, ACL.
> inval Invalidate key.
> revoke Revoke key.
> join Join keyring as session.
> clear Clear a keyring.
>
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> include/linux/key.h | 3 +-
> security/keys/keyctl.c | 3 +-
> security/selinux/hooks.c | 73 ++++++++++++++++++++++++++++++-------
> security/selinux/include/classmap.h | 2 +-
> security/selinux/include/security.h | 8 ++++
> security/selinux/ss/services.c | 3 +-
> 6 files changed, 74 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 24c69457783f..ddfc0709569b 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -435,7 +435,8 @@ extern void key_free_user_ns(struct user_namespace *);
> #define KEY_NEED_REVOKE 0x080 /* Require permission to revoke key */
> #define KEY_NEED_JOIN 0x100 /* Require permission to join keyring as session */
> #define KEY_NEED_CLEAR 0x200 /* Require permission to clear a keyring */
> -#define KEY_NEED_ALL 0x3ff
> +#define KEY_NEED_PARENT_JOIN 0x400 /* Require permission to impose keyring on parent */
> +#define KEY_NEED_ALL 0x7ff
>
> #define OLD_KEY_NEED_SETATTR 0x20 /* Used to be Require permission to change attributes */
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index a0c97d4d8251..65e1c0c3feb1 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
> struct cred *cred;
> int ret;
>
> - keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_JOIN);
> + keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
> + KEY_NEED_PARENT_JOIN);
I'm unclear on how this works with the regular key permission checking
(not SELinux). There is no KEY_ACE_PARENT_JOIN permission AFAICT and
the check would fail if the regular permissions were only KEY_ACE_JOIN.
What we need is an additional flag that will get ignored by
key_permission() for its permission checking but signify to SELinux that
different handling is required here.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c8db5235b01f..a499bd7d9777 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6565,18 +6566,62 @@ 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;
> + if (selinux_policycap_key_perms()) {
> + while (count) {
> + switch_perm = bit & perm;
> + switch (switch_perm) {
> + case KEY_NEED_VIEW:
> + key_perm |= KEY__VIEW;
> + break;
> + case KEY_NEED_READ:
> + key_perm |= KEY__READ;
> + break;
> + case KEY_NEED_WRITE:
> + key_perm |= KEY__WRITE;
> + break;
> + case KEY_NEED_SEARCH:
> + key_perm |= KEY__SEARCH;
> + break;
> + case KEY_NEED_LINK:
> + key_perm |= KEY__LINK;
> + break;
> + case KEY_NEED_SETSEC:
> + key_perm |= KEY__SETATTR;
> + break;
> + case KEY_NEED_INVAL:
> + key_perm |= KEY__INVAL;
> + break;
> + case KEY_NEED_REVOKE:
> + key_perm |= KEY__REVOKE;
> + break;
> + case KEY_NEED_JOIN:
> + case KEY_NEED_PARENT_JOIN:
> + key_perm |= KEY__JOIN;
> + break;
> + case KEY_NEED_CLEAR:
> + key_perm |= KEY__CLEAR;
> + break;
> + }
> + bit <<= 1;
> + count >>= 1;
> + }
So I assume then that it is unreasonable to have key_permission() limit
its inputs to single-permission-at-a-time checks and therefore we have
to permit multiple permissions? David? If not, is there any concern
about the performance overhead of this loop and if so should we optimize
for the common case of a single permission?
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ae840634e3c7..6b264b6d9d31 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -79,6 +79,7 @@ enum {
> POLICYDB_CAPABILITY_ALWAYSNETWORK,
> POLICYDB_CAPABILITY_CGROUPSECLABEL,
> POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
> + POLICYDB_CAPABILITY_KEYPERMS,
> __POLICYDB_CAPABILITY_MAX
> };
> #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> @@ -178,6 +179,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void)
This will collide with
https://lore.kernel.org/selinux/20200128191656.111902-1-cgzones@googlemail.com/
so we'll have to sort out which one goes first.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: security/selinux: Add support for new key permissions
2020-02-03 15:29 ` Stephen Smalley
@ 2020-02-03 15:42 ` Richard Haines
2020-02-06 16:03 ` Stephen Smalley
0 siblings, 1 reply; 5+ messages in thread
From: Richard Haines @ 2020-02-03 15:42 UTC (permalink / raw)
To: Stephen Smalley, selinux, dhowells; +Cc: paul
On Mon, 2020-02-03 at 10:29 -0500, Stephen Smalley wrote:
> On 2/3/20 9:13 AM, Richard Haines wrote:
> > Add a new 'key_perms' policy capability and support for the
> > additional
> > key permissions: inval, revoke, join, clear
>
> For future reference, subject line should have [PATCH] prefix; git
> send-email will do this for you automatically. Also, the subsystem
> prefix would conventionally be "keys,selinux:" to indicates that it
> touches the keys and selinux subsystems, no need for "security/".
>
> > Also fixes JOIN -> LINK permission translation when policy
> > capability 'keys_perms' = 0;
> >
> > The current "setattr" perm name remains and is used for
> > KEY_NEED_SETSEC.
> > This gives the following permissions for the 'key' class:
> >
> > create Create a key or keyring.
> > view View attributes.
> > read Read contents.
> > write Update or modify.
> > search Search (keyring) or find (key).
> > link Link a key into the keyring.
> > setattr kernel < 5.X Change permissions on a keyring.
> > kernel >= 5.X Set owner, group, ACL.
> > inval Invalidate key.
> > revoke Revoke key.
> > join Join keyring as session.
> > clear Clear a keyring.
> >
> > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > ---
> > include/linux/key.h | 3 +-
> > security/keys/keyctl.c | 3 +-
> > security/selinux/hooks.c | 73
> > ++++++++++++++++++++++++++++++-------
> > security/selinux/include/classmap.h | 2 +-
> > security/selinux/include/security.h | 8 ++++
> > security/selinux/ss/services.c | 3 +-
> > 6 files changed, 74 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/key.h b/include/linux/key.h
> > index 24c69457783f..ddfc0709569b 100644
> > --- a/include/linux/key.h
> > +++ b/include/linux/key.h
> > @@ -435,7 +435,8 @@ extern void key_free_user_ns(struct
> > user_namespace *);
> > #define KEY_NEED_REVOKE 0x080 /* Require permission to
> > revoke key */
> > #define KEY_NEED_JOIN 0x100 /* Require permission to
> > join keyring as session */
> > #define KEY_NEED_CLEAR 0x200 /* Require permission to
> > clear a keyring */
> > -#define KEY_NEED_ALL 0x3ff
> > +#define KEY_NEED_PARENT_JOIN 0x400 /* Require permission to impose
> > keyring on parent */
> > +#define KEY_NEED_ALL 0x7ff
> >
> > #define OLD_KEY_NEED_SETATTR 0x20 /* Used to be Require
> > permission to change attributes */
> >
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index a0c97d4d8251..65e1c0c3feb1 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
> > struct cred *cred;
> > int ret;
> >
> > - keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
> > KEY_NEED_JOIN);
> > + keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
> > + KEY_NEED_PARENT_JOIN);
>
> I'm unclear on how this works with the regular key permission
> checking
> (not SELinux). There is no KEY_ACE_PARENT_JOIN permission AFAICT
> and
> the check would fail if the regular permissions were only
> KEY_ACE_JOIN.
> What we need is an additional flag that will get ignored by
> key_permission() for its permission checking but signify to SELinux
> that
> different handling is required here.
David will need to answer this.
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index c8db5235b01f..a499bd7d9777 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6565,18 +6566,62 @@ 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;
> > + if (selinux_policycap_key_perms()) {
> > + while (count) {
> > + switch_perm = bit & perm;
> > + switch (switch_perm) {
> > + case KEY_NEED_VIEW:
> > + key_perm |= KEY__VIEW;
> > + break;
> > + case KEY_NEED_READ:
> > + key_perm |= KEY__READ;
> > + break;
> > + case KEY_NEED_WRITE:
> > + key_perm |= KEY__WRITE;
> > + break;
> > + case KEY_NEED_SEARCH:
> > + key_perm |= KEY__SEARCH;
> > + break;
> > + case KEY_NEED_LINK:
> > + key_perm |= KEY__LINK;
> > + break;
> > + case KEY_NEED_SETSEC:
> > + key_perm |= KEY__SETATTR;
> > + break;
> > + case KEY_NEED_INVAL:
> > + key_perm |= KEY__INVAL;
> > + break;
> > + case KEY_NEED_REVOKE:
> > + key_perm |= KEY__REVOKE;
> > + break;
> > + case KEY_NEED_JOIN:
> > + case KEY_NEED_PARENT_JOIN:
> > + key_perm |= KEY__JOIN;
> > + break;
> > + case KEY_NEED_CLEAR:
> > + key_perm |= KEY__CLEAR;
> > + break;
> > + }
> > + bit <<= 1;
> > + count >>= 1;
> > + }
>
> So I assume then that it is unreasonable to have key_permission()
> limit
> its inputs to single-permission-at-a-time checks and therefore we
> have
> to permit multiple permissions? David? If not, is there any concern
> about the performance overhead of this loop and if so should we
> optimize
> for the common case of a single permission?
I have a single perm patch as well if that is required
>
> > diff --git a/security/selinux/include/security.h
> > b/security/selinux/include/security.h
> > index ae840634e3c7..6b264b6d9d31 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -79,6 +79,7 @@ enum {
> > POLICYDB_CAPABILITY_ALWAYSNETWORK,
> > POLICYDB_CAPABILITY_CGROUPSECLABEL,
> > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
> > + POLICYDB_CAPABILITY_KEYPERMS,
> > __POLICYDB_CAPABILITY_MAX
> > };
> > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> > @@ -178,6 +179,13 @@ static inline bool
> > selinux_policycap_nnp_nosuid_transition(void)
>
> This will collide with
> https://lore.kernel.org/selinux/20200128191656.111902-1-cgzones@googlemail.com/
> so we'll have to sort out which one goes first.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: security/selinux: Add support for new key permissions
2020-02-03 15:42 ` Richard Haines
@ 2020-02-06 16:03 ` Stephen Smalley
2020-02-06 18:07 ` Richard Haines
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Smalley @ 2020-02-06 16:03 UTC (permalink / raw)
To: Richard Haines, selinux, dhowells; +Cc: paul
On 2/3/20 10:42 AM, Richard Haines wrote:
> On Mon, 2020-02-03 at 10:29 -0500, Stephen Smalley wrote:
>> On 2/3/20 9:13 AM, Richard Haines wrote:
>>> Add a new 'key_perms' policy capability and support for the
>>> additional
>>> key permissions: inval, revoke, join, clear
>>
>> For future reference, subject line should have [PATCH] prefix; git
>> send-email will do this for you automatically. Also, the subsystem
>> prefix would conventionally be "keys,selinux:" to indicates that it
>> touches the keys and selinux subsystems, no need for "security/".
>>
>>> Also fixes JOIN -> LINK permission translation when policy
>>> capability 'keys_perms' = 0;
>>>
>>> The current "setattr" perm name remains and is used for
>>> KEY_NEED_SETSEC.
>>> This gives the following permissions for the 'key' class:
>>>
>>> create Create a key or keyring.
>>> view View attributes.
>>> read Read contents.
>>> write Update or modify.
>>> search Search (keyring) or find (key).
>>> link Link a key into the keyring.
>>> setattr kernel < 5.X Change permissions on a keyring.
>>> kernel >= 5.X Set owner, group, ACL.
>>> inval Invalidate key.
>>> revoke Revoke key.
>>> join Join keyring as session.
>>> clear Clear a keyring.
>>>
>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> ---
>>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>>> index a0c97d4d8251..65e1c0c3feb1 100644
>>> --- a/security/keys/keyctl.c
>>> +++ b/security/keys/keyctl.c
>>> @@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
>>> struct cred *cred;
>>> int ret;
>>>
>>> - keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
>>> KEY_NEED_JOIN);
>>> + keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
>>> + KEY_NEED_PARENT_JOIN);
>>
>> I'm unclear on how this works with the regular key permission
>> checking
>> (not SELinux). There is no KEY_ACE_PARENT_JOIN permission AFAICT
>> and
>> the check would fail if the regular permissions were only
>> KEY_ACE_JOIN.
>> What we need is an additional flag that will get ignored by
>> key_permission() for its permission checking but signify to SELinux
>> that
>> different handling is required here.
>
> David will need to answer this.
Until this gets resolved, this patch must not go into mainline.
Otherwise we're looking at a potential userspace ABI issue when/if it
gets resolved.
>>> diff --git a/security/selinux/include/security.h
>>> b/security/selinux/include/security.h
>>> index ae840634e3c7..6b264b6d9d31 100644
>>> --- a/security/selinux/include/security.h
>>> +++ b/security/selinux/include/security.h
>>> @@ -79,6 +79,7 @@ enum {
>>> POLICYDB_CAPABILITY_ALWAYSNETWORK,
>>> POLICYDB_CAPABILITY_CGROUPSECLABEL,
>>> POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
>>> + POLICYDB_CAPABILITY_KEYPERMS,
>>> __POLICYDB_CAPABILITY_MAX
>>> };
>>> #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
>>> @@ -178,6 +179,13 @@ static inline bool
>>> selinux_policycap_nnp_nosuid_transition(void)
>>
>> This will collide with
>> https://lore.kernel.org/selinux/20200128191656.111902-1-cgzones@googlemail.com/
>> so we'll have to sort out which one goes first.
Already noted this elsewhere but for the sake of ensuring it is noted in
this thread too: the key_perms capability will need to be added after
the genfs_seclabel_symlinks capability since the latter is already
queued on selinux/next and the corresponding libsepol patch has been
merged upstream.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: security/selinux: Add support for new key permissions
2020-02-06 16:03 ` Stephen Smalley
@ 2020-02-06 18:07 ` Richard Haines
0 siblings, 0 replies; 5+ messages in thread
From: Richard Haines @ 2020-02-06 18:07 UTC (permalink / raw)
To: Stephen Smalley, selinux, dhowells; +Cc: paul
On Thu, 2020-02-06 at 11:03 -0500, Stephen Smalley wrote:
> On 2/3/20 10:42 AM, Richard Haines wrote:
> > On Mon, 2020-02-03 at 10:29 -0500, Stephen Smalley wrote:
> > > On 2/3/20 9:13 AM, Richard Haines wrote:
> > > > Add a new 'key_perms' policy capability and support for the
> > > > additional
> > > > key permissions: inval, revoke, join, clear
> > >
> > > For future reference, subject line should have [PATCH] prefix;
> > > git
> > > send-email will do this for you automatically. Also, the
> > > subsystem
> > > prefix would conventionally be "keys,selinux:" to indicates that
> > > it
> > > touches the keys and selinux subsystems, no need for "security/".
> > >
> > > > Also fixes JOIN -> LINK permission translation when policy
> > > > capability 'keys_perms' = 0;
> > > >
> > > > The current "setattr" perm name remains and is used for
> > > > KEY_NEED_SETSEC.
> > > > This gives the following permissions for the 'key' class:
> > > >
> > > > create Create a key or keyring.
> > > > view View attributes.
> > > > read Read contents.
> > > > write Update or modify.
> > > > search Search (keyring) or find (key).
> > > > link Link a key into the keyring.
> > > > setattr kernel < 5.X Change permissions on a keyring.
> > > > kernel >= 5.X Set owner, group, ACL.
> > > > inval Invalidate key.
> > > > revoke Revoke key.
> > > > join Join keyring as session.
> > > > clear Clear a keyring.
> > > >
> > > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > > ---
> > > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > > > index a0c97d4d8251..65e1c0c3feb1 100644
> > > > --- a/security/keys/keyctl.c
> > > > +++ b/security/keys/keyctl.c
> > > > @@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
> > > > struct cred *cred;
> > > > int ret;
> > > >
> > > > - keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING,
> > > > 0,
> > > > KEY_NEED_JOIN);
> > > > + keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING,
> > > > 0,
> > > > + KEY_NEED_PARENT_JOIN);
> > >
> > > I'm unclear on how this works with the regular key permission
> > > checking
> > > (not SELinux). There is no KEY_ACE_PARENT_JOIN permission AFAICT
> > > and
> > > the check would fail if the regular permissions were only
> > > KEY_ACE_JOIN.
> > > What we need is an additional flag that will get ignored by
> > > key_permission() for its permission checking but signify to
> > > SELinux
> > > that
> > > different handling is required here.
> >
> > David will need to answer this.
>
> Until this gets resolved, this patch must not go into mainline.
> Otherwise we're looking at a potential userspace ABI issue when/if
> it
> gets resolved.
>
> > > > diff --git a/security/selinux/include/security.h
> > > > b/security/selinux/include/security.h
> > > > index ae840634e3c7..6b264b6d9d31 100644
> > > > --- a/security/selinux/include/security.h
> > > > +++ b/security/selinux/include/security.h
> > > > @@ -79,6 +79,7 @@ enum {
> > > > POLICYDB_CAPABILITY_ALWAYSNETWORK,
> > > > POLICYDB_CAPABILITY_CGROUPSECLABEL,
> > > > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
> > > > + POLICYDB_CAPABILITY_KEYPERMS,
> > > > __POLICYDB_CAPABILITY_MAX
> > > > };
> > > > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX -
> > > > 1)
> > > > @@ -178,6 +179,13 @@ static inline bool
> > > > selinux_policycap_nnp_nosuid_transition(void)
> > >
> > > This will collide with
> > > https://lore.kernel.org/selinux/20200128191656.111902-1-cgzones@googlemail.com/
> > > so we'll have to sort out which one goes first.
>
> Already noted this elsewhere but for the sake of ensuring it is noted
> in
> this thread too: the key_perms capability will need to be added
> after
> the genfs_seclabel_symlinks capability since the latter is already
> queued on selinux/next and the corresponding libsepol patch has been
> merged upstream.
I'm still waiting for David to get back regarding the permission loop.
Anyway I'll update the patch as soon as I see
POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS in Paul's
https://github.com/SELinuxProject/selinux-kernel/tree/next
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-06 18:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-03 14:13 security/selinux: Add support for new key permissions Richard Haines
2020-02-03 15:29 ` Stephen Smalley
2020-02-03 15:42 ` Richard Haines
2020-02-06 16:03 ` Stephen Smalley
2020-02-06 18:07 ` Richard Haines
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.