[RFC PATCH v1] selinux: add transitions for kernel keys

[RFC PATCH v1] selinux: add transitions for kernel keys

[Paul Moore]

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(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 57b0b49..3ea05b6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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();
 	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);
+		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;
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index fe341ae..8460217 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -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
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index d106733..4f57007 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -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);

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Stephen Smalley]

On 03/12/2014 05:53 PM, Paul Moore wrote:
> 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(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 57b0b49..3ea05b6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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();
>  	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?

> +		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;
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index fe341ae..8460217 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -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
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index d106733..4f57007 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -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
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 
> 

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Paul Moore]

On Thursday, March 13, 2014 09:17:11 AM Stephen Smalley wrote:
> On 03/12/2014 05:53 PM, Paul Moore wrote:
> > Unfortunately, experience has shown that transitions are necessary for
> > keys stored in the kernel's keyring ...

...

> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 57b0b49..3ea05b6 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -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();
> > 
> >  	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?

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 ... ?

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Stephen Smalley]

On 03/13/2014 11:46 AM, Paul Moore wrote:
> On Thursday, March 13, 2014 09:17:11 AM Stephen Smalley wrote:
>> On 03/12/2014 05:53 PM, Paul Moore wrote:
>>> Unfortunately, experience has shown that transitions are necessary for
>>> keys stored in the kernel's keyring ...
> 
> ...
> 
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 57b0b49..3ea05b6 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -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();
>>>
>>>  	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?
> 
> 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.

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Paul Moore]

On Thursday, March 13, 2014 12:34:36 PM Stephen Smalley wrote:
> 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."

> 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.

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Stephen Smalley]

On 03/13/2014 07:00 PM, Paul Moore wrote:
> On Thursday, March 13, 2014 12:34:36 PM Stephen Smalley wrote:
>> 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."
> 
>> 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.

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).

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/14/2014 08:00 AM, Stephen Smalley wrote:
> On 03/13/2014 07:00 PM, Paul Moore wrote:
>> On Thursday, March 13, 2014 12:34:36 PM Stephen Smalley wrote:
>>> 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."
>> 
>>> 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.
> 
> 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 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 
> 
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?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMjCO8ACgkQrlYvE4MpobMF0QCgpcT53BKaAUt3S3y83AZGJTxw
UXoAoNAU2ZgPOLbq+3hrdpKx5SDcCRbp
=bh+0
-----END PGP SIGNATURE-----

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Paul Moore]

On Friday, March 14, 2014 09:49:35 AM Daniel J Walsh wrote:
> On 03/14/2014 08:00 AM, Stephen Smalley wrote:
> > On 03/13/2014 07:00 PM, Paul Moore wrote:
> >> On Thursday, March 13, 2014 12:34:36 PM Stephen Smalley wrote:
> >>> 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."
> >> 
> >>> 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.
> > 
> > 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).
>
> 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?

I guess I misunderstood what you wanted Dan, I thought you wanted a transition 
rule for keys; if that isn't the case we can drop this patch.

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH v1] selinux: add transitions for kernel keys

[Paul Moore]

On Friday, March 14, 2014 08:00:29 AM Stephen Smalley wrote:
> On 03/13/2014 07:00 PM, Paul Moore wrote:
> > On Thursday, March 13, 2014 12:34:36 PM Stephen Smalley wrote:
> >> 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."
> >> 
> >> 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.
> 
> No, we need to understand the model you are proposing first before we
> discuss the code.

[NOTE: I'm sending this out to wrap things up for the time being and so
 that we have something in the archives in case this comes up again, I'm not 
 sure it is worth the time/effort to discuss this further for reasons
 described below.]

Considering the confusion all around it seemed to me that the appropriate 
thing to do would have been to document the model in the keys.txt file along 
with the code changes (as I said above for the v2 RFC patch).

> 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?

This is sort of moot at this point as there have been some more offline 
discussions with the policy folks and the sssd folks (see the example above) 
that may remove the need for these transitions.  More work needs to be done 
with applications like sssd so I'm shelving this patch for the time being.  If 
the sssd changes don't end up being viable we'll revisit this idea.

However, the original idea was to preserve the behavior as it exist today 
expect for when a transition was specified in policy.  The idea was to allow 
things like sssd, and other login/key related processes, to create user keys 
that were not labeled as their own, e.g. sssd_t, but rather some separate user 
key type, e.g. user_key_t.  The reason for this was to ensure a consistently 
labeled key/keyring regardless of the login mechanism which would allow for a 
less restrictive policy.  As things stand currently the key related policy 
needs to be more-or-less wide open for things to work correctly.

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

As mentioned above, keys will be labeled with the creator's domain by default 
(excluding transitions and setkeycreatecon).  Keys would only be labeled with 
the kernel's type for the same reason they are today, creation before root has 
logged into the system.

> 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 think this is all explained above.

-- 
paul moore
security and virtualization @ redhat