[PATCH] selinux: fix the default socket labeling in sock_graft()

[PATCH] selinux: fix the default socket labeling in sock_graft()

[Paul Moore]

The sock_graft() hook has special handling for AF_INET, AF_INET, and
AF_UNIX sockets as those address families have special hooks which
label the sock before it is attached its associated socket.
Unfortunately, the sock_graft() hook was missing a default approach
to labeling sockets which meant that any other address family which
made use of connections or the accept() syscall would find the
returned socket to be in an "unlabeled" state.  This was recently
demonstrated by the kcrypto/AF_ALG subsystem and the newly released
cryptsetup package (cryptsetup v1.6.5 and later).

This patch preserves the special handling in selinux_sock_graft(),
but adds a default behavior - setting the sock's label equal to the
associated socket - which resolves the problem with AF_ALG and
presumably any other address family which makes use of accept().

Cc: stable@vger.kernel.org
Signed-off-by: Paul Moore <pmoore@redhat.com>
Tested-by: Milan Broz <gmazyland@gmail.com>
---
 security/selinux/hooks.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 336f0a0..39f16d0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
 	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
 	struct sk_security_struct *sksec = sk->sk_security;
 
-	if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
-	    sk->sk_family == PF_UNIX)
+	switch (sk->sk_family) {
+	case PF_INET:
+	case PF_INET6:
+	case PF_UNIX:
 		isec->sid = sksec->sid;
+		break;
+	default:
+		/* by default there is no special labeling mechanism for the
+		 * sock label so inherit the label from the parent socket */
+		sksec->sid = isec->sid;
+	}
 	sksec->sclass = isec->sclass;
 }
 

Re: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Stephen Smalley]

On 07/10/2014 11:37 AM, Paul Moore wrote:
> The sock_graft() hook has special handling for AF_INET, AF_INET, and
> AF_UNIX sockets as those address families have special hooks which
> label the sock before it is attached its associated socket.
> Unfortunately, the sock_graft() hook was missing a default approach
> to labeling sockets which meant that any other address family which
> made use of connections or the accept() syscall would find the
> returned socket to be in an "unlabeled" state.  This was recently
> demonstrated by the kcrypto/AF_ALG subsystem and the newly released
> cryptsetup package (cryptsetup v1.6.5 and later).
> 
> This patch preserves the special handling in selinux_sock_graft(),
> but adds a default behavior - setting the sock's label equal to the
> associated socket - which resolves the problem with AF_ALG and
> presumably any other address family which makes use of accept().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> Tested-by: Milan Broz <gmazyland@gmail.com>
> ---
>  security/selinux/hooks.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 336f0a0..39f16d0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
>  	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
>  	struct sk_security_struct *sksec = sk->sk_security;
>  
> -	if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
> -	    sk->sk_family == PF_UNIX)
> +	switch (sk->sk_family) {
> +	case PF_INET:
> +	case PF_INET6:
> +	case PF_UNIX:
>  		isec->sid = sksec->sid;
> +		break;
> +	default:
> +		/* by default there is no special labeling mechanism for the
> +		 * sock label so inherit the label from the parent socket */
> +		sksec->sid = isec->sid;
> +	}

Wait...why would we assign isec->sid from sksec->sid in the former case
but the reverse here?  Shouldn't we be setting isec->sid in all cases?
The hook documentation in include/linux/security.h unfortunately does
not describe the actual abstract behavior but rather describes the
implementation in the inet case.


>  	sksec->sclass = isec->sclass;
>  }
>  
> 
> _______________________________________________
> 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: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Stephen Smalley]

On 07/10/2014 12:56 PM, Stephen Smalley wrote:
> On 07/10/2014 11:37 AM, Paul Moore wrote:
>> The sock_graft() hook has special handling for AF_INET, AF_INET, and
>> AF_UNIX sockets as those address families have special hooks which
>> label the sock before it is attached its associated socket.
>> Unfortunately, the sock_graft() hook was missing a default approach
>> to labeling sockets which meant that any other address family which
>> made use of connections or the accept() syscall would find the
>> returned socket to be in an "unlabeled" state.  This was recently
>> demonstrated by the kcrypto/AF_ALG subsystem and the newly released
>> cryptsetup package (cryptsetup v1.6.5 and later).
>>
>> This patch preserves the special handling in selinux_sock_graft(),
>> but adds a default behavior - setting the sock's label equal to the
>> associated socket - which resolves the problem with AF_ALG and
>> presumably any other address family which makes use of accept().
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> Tested-by: Milan Broz <gmazyland@gmail.com>
>> ---
>>  security/selinux/hooks.c |   12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 336f0a0..39f16d0 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
>>  	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
>>  	struct sk_security_struct *sksec = sk->sk_security;
>>  
>> -	if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
>> -	    sk->sk_family == PF_UNIX)
>> +	switch (sk->sk_family) {
>> +	case PF_INET:
>> +	case PF_INET6:
>> +	case PF_UNIX:
>>  		isec->sid = sksec->sid;
>> +		break;
>> +	default:
>> +		/* by default there is no special labeling mechanism for the
>> +		 * sock label so inherit the label from the parent socket */
>> +		sksec->sid = isec->sid;
>> +	}
> 
> Wait...why would we assign isec->sid from sksec->sid in the former case
> but the reverse here?  Shouldn't we be setting isec->sid in all cases?
> The hook documentation in include/linux/security.h unfortunately does
> not describe the actual abstract behavior but rather describes the
> implementation in the inet case.

Ok, I think I understand this now:  for inet and unix, sksec->sid is set
in other hooks upon connection establishment based on the peer label -
primarily for multi-level servers - and we are propagating it upward to
the parent socket inode.  For others, sksec->sid is not set anywhere
except initialized to unlabeled upon sock creation and so you are just
pushing the parent socket inode label down to the sock in your patch.

It seems a bit fragile though and certainly doesn't align with the
sock_graft hook documentation anymore.  Wondering if we should assert
that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case
(i.e. that sksec->sid has been set prior to copying it to isec->sid) and
that sksec->sid is SECINITSID_UNLABELED (i.e. that it has not already
been set by the protocol implementation) in the default case.
We need to update the hook documentation too.

Re: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Paul Moore]

On Thursday, July 10, 2014 01:45:55 PM Stephen Smalley wrote:
> Ok, I think I understand this now:  for inet and unix, sksec->sid is set
> in other hooks upon connection establishment based on the peer label -
> primarily for multi-level servers - and we are propagating it upward to
> the parent socket inode.  For others, sksec->sid is not set anywhere
> except initialized to unlabeled upon sock creation and so you are just
> pushing the parent socket inode label down to the sock in your patch.

Yep.

> It seems a bit fragile though and certainly doesn't align with the
> sock_graft hook documentation anymore.  Wondering if we should assert
> that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case
> (i.e. that sksec->sid has been set prior to copying it to isec->sid) and
> that sksec->sid is SECINITSID_UNLABELED (i.e. that it has not already
> been set by the protocol implementation) in the default case.
> We need to update the hook documentation too.

Since we can't return an error code, we would be stuck with a BUG_ON() which 
is okay, but doesn't help in the situation where the kernel has compiled out 
the BUG/BUG_ON macros.  Regardless, there probably is some value in adding a 
BUG_ON(), if for no other reason than documentation.

I'll see about correcting the comment in, thanks for catching that.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Paul Moore]

On Thursday, July 10, 2014 01:45:55 PM Stephen Smalley wrote:
> It seems a bit fragile though and certainly doesn't align with the
> sock_graft hook documentation anymore.  Wondering if we should assert
> that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case
> (i.e. that sksec->sid has been set prior to copying it to isec->sid) ...

Now that I'm changing the code I'm wondering if we could ever arrive at a 
situation where sksec->sid would legitimately end up as SECINITSID_UNLABELED 
in selinux_sock_graft().  In the normal case of no labeled networking, the 
sksec->sid label should end up as SECSID_NULL so I'm not to worried about 
that, but if would you get the SECINITSID_UNLABELED SID if you fed 
"...:unlabeled_t:..." into security_secctx_to_secid()?  I'm pretty sure the 
answer is "no", but I haven't looked at that code in a while.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Stephen Smalley]

On 07/10/2014 03:47 PM, Paul Moore wrote:
> On Thursday, July 10, 2014 01:45:55 PM Stephen Smalley wrote:
>> It seems a bit fragile though and certainly doesn't align with the
>> sock_graft hook documentation anymore.  Wondering if we should assert
>> that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case
>> (i.e. that sksec->sid has been set prior to copying it to isec->sid) ...
> 
> Now that I'm changing the code I'm wondering if we could ever arrive at a 
> situation where sksec->sid would legitimately end up as SECINITSID_UNLABELED 
> in selinux_sock_graft().  In the normal case of no labeled networking, the 
> sksec->sid label should end up as SECSID_NULL so I'm not to worried about 
> that, but if would you get the SECINITSID_UNLABELED SID if you fed 
> "...:unlabeled_t:..." into security_secctx_to_secid()?  I'm pretty sure the 
> answer is "no", but I haven't looked at that code in a while.

Yes, you can get back an initial SID from security_secctx_to_secid() if
the context string matches.  In what scenario would sksec->sid be set to
the result of security_secctx_to_secid() on a context string though, and
from where would that context string originate?  At most we only copy
the MLS attributes of the peer label, right?

Re: [PATCH] selinux: fix the default socket labeling in sock_graft()

[Paul Moore]

On Thursday, July 10, 2014 03:57:15 PM Stephen Smalley wrote:
> On 07/10/2014 03:47 PM, Paul Moore wrote:
> > On Thursday, July 10, 2014 01:45:55 PM Stephen Smalley wrote:
> >> It seems a bit fragile though and certainly doesn't align with the
> >> sock_graft hook documentation anymore.  Wondering if we should assert
> >> that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case
> >> (i.e. that sksec->sid has been set prior to copying it to isec->sid) ...
> > 
> > Now that I'm changing the code I'm wondering if we could ever arrive at a
> > situation where sksec->sid would legitimately end up as
> > SECINITSID_UNLABELED in selinux_sock_graft().  In the normal case of no
> > labeled networking, the sksec->sid label should end up as SECSID_NULL so
> > I'm not to worried about that, but if would you get the
> > SECINITSID_UNLABELED SID if you fed "...:unlabeled_t:..." into
> > security_secctx_to_secid()?  I'm pretty sure the answer is "no", but I
> > haven't looked at that code in a while.
> 
> Yes, you can get back an initial SID from security_secctx_to_secid() if
> the context string matches.  In what scenario would sksec->sid be set to
> the result of security_secctx_to_secid() on a context string though, and
> from where would that context string originate?  At most we only copy
> the MLS attributes of the peer label, right?

Yes, my apologies, I wasn't think through the full process to the MLS 
attribute copy.

Regardless, I suppose I'm still a bit leery of adding the assertion in the 
AF_INET/AF_INET6/AF_UNIX case, especially since that code has been there for 
years now without problem (I know, famous last words).  I don't have a problem 
adding an assertion in the other, default case.

-- 
paul moore
security and virtualization @ redhat