selinux: UNIX domain socket fixes
PATCH 1/1 selinux: UNIX domain socket fixes
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.
---
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(-)
security/selinux/hooks.c +17 -28
@@ -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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 5 lines · 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 <[email protected]>
Paul,
Do you have a before/after test case?
joe
Quoted 89 lines
---
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(-)security/selinux/hooks.c +17
-29@@ -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, --
Quoted 3 lines
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] 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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 9 lines · Joe Nall
Quoted 5 lines · 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 <[email protected]>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
Signature · 7 lines
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 5 lines · 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 <[email protected]>
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).
Signature · 7 lines
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 5 lines · Paul Moore
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Paul Moore <[email protected]>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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 6 lines · Eric Paris
Quoted 5 lines · Paul Moore
Quoted 4 lines · 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.
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Signed-off-by Paul Moore <[email protected]>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 ;)
Quoted 3 lines
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.
Signature · 7 lines
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 3 lines · 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.
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).
Quoted 41 lines
Signed-off-by Paul Moore <[email protected]>---
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(-)security/selinux/hooks.c +4
-9@@ -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 */
security/selinux/hooks.c static int selinux_socket_unix_stream_connect(struct socket *sock, · line 4022
Quoted 10 lines
security/selinux/hooks.c +3
-6- 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.
security/selinux/hooks.c static int selinux_socket_unix_stream_connect(struct socket *sock, · line 4027
Quoted 6 lines
security/selinux/hooks.c +4
-1+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.
Quoted 34 lines
security/selinux/hooks.c +6
-13+ +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, --
Quoted 3 lines
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] 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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 5 lines · Stephen Smalley
Quoted 3 lines · 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.
Quoted 12 lines
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 usingfsetxattr()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.
Quoted 104 lines
Quoted 53 lines
Signed-off-by Paul Moore <[email protected]>---
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(-)
security/selinux/hooks.c +4
-11@@ -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;security/selinux/hooks.c static int · line 4033
Quoted 10 lines
security/selinux/hooks.c +2
-4-/* 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.security/selinux/hooks.c static int selinux_socket_unix_stream_connect(struct socket *sock, · line 4027
Quoted 6 lines
security/selinux/hooks.c +4
-1+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.
Quoted 26 lines
security/selinux/hooks.c +4
-6+ +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;
Quoted 7 lines
security/selinux/hooks.c static int selinux_socket_unix_stream_connect(struct socket *sock,
Quoted 7 lines
security/selinux/hooks.c +1
-6- 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,
Quoted 11 lines
Quoted 11 lines
security/selinux/hooks.c +1
-1+ &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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Signature · 7 lines
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.