selinux: add transitions for kernel keys

Unfortunately, experience has shown that transitions are necessary for keys stored in the kernel's keyring; inheriting labels from the creating process has shown to result in keys with unpredictable labels as the different kernel keyring users race to create new keys. While setkeycreatecon() does exist to allow applications to set the security label of newly created keys, it is impractical to modify all keyring users to use the SELinux API. As with several other kernel resources, a policy driven transition is the most practical solution.

This patch adds the necessary transition call and protects it via the newly created "keytrans" policy capability. The policy capability is necessary to ensure compatibility with existing policies as the default transition behavior is to inherit the target's type information which is not the current, or desired, behavior. For similar reasons, the uninitialized behavior of security_compute_sid() has been updated such that key objects inherit the subject's label.

As one might expect, this patch does require an updated userspace to create a policy to use this new functionality. Userspace changes would require the addition of the new "keytrans" policy capability and most likely a change to the way type transitions are applied to the key object class, see below:

default_type key source;

... this would ensure that newly created keys would inherit the type information of the calling process in case a type transition was not explicitly defined in policy. This leads to the SELinux type transition statement for the key object class, see below:

type_transition <domain> kernel_t:key <new_key_type>

... in the case of the key object class, the target type is always the kernel's security label, which in the case of the Reference Policy is "kernel_t".

Signed-off-by XXX

---
 security/selinux/hooks.c            |   21 ++++++++++++++-------
 security/selinux/include/security.h |    2 ++
 security/selinux/ss/services.c      |    4 ++++
 3 files changed, 20 insertions(+), 7 deletions(-)

security/selinux/hooks.c +0 -1

index 57b0b49..3ea05b6 100644
@@ -5682,18 +5682,25 @@ static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 static int selinux_key_alloc(struct key *k, const struct cred *cred,
 			     unsigned long flags)
 {
-	const struct task_security_struct *tsec;

security/selinux/hooks.c +5 -6

+		if (rc)
+			return rc;
+	} else
+		sid = tsec->sid;
 
 	ksec = kzalloc(sizeof(struct key_security_struct), GFP_KERNEL);
 	if (!ksec)
 		return -ENOMEM;
-
-	tsec = cred->security;
-	if (tsec->keycreate_sid)
-		ksec->sid = tsec->keycreate_sid;
-	else
-		ksec->sid = tsec->sid;
+	ksec->sid = sid;
 
 	k->security = ksec;
 	return 0;

security/selinux/include/security.h +2 -0

index fe341ae..8460217 100644
@@ -71,6 +71,7 @@ enum {
 	POLICYDB_CAPABILITY_OPENPERM,
 	POLICYDB_CAPABILITY_REDHAT1,
 	POLICYDB_CAPABILITY_ALWAYSNETWORK,
+	POLICYDB_CAPABILITY_KEYTRANS,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)

@@ -78,6 +79,7 @@ enum {
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
 extern int selinux_policycap_alwaysnetwork;
+extern int selinux_policycap_keytrans;
 
 /*
  * type_datum properties

security/selinux/ss/services.c +4 -0

index d106733..4f57007 100644
@@ -73,6 +73,7 @@
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
 int selinux_policycap_alwaysnetwork;
+int selinux_policycap_keytrans;
 
 static DEFINE_RWLOCK(policy_rwlock);
 

@@ -1404,6 +1405,7 @@ static int security_compute_sid(u32 ssid,
 
 	if (!ss_initialized) {
 		switch (orig_tclass) {
+		case SECCLASS_KEY:
 		case SECCLASS_PROCESS: /* kernel value */
 			*out_sid = ssid;
 			break;

@@ -1815,6 +1817,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_OPENPERM);
 	selinux_policycap_alwaysnetwork = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_ALWAYSNETWORK);
+	selinux_policycap_keytrans = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_KEYTRANS);
 }
 
 static int security_preserve_bools(struct policydb *p);

_______________________________________________
Selinux mailing list
[email protected]
To unsubscribe, send email to [email protected].
To get help, send an email containing "help" to [email protected].

Quoted 2 lines · Paul Moore

Unfortunately, experience has shown that transitions are necessary for keys stored in the kernel's keyring ...

Quoted 15 lines

security/selinux/hooks.c +2 -1

index 57b0b49..3ea05b6 100644
@@ -5682,18 +5682,25 @@ static int selinux_inode_getsecctx(struct inode
 *inode, void **ctx, u32 *ctxlen)> 
 static int selinux_key_alloc(struct key *k, const struct cred *cred,
 
 			     unsigned long flags)
 
 {
 
-	const struct task_security_struct *tsec;
+	int rc;
+	const struct task_security_struct *tsec = current_security();

security/selinux/hooks.c static int selinux_inode_getsecctx(struct inode · line 5700

Quoted 10 lines

security/selinux/hooks.c +7 -0

 
 	struct key_security_struct *ksec;
 
+	u32 sid;
+
+	if (tsec->keycreate_sid)
+		sid = tsec->keycreate_sid;
+	else if (selinux_policycap_keytrans) {
+		rc = security_transition_sid(tsec->sid, SECINITSID_KERNEL,
+					     SECCLASS_KEY, NULL, &sid);

Wouldn't it be more general and help avoid the need for a separate policy capability and avoid special casing keys, to pass both the cred->security->sid and the current->security->sid to security_transition_sid()?

Then you just arrange the sid argument order so that the current default behavior will be in effect in the absence of any type_transition or default_type rules?

Also, isn't the use of current_security() here rather than the provided cred a divergence from the keyring model for DAC, and what implications exist there? Why isn't this a problem for their DAC model if it is a problem for SELinux?

Quoted 3 lines · Stephen Smalley

Quoted 2 lines · Paul Moore

Unfortunately, experience has shown that transitions are necessary for keys stored in the kernel's keyring ...

...

Quoted 31 lines

Quoted 15 lines

security/selinux/hooks.c +2 -1

index 57b0b49..3ea05b6 100644
@@ -5682,18 +5682,25 @@ static int selinux_inode_getsecctx(struct inode
 *inode, void **ctx, u32 *ctxlen)> 
 static int selinux_key_alloc(struct key *k, const struct cred *cred,
 
 			     unsigned long flags)
 
 {
 
-	const struct task_security_struct *tsec;
+	int rc;
+	const struct task_security_struct *tsec = current_security();

security/selinux/hooks.c static int selinux_inode_getsecctx(struct inode · line 5700

Quoted 10 lines

security/selinux/hooks.c +7 -0

 
 	struct key_security_struct *ksec;
 
+	u32 sid;
+
+	if (tsec->keycreate_sid)
+		sid = tsec->keycreate_sid;
+	else if (selinux_policycap_keytrans) {
+		rc = security_transition_sid(tsec->sid, SECINITSID_KERNEL,
+					     SECCLASS_KEY, NULL, &sid);

Wouldn't it be more general and help avoid the need for a separate policy capability and avoid special casing keys, to pass both the cred->security->sid and the current->security->sid to security_transition_sid()?

Quoted 8 lines

Then you just arrange the sid argument order so that the current default behavior will be in effect in the absence of any type_transition or default_type rules?

Also, isn't the use of current_security() here rather than the provided cred a divergence from the keyring model for DAC, and what implications exist there? Why isn't this a problem for their DAC model if it is a problem for SELinux?

My mistake, I agree that we should use cred->security instead of current_security(), thanks for catching that.

However, I'm not sure passing both cred->sid and current->sid to the transition call is the correct solution, regardless of which is acting as the source/target (although both could be kernel_t in certain cases). In my mind the target is either the kernel, or ideally, the linked keyring; unfortunately we don't have a keyring in this scope and in some cases we may never have a keyring. Perhaps you can elaborate a bit ... ?

I guess I don't understand the original problem or fix then. I assumed that cred->security was referring to one set of credentials while current_security() was referring to another and you wanted to use the latter instead of the former. If not, then none of this makes any sense. Transitions are for computing new labels from a pair of labels, either by allowing selection of either subject or object or by deriving a new label from the two. If there is only one label in view and the other is always fixed (kernel), then under what conditions are you going to label some keys with the kernel domain and why does that make any sense at all? It was still allocated by some userspace process and ought to carry along the credentials of that process.

The SELinux key/keyring model was designed quite a while ago and vetted by the keys/keyrings maintainer, so I'd be careful about any changes without careful consideration and review by the keys maintainer (David Howells). It is also documented in Documentation/security/keys.txt.

Quoted 11 lines · Stephen Smalley

I guess I don't understand the original problem or fix then. I assumed that cred->security was referring to one set of credentials while current_security() was referring to another and you wanted to use the latter instead of the former. If not, then none of this makes any sense. Transitions are for computing new labels from a pair of labels, either by allowing selection of either subject or object or by deriving a new label from the two. If there is only one label in view and the other is always fixed (kernel), then under what conditions are you going to label some keys with the kernel domain and why does that make any sense at all? It was still allocated by some userspace process and ought to carry along the credentials of that process.

If we ignore setkeycreatecon() for a moment, keys are always created with the label of the process that creates them, this is normal and expected. The issue arises when several different processes, each with a different label, need to update/modify the same key; there is a race-like condition where the security label for a key could change depending on which process "won" and created the key. This unpredictability is causing the policy writers a headache as they try to craft policy that isn't overly permissive when it comes to the keyring.

One example taken from a problem report:

"Right now keys created in the kernel are done so with the label
of the creating process. That means that if sssd adds a key to be used by the user all user types must be able to read sssd_t keys. If they want to use kinit to update the key, now all user types must be able to write to sssd_t keys."

Quoted 4 lines

The SELinux key/keyring model was designed quite a while ago and vetted by the keys/keyrings maintainer, so I'd be careful about any changes without careful consideration and review by the keys maintainer (David Howells). It is also documented in Documentation/security/keys.txt.

You are right, I should have CC'd David on the posting, I'll do that for v2.

As for the documentation, I'll update it in the v2 patch to mention the proposed transition mechanism. As it stands right now I don't see a major issue with this proposal so long as we ensure that the default behavior matches what is seen on systems today.

Quoted 36 lines · Paul Moore

Quoted 11 lines · Stephen Smalley

I guess I don't understand the original problem or fix then. I assumed that cred->security was referring to one set of credentials while current_security() was referring to another and you wanted to use the latter instead of the former. If not, then none of this makes any sense. Transitions are for computing new labels from a pair of labels, either by allowing selection of either subject or object or by deriving a new label from the two. If there is only one label in view and the other is always fixed (kernel), then under what conditions are you going to label some keys with the kernel domain and why does that make any sense at all? It was still allocated by some userspace process and ought to carry along the credentials of that process.

If we ignore setkeycreatecon() for a moment, keys are always created with the label of the process that creates them, this is normal and expected. The issue arises when several different processes, each with a different label, need to update/modify the same key; there is a race-like condition where the security label for a key could change depending on which process "won" and created the key. This unpredictability is causing the policy writers a headache as they try to craft policy that isn't overly permissive when it comes to the keyring.

One example taken from a problem report:

"Right now keys created in the kernel are done so with the label of the creating process. That means that if sssd adds a key to be used by the user all user types must be able to read sssd_t keys. If they want to use kinit to update the key, now all user types must be able to write to sssd_t keys."

Quoted 5 lines

The SELinux key/keyring model was designed quite a while ago and vetted by the keys/keyrings maintainer, so I'd be careful about any changes without careful consideration and review by the keys maintainer (David Howells). It is also documented in Documentation/security/keys.txt.

Quoted 7 lines

You are right, I should have CC'd David on the posting, I'll do that for v2.

As for the documentation, I'll update it in the v2 patch to mention the proposed transition mechanism. As it stands right now I don't see a major issue with this proposal so long as we ensure that the default behavior matches what is seen on systems today.

No, we need to understand the model you are proposing first before we discuss the code.

Currently, we label keys based on creator unless using setkeycreatecon(), same as sockets. With sockets though there is a special case for kernel-internal sockets where those are labeled with the kernel domain and exempted, but those are never exposed directly to userspace.

How do you envision labeling keys? Will some keys still be labeled with the creator's domain, and others with the kernel domain? Which ones get labeled which way? And if you have to allow everyone to write to the kernel domain labeled keys, then how are you helping prevent the very problem you described, i.e. you are still introducing a globally-writable type in the policy (unconstrained sharing, potential for misuse of a key intended for another).

_______________________________________________ Selinux mailing list [email protected] To unsubscribe, send email to [email protected]. To get help, send an email containing "help" to [email protected].

Quoted 53 lines · Stephen Smalley

Quoted 36 lines · Paul Moore

Quoted 11 lines · Stephen Smalley

I guess I don't understand the original problem or fix then. I assumed that cred->security was referring to one set of credentials while current_security() was referring to another and you wanted to use the latter instead of the former. If not, then none of this makes any sense. Transitions are for computing new labels from a pair of labels, either by allowing selection of either subject or object or by deriving a new label from the two. If there is only one label in view and the other is always fixed (kernel), then under what conditions are you going to label some keys with the kernel domain and why does that make any sense at all? It was still allocated by some userspace process and ought to carry along the credentials of that process.

If we ignore setkeycreatecon() for a moment, keys are always created with the label of the process that creates them, this is normal and expected. The issue arises when several different processes, each with a different label, need to update/modify the same key; there is a race-like condition where the security label for a key could change depending on which process "won" and created the key. This unpredictability is causing the policy writers a headache as they try to craft policy that isn't overly permissive when it comes to the keyring.

One example taken from a problem report:

"Right now keys created in the kernel are done so with the label of the creating process. That means that if sssd adds a key to be used by the user all user types must be able to read sssd_t keys. If they want to use kinit to update the key, now all user types must be able to write to sssd_t keys."

Quoted 5 lines

The SELinux key/keyring model was designed quite a while ago and vetted by the keys/keyrings maintainer, so I'd be careful about any changes without careful consideration and review by the keys maintainer (David Howells). It is also documented in Documentation/security/keys.txt.

Quoted 7 lines

You are right, I should have CC'd David on the posting, I'll do that for v2.

As for the documentation, I'll update it in the v2 patch to mention the proposed transition mechanism. As it stands right now I don't see a major issue with this proposal so long as we ensure that the default behavior matches what is seen on systems today.

No, we need to understand the model you are proposing first before we discuss the code.

Currently, we label keys based on creator unless using setkeycreatecon(), same as sockets. With sockets though there is a special case for kernel-internal sockets where those are labeled with the kernel domain and exempted, but those are never exposed directly to userspace.

Quoted 7 lines

How do you envision labeling keys? Will some keys still be labeled with the creator's domain, and others with the kernel domain? Which ones get labeled which way? And if you have to allow everyone to write to the kernel domain labeled keys, then how are you helping prevent the very problem you described, i.e. you are still introducing a globally-writable type in the policy (unconstrained sharing, potential for misuse of a key intended for another).

I believe the problem is there is no way for sssd to know which label to assign to the keyring at the time of creation. I have suggested that they wait to create the keyring until after pam_selinux has been called so they could then create the keyring with the setkeycreatecon(getexeccon()). Not sure what a transition rule would buy you?

Quoted 37 lines · Paul Moore

Quoted 11 lines · Stephen Smalley

I guess I don't understand the original problem or fix then. I assumed that cred->security was referring to one set of credentials while current_security() was referring to another and you wanted to use the latter instead of the former. If not, then none of this makes any sense. Transitions are for computing new labels from a pair of labels, either by allowing selection of either subject or object or by deriving a new label from the two. If there is only one label in view and the other is always fixed (kernel), then under what conditions are you going to label some keys with the kernel domain and why does that make any sense at all? It was still allocated by some userspace process and ought to carry along the credentials of that process.

If we ignore setkeycreatecon() for a moment, keys are always created with the label of the process that creates them, this is normal and expected. The issue arises when several different processes, each with a different label, need to update/modify the same key; there is a race-like condition where the security label for a key could change depending on which process "won" and created the key. This unpredictability is causing the policy writers a headache as they try to craft policy that isn't overly permissive when it comes to the keyring.

One example taken from a problem report:
"Right now keys created in the kernel are done so with the label
of the creating process. That means that if sssd adds a key to be used by the user all user types must be able to read sssd_t keys. If they want to use kinit to update the key, now all user types must be able to write to sssd_t keys."

Quoted 5 lines

The SELinux key/keyring model was designed quite a while ago and vetted by the keys/keyrings maintainer, so I'd be careful about any changes without careful consideration and review by the keys maintainer (David Howells). It is also documented in Documentation/security/keys.txt.

You are right, I should have CC'd David on the posting, I'll do that for v2.

Quoted 4 lines

As for the documentation, I'll update it in the v2 patch to mention the proposed transition mechanism. As it stands right now I don't see a major issue with this proposal so long as we ensure that the default behavior matches what is seen on systems today.

No, we need to understand the model you are proposing first before we discuss the code.

Currently, we label keys based on creator unless using setkeycreatecon(), same as sockets. With sockets though there is a special case for kernel-internal sockets where those are labeled with the kernel domain and exempted, but those are never exposed directly to userspace.

How do you envision labeling keys?

Will some keys still be labeled with the creator's domain, and others with the kernel domain?

Which ones get labeled which way? And if you have to allow everyone to write to the kernel domain labeled keys, then how are you helping prevent the very problem you described, i.e. you are still introducing a globally- writable type in the policy (unconstrained sharing, potential for misuse of a key intended for another).