[PATCH] selinux: UNIX domain socket fixes

[PATCH] selinux: UNIX domain socket fixes

[Paul Moore]

Correct a problem where we weren't setting the peer label correctly on
connected UNIX domain sockets and do some other general fixup while we
are messing with the code.

Signed-off-by: Paul Moore <paul.moore@hp.com>

---

This patch has now been tested on 2.6.34-rc3 without any visible problems.
---
 security/selinux/hooks.c |   45 +++++++++++++++++----------------------------
 1 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5feecb4..326e014 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4002,56 +4002,45 @@ static int selinux_socket_unix_stream_connect(struct socket *sock,
 					      struct socket *other,
 					      struct sock *newsk)
 {
-	struct sk_security_struct *ssec;
-	struct inode_security_struct *isec;
-	struct inode_security_struct *other_isec;
+	struct sk_security_struct *s_sksec = sock->sk->sk_security;
+	struct sk_security_struct *o_sksec = other->sk->sk_security;
+	struct sk_security_struct *n_sksec = newsk->sk_security;
 	struct common_audit_data ad;
 	int err;
 
-	isec = SOCK_INODE(sock)->i_security;
-	other_isec = SOCK_INODE(other)->i_security;
-
 	COMMON_AUDIT_DATA_INIT(&ad, NET);
 	ad.u.net.sk = other->sk;
 
-	err = avc_has_perm(isec->sid, other_isec->sid,
-			   isec->sclass,
+	err = avc_has_perm(s_sksec->sid, o_sksec->sid, o_sksec->sclass,
 			   UNIX_STREAM_SOCKET__CONNECTTO, &ad);
 	if (err)
 		return err;
 
-	/* connecting socket */
-	ssec = sock->sk->sk_security;
-	ssec->peer_sid = other_isec->sid;
-
 	/* server child socket */
-	ssec = newsk->sk_security;
-	ssec->peer_sid = isec->sid;
-	err = security_sid_mls_copy(other_isec->sid, ssec->peer_sid, &ssec->sid);
+	n_sksec->peer_sid = s_sksec->sid;
+	err = security_sid_mls_copy(o_sksec->sid, s_sksec->peer_sid,
+				    &n_sksec->sid);
+	if (err)
+		return err;
 
-	return err;
+	/* connecting socket */
+	s_sksec->peer_sid = n_sksec->sid;
+
+	return 0;
 }
 
 static int selinux_socket_unix_may_send(struct socket *sock,
 					struct socket *other)
 {
-	struct inode_security_struct *isec;
-	struct inode_security_struct *other_isec;
+	struct sk_security_struct *ssec = sock->sk->sk_security;
+	struct sk_security_struct *osec = other->sk->sk_security;
 	struct common_audit_data ad;
-	int err;
-
-	isec = SOCK_INODE(sock)->i_security;
-	other_isec = SOCK_INODE(other)->i_security;
 
 	COMMON_AUDIT_DATA_INIT(&ad, NET);
 	ad.u.net.sk = other->sk;
 
-	err = avc_has_perm(isec->sid, other_isec->sid,
-			   isec->sclass, SOCKET__SENDTO, &ad);
-	if (err)
-		return err;
-
-	return 0;
+	return avc_has_perm(ssec->sid, osec->sid, osec->sclass, SOCKET__SENDTO,
+			    &ad);
 }
 
 static int selinux_inet_sys_rcv_skb(int ifindex, char *addrp, u16 family,


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Joe Nall]

On Apr 5, 2010, at 2:01 PM, Paul Moore wrote:

> Correct a problem where we weren't setting the peer label correctly on
> connected UNIX domain sockets and do some other general fixup while we
> are messing with the code.
> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>

Paul,
Do you have a before/after test case?

joe


> 
> ---
> 
> This patch has now been tested on 2.6.34-rc3 without any visible problems.
> ---
> security/selinux/hooks.c |   45 +++++++++++++++++----------------------------
> 1 files changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5feecb4..326e014 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4002,56 +4002,45 @@ static int selinux_socket_unix_stream_connect(struct socket *sock,
> 					      struct socket *other,
> 					      struct sock *newsk)
> {
> -	struct sk_security_struct *ssec;
> -	struct inode_security_struct *isec;
> -	struct inode_security_struct *other_isec;
> +	struct sk_security_struct *s_sksec = sock->sk->sk_security;
> +	struct sk_security_struct *o_sksec = other->sk->sk_security;
> +	struct sk_security_struct *n_sksec = newsk->sk_security;
> 	struct common_audit_data ad;
> 	int err;
> 
> -	isec = SOCK_INODE(sock)->i_security;
> -	other_isec = SOCK_INODE(other)->i_security;
> -
> 	COMMON_AUDIT_DATA_INIT(&ad, NET);
> 	ad.u.net.sk = other->sk;
> 
> -	err = avc_has_perm(isec->sid, other_isec->sid,
> -			   isec->sclass,
> +	err = avc_has_perm(s_sksec->sid, o_sksec->sid, o_sksec->sclass,
> 			   UNIX_STREAM_SOCKET__CONNECTTO, &ad);
> 	if (err)
> 		return err;
> 
> -	/* connecting socket */
> -	ssec = sock->sk->sk_security;
> -	ssec->peer_sid = other_isec->sid;
> -
> 	/* server child socket */
> -	ssec = newsk->sk_security;
> -	ssec->peer_sid = isec->sid;
> -	err = security_sid_mls_copy(other_isec->sid, ssec->peer_sid, &ssec->sid);
> +	n_sksec->peer_sid = s_sksec->sid;
> +	err = security_sid_mls_copy(o_sksec->sid, s_sksec->peer_sid,
> +				    &n_sksec->sid);
> +	if (err)
> +		return err;
> 
> -	return err;
> +	/* connecting socket */
> +	s_sksec->peer_sid = n_sksec->sid;
> +
> +	return 0;
> }
> 
> static int selinux_socket_unix_may_send(struct socket *sock,
> 					struct socket *other)
> {
> -	struct inode_security_struct *isec;
> -	struct inode_security_struct *other_isec;
> +	struct sk_security_struct *ssec = sock->sk->sk_security;
> +	struct sk_security_struct *osec = other->sk->sk_security;
> 	struct common_audit_data ad;
> -	int err;
> -
> -	isec = SOCK_INODE(sock)->i_security;
> -	other_isec = SOCK_INODE(other)->i_security;
> 
> 	COMMON_AUDIT_DATA_INIT(&ad, NET);
> 	ad.u.net.sk = other->sk;
> 
> -	err = avc_has_perm(isec->sid, other_isec->sid,
> -			   isec->sclass, SOCKET__SENDTO, &ad);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return avc_has_perm(ssec->sid, osec->sid, osec->sclass, SOCKET__SENDTO,
> +			    &ad);
> }
> 
> static int selinux_inet_sys_rcv_skb(int ifindex, char *addrp, u16 family,
> 
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Paul Moore]

On Monday 05 April 2010 03:28:12 pm Joe Nall wrote:
> On Apr 5, 2010, at 2:01 PM, Paul Moore wrote:
> > Correct a problem where we weren't setting the peer label correctly on
> > connected UNIX domain sockets and do some other general fixup while we
> > are messing with the code.
> > 
> > Signed-off-by: Paul Moore <paul.moore@hp.com>
> 
> Paul,
> Do you have a before/after test case?

Not really a before/after no, as I don't have anything that really performs a 
getpeercon() on the client end as typically only the server side cares about 
the peer's label (or at least that has been my experience).  What I did test 
was to make sure I didn't see any regressions in the UNIX stream socket 
connections.  To accomplish that I tweaked a little SELinux aware server I use 
for testing INET sockets to make it work with UNIX sockets and connected to it 
with socat at a variety of different levels, making sure getpeercon() always 
displayed the correct level over a UNIX socket connection.

You can find a copy of my little test server at the URL below; I will caution 
you it isn't particularly well written but it works well for situations like 
these.

 * http://free.linux.hp.com/~pmoore/files/getpeercon_server.c

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Paul Moore]

On Monday 05 April 2010 03:01:24 pm Paul Moore wrote:
> Correct a problem where we weren't setting the peer label correctly on
> connected UNIX domain sockets and do some other general fixup while we
> are messing with the code.
> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>

Does anyone object to this patch going in?

I know Stephen had a comment about some of the changes regarding the inode and 
sock security structs, but I haven't heard anything since my response to him 
so I'm going to assume he's okay with the changes (speak up if your not).

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Eric Paris]

On Thu, Apr 8, 2010 at 11:45 AM, Paul Moore <paul.moore@hp.com> wrote:
> On Monday 05 April 2010 03:01:24 pm Paul Moore wrote:
>> Correct a problem where we weren't setting the peer label correctly on
>> connected UNIX domain sockets and do some other general fixup while we
>> are messing with the code.
>>
>> Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> Does anyone object to this patch going in?
>
> I know Stephen had a comment about some of the changes regarding the inode and
> sock security structs, but I haven't heard anything since my response to him
> so I'm going to assume he's okay with the changes (speak up if your not).

It's going to conflict with the patch James committed last night from
me to rename all of the "ssec" in the code to sksec.  I'm pretty sure
James can handle that conflict, but just so we know it is there.....

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Paul Moore]

On Thursday 08 April 2010 12:01:54 pm Eric Paris wrote:
> On Thu, Apr 8, 2010 at 11:45 AM, Paul Moore <paul.moore@hp.com> wrote:
> > On Monday 05 April 2010 03:01:24 pm Paul Moore wrote:
> >> Correct a problem where we weren't setting the peer label correctly on
> >> connected UNIX domain sockets and do some other general fixup while we
> >> are messing with the code.
> >> 
> >> Signed-off-by: Paul Moore <paul.moore@hp.com>
> > 
> > Does anyone object to this patch going in?
> > 
> > I know Stephen had a comment about some of the changes regarding the
> > inode and sock security structs, but I haven't heard anything since my
> > response to him so I'm going to assume he's okay with the changes (speak
> > up if your not).

Okay, thanks; I thought that was fixed, guess not.

There is a joke about SELinux and networking here I'm sure, but I'm not going 
to make it ;)

> It's going to conflict with the patch James committed last night from
> me to rename all of the "ssec" in the code to sksec.  I'm pretty sure
> James can handle that conflict, but just so we know it is there.....

Yeah, your patch is what jogged my memory on this.  As you say, the merge 
should be trivial, but I can do a quick re-spin if needed; I just didn't want 
to spend the time if there is a complaint against the patch and I need to re-
work it.

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Stephen Smalley]

On Mon, 2010-04-05 at 15:01 -0400, Paul Moore wrote:
> Correct a problem where we weren't setting the peer label correctly on
> connected UNIX domain sockets and do some other general fixup while we
> are messing with the code.

I think you should state the user-visible behavior change(s) more
explicitly, e.g.
- getpeercon() by client on connected LOCAL socket will now return the
context of the new connection / server socket rather than the context of
the listening socket, matching INET behavior.
- All permission checks and peer SID assignment will use the sock SID
rather than the inode SID so if they are ever out of sync, the end
result will be different (different permission check, different peer SID
visible).  Only case where that can happen today is if someone is using
fsetxattr() on sockets, which isn't allowed by refpolicy and isn't truly
supported (needs a .setxattr handler in the socket inode code that
propagates the SID down to the sock).

> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>
> 
> ---
> 
> This patch has now been tested on 2.6.34-rc3 without any visible problems.
> ---
>  security/selinux/hooks.c |   45 +++++++++++++++++----------------------------
>  1 files changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5feecb4..326e014 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4002,56 +4002,45 @@ static int selinux_socket_unix_stream_connect(struct socket *sock,
>  					      struct socket *other,
>  					      struct sock *newsk)
>  {
> -	struct sk_security_struct *ssec;
> -	struct inode_security_struct *isec;
> -	struct inode_security_struct *other_isec;
> +	struct sk_security_struct *s_sksec = sock->sk->sk_security;
> +	struct sk_security_struct *o_sksec = other->sk->sk_security;
> +	struct sk_security_struct *n_sksec = newsk->sk_security;
>  	struct common_audit_data ad;
>  	int err;
>  
> -	isec = SOCK_INODE(sock)->i_security;
> -	other_isec = SOCK_INODE(other)->i_security;
> -
>  	COMMON_AUDIT_DATA_INIT(&ad, NET);
>  	ad.u.net.sk = other->sk;
>  
> -	err = avc_has_perm(isec->sid, other_isec->sid,
> -			   isec->sclass,
> +	err = avc_has_perm(s_sksec->sid, o_sksec->sid, o_sksec->sclass,
>  			   UNIX_STREAM_SOCKET__CONNECTTO, &ad);
>  	if (err)
>  		return err;
>  
> -	/* connecting socket */
> -	ssec = sock->sk->sk_security;
> -	ssec->peer_sid = other_isec->sid;
> -
>  	/* server child socket */
> -	ssec = newsk->sk_security;
> -	ssec->peer_sid = isec->sid;
> -	err = security_sid_mls_copy(other_isec->sid, ssec->peer_sid, &ssec->sid);
> +	n_sksec->peer_sid = s_sksec->sid;
> +	err = security_sid_mls_copy(o_sksec->sid, s_sksec->peer_sid,
> +				    &n_sksec->sid);

Here you use s_sksec->peer_sid.

> +	if (err)
> +		return err;
>  
> -	return err;
> +	/* connecting socket */
> +	s_sksec->peer_sid = n_sksec->sid;

Here you assign s_sksec->peer_sid.

That's a bug introduced by the "clean up", I think.  Which is why I'd
prefer separating the bug fix from the cleanup generally.


> +
> +	return 0;
>  }
>  
>  static int selinux_socket_unix_may_send(struct socket *sock,
>  					struct socket *other)
>  {
> -	struct inode_security_struct *isec;
> -	struct inode_security_struct *other_isec;
> +	struct sk_security_struct *ssec = sock->sk->sk_security;
> +	struct sk_security_struct *osec = other->sk->sk_security;
>  	struct common_audit_data ad;
> -	int err;
> -
> -	isec = SOCK_INODE(sock)->i_security;
> -	other_isec = SOCK_INODE(other)->i_security;
>  
>  	COMMON_AUDIT_DATA_INIT(&ad, NET);
>  	ad.u.net.sk = other->sk;
>  
> -	err = avc_has_perm(isec->sid, other_isec->sid,
> -			   isec->sclass, SOCKET__SENDTO, &ad);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return avc_has_perm(ssec->sid, osec->sid, osec->sclass, SOCKET__SENDTO,
> +			    &ad);
>  }
>  
>  static int selinux_inet_sys_rcv_skb(int ifindex, char *addrp, u16 family,
> 
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: UNIX domain socket fixes

[Paul Moore]

On Thursday 08 April 2010 01:15:03 pm Stephen Smalley wrote:
> On Mon, 2010-04-05 at 15:01 -0400, Paul Moore wrote:
> > Correct a problem where we weren't setting the peer label correctly on
> > connected UNIX domain sockets and do some other general fixup while we
> > are messing with the code.
> 
> I think you should state the user-visible behavior change(s) more
> explicitly, e.g.
> - getpeercon() by client on connected LOCAL socket will now return the
> context of the new connection / server socket rather than the context of
> the listening socket, matching INET behavior.
> - All permission checks and peer SID assignment will use the sock SID
> rather than the inode SID so if they are ever out of sync, the end
> result will be different (different permission check, different peer SID
> visible).  Only case where that can happen today is if someone is using
> fsetxattr() on sockets, which isn't allowed by refpolicy and isn't truly
> supported (needs a .setxattr handler in the socket inode code that
> propagates the SID down to the sock).

Thanks for the response, didn't realize you guys were still having network 
problems.

Let's just nix this patch for right now, while the client getpeercon() label 
is an issue, it is unlikely that it is causing anyone a problem and you're 
right, we really should fix the setxattr handler for sockets; it has been on 
my todo list (although very near the bottom) for some time now and this seems 
like as good an excuse as any.  I'll start working on a patchset to address 
that, as well as the other things already discussed, but I think that is more 
of a 2.6.35 candidate than something worth putting into 2.6.34-rcX.

> > Signed-off-by: Paul Moore <paul.moore@hp.com>
> > 
> > ---
> > 
> > This patch has now been tested on 2.6.34-rc3 without any visible
> > problems. ---
> > 
> >  security/selinux/hooks.c |   45
> >  +++++++++++++++++---------------------------- 1 files changed, 17
> >  insertions(+), 28 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 5feecb4..326e014 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4002,56 +4002,45 @@ static int
> > selinux_socket_unix_stream_connect(struct socket *sock,
> > 
> >  					      struct socket *other,
> >  					      struct sock *newsk)
> >  
> >  {
> > 
> > -	struct sk_security_struct *ssec;
> > -	struct inode_security_struct *isec;
> > -	struct inode_security_struct *other_isec;
> > +	struct sk_security_struct *s_sksec = sock->sk->sk_security;
> > +	struct sk_security_struct *o_sksec = other->sk->sk_security;
> > +	struct sk_security_struct *n_sksec = newsk->sk_security;
> > 
> >  	struct common_audit_data ad;
> >  	int err;
> > 
> > -	isec = SOCK_INODE(sock)->i_security;
> > -	other_isec = SOCK_INODE(other)->i_security;
> > -
> > 
> >  	COMMON_AUDIT_DATA_INIT(&ad, NET);
> >  	ad.u.net.sk = other->sk;
> > 
> > -	err = avc_has_perm(isec->sid, other_isec->sid,
> > -			   isec->sclass,
> > +	err = avc_has_perm(s_sksec->sid, o_sksec->sid, o_sksec->sclass,
> > 
> >  			   UNIX_STREAM_SOCKET__CONNECTTO, &ad);
> >  	
> >  	if (err)
> >  	
> >  		return err;
> > 
> > -	/* connecting socket */
> > -	ssec = sock->sk->sk_security;
> > -	ssec->peer_sid = other_isec->sid;
> > -
> > 
> >  	/* server child socket */
> > 
> > -	ssec = newsk->sk_security;
> > -	ssec->peer_sid = isec->sid;
> > -	err = security_sid_mls_copy(other_isec->sid, ssec->peer_sid,
> > &ssec->sid); +	n_sksec->peer_sid = s_sksec->sid;
> > +	err = security_sid_mls_copy(o_sksec->sid, s_sksec->peer_sid,
> > +				    &n_sksec->sid);
> 
> Here you use s_sksec->peer_sid.
> 
> > +	if (err)
> > +		return err;
> > 
> > -	return err;
> > +	/* connecting socket */
> > +	s_sksec->peer_sid = n_sksec->sid;
> 
> Here you assign s_sksec->peer_sid.
> 
> That's a bug introduced by the "clean up", I think.  Which is why I'd
> prefer separating the bug fix from the cleanup generally.
> 
> > +
> > +	return 0;
> > 
> >  }
> >  
> >  static int selinux_socket_unix_may_send(struct socket *sock,
> >  
> >  					struct socket *other)
> >  
> >  {
> > 
> > -	struct inode_security_struct *isec;
> > -	struct inode_security_struct *other_isec;
> > +	struct sk_security_struct *ssec = sock->sk->sk_security;
> > +	struct sk_security_struct *osec = other->sk->sk_security;
> > 
> >  	struct common_audit_data ad;
> > 
> > -	int err;
> > -
> > -	isec = SOCK_INODE(sock)->i_security;
> > -	other_isec = SOCK_INODE(other)->i_security;
> > 
> >  	COMMON_AUDIT_DATA_INIT(&ad, NET);
> >  	ad.u.net.sk = other->sk;
> > 
> > -	err = avc_has_perm(isec->sid, other_isec->sid,
> > -			   isec->sclass, SOCKET__SENDTO, &ad);
> > -	if (err)
> > -		return err;
> > -
> > -	return 0;
> > +	return avc_has_perm(ssec->sid, osec->sid, osec->sclass, 
SOCKET__SENDTO,
> > +			    &ad);
> > 
> >  }
> >  
> >  static int selinux_inet_sys_rcv_skb(int ifindex, char *addrp, u16
> >  family,
> > 
> > --
> > This message was distributed to subscribers of the selinux mailing list.
> > If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov
> > with the words "unsubscribe selinux" without quotes as the message.

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.