[RFC PATCH v1 0/6] UNIX domain socket fixes and other cleanups

[RFC PATCH v1 0/6] UNIX domain socket fixes and other cleanups

[Paul Moore]

Hello all,

This patchset grew out of the SELinux UNIX domain socket patch that I
kicked around on this list several weeks ago that fixed a problem where
we weren't always setting a UNIX socket's peer label correctly.  This
patchset still includes this fix but it also includes a number of other
improvements.  I'm posting these patches as an RFC for two main reasons,
I haven't had a chance to give them the testing I want (they boot and
there are no obvious regressions in light usage) and they are based of
Linus' tree and not security-testing (I will fix that before submission).
However, if you want to give the patches a shot or even just review them
I would appreciate any feedback you care to send along.

For those of you who like to get your patches via git, this patchset can
also be found at the URL below:

 * git://git.infradead.org/users/pcmoore/lblnet-2.6_testing

---

Paul Moore (6):
      selinux: Update socket's label alongside inode's label
      selinux: Set the peer label correctly on connected UNIX domain sockets
      selinux: Consolidate sockcreate_sid logic
      selinux: Shuffle the sk_security_struct alloc and free routines
      selinux: Convert socket related access controls to use socket labels
      selinux: Use current_security() when possible


 security/selinux/hooks.c            |  282 ++++++++++++++++-------------------
 security/selinux/include/netlabel.h |    5 -
 security/selinux/netlabel.c         |    8 +
 3 files changed, 139 insertions(+), 156 deletions(-)


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

[RFC PATCH v1 1/6] selinux: Update socket's label alongside inode's label

[Paul Moore]

We have always had a potential disconnect between the label on socket and
the label on the associated inode when a user calls fsetxattr() on a
socket.  The problem is that the fsetxattr() call would only relabel the
inode and not the corresponding socket; the good news is that the
mainstream SELinux policies have always prevented this, but better safe
than sorry ...

This patch fixes this problem by adding the necessary socket labeling code
to selinux_inode_setsecurity() so that if a user did relabel a socket via
fsetxattr() both the inode and socket would be relabeled.

Signed-off-by: XXX
---
 security/selinux/hooks.c            |   39 ++++++++++++++++++++++++++++++++++-
 security/selinux/include/netlabel.h |    5 ++--
 security/selinux/netlabel.c         |    8 +++++--
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5feecb4..f9545c8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2920,6 +2920,43 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 	if (rc)
 		return rc;
 
+	if (S_ISSOCK(inode->i_mode)) {
+		struct sock *sk = SOCKET_I(inode)->sk;
+		struct sk_security_struct *sksec = sk->sk_security;
+
+		/* XXX - In order to safely relabel a socket when labeled IPsec
+		 *       is in use we need to also change the corresponding
+		 *       flow secid (if any), if we don't change the flow's
+		 *       secid then we run the risk of mislabeling traffic which
+		 *       is not good.  Since the odds of us hitting this code
+		 *       are very low (actually zero given refpolicy circa 2010)
+		 *       we're not going to expend the effort in relabeling the
+		 *       flow, just cause the fsetxattr() operation to fail
+		 *       which should guarantee labeling safety. */
+		if (selinux_xfrm_enabled())
+			return -EPERM;
+
+		/* It is worth mentioning here that you could potentially see a
+		 * labeling race condition if the socket being relabeled is
+		 * undergoing lots of writes at the same time, as writes sent
+		 * before the fsetxattr() operation may not receive their
+		 * on-the-wire security label until after the fsetxattr()
+		 * completes resulting in pre-fsetxattr() data getting labeled
+		 * with a post-fsetxattr() security label.  However, we're just
+		 * going to assume that if someone is silly enough to try and
+		 * relabel a socket mid-stream then they should bear the
+		 * responsibility of dealing with the potential problems.  It
+		 * is also worth mentioning that this operation is forbidden by
+		 * the 2010 refpolicy for this very reason. */
+		lock_sock(sk);
+		sksec->sid = newsid;
+		selinux_netlbl_sk_security_reset(sksec);
+		rc = selinux_netlbl_socket_setsid(sk, sk->sk_family);
+		release_sock(sk);
+		if (rc)
+			return rc;
+	}
+
 	isec->sid = newsid;
 	isec->initialized = 1;
 	return 0;
@@ -3766,7 +3803,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 		sksec = sock->sk->sk_security;
 		sksec->sid = isec->sid;
 		sksec->sclass = isec->sclass;
-		err = selinux_netlbl_socket_post_create(sock->sk, family);
+		err = selinux_netlbl_socket_setsid(sock->sk, family);
 	}
 
 	return err;
diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
index 8d73842..4edab04 100644
--- a/security/selinux/include/netlabel.h
+++ b/security/selinux/include/netlabel.h
@@ -55,7 +55,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 
 int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family);
 void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family);
-int selinux_netlbl_socket_post_create(struct sock *sk, u16 family);
+int selinux_netlbl_socket_setsid(struct sock *sk, u16 family);
 int selinux_netlbl_sock_rcv_skb(struct sk_security_struct *sksec,
 				struct sk_buff *skb,
 				u16 family,
@@ -121,8 +121,7 @@ static inline void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
 {
 	return;
 }
-static inline int selinux_netlbl_socket_post_create(struct sock *sk,
-						    u16 family)
+static inline int selinux_netlbl_socket_setsid(struct sock *sk, u16 family)
 {
 	return 0;
 }
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 628da72..694508e 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -157,6 +157,10 @@ void selinux_netlbl_sk_security_free(struct sk_security_struct *ssec)
 void selinux_netlbl_sk_security_reset(struct sk_security_struct *ssec)
 {
 	ssec->nlbl_state = NLBL_UNSET;
+	if (ssec->nlbl_secattr != NULL) {
+		netlbl_secattr_free(ssec->nlbl_secattr);
+		ssec->nlbl_secattr = NULL;
+	}
 }
 
 /**
@@ -292,7 +296,7 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
 }
 
 /**
- * selinux_netlbl_socket_post_create - Label a socket using NetLabel
+ * selinux_netlbl_socket_setsid - Label a socket using NetLabel
  * @sock: the socket to label
  * @family: protocol family
  *
@@ -301,7 +305,7 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
  * SID.  Returns zero values on success, negative values on failure.
  *
  */
-int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
+int selinux_netlbl_socket_setsid(struct sock *sk, u16 family)
 {
 	int rc;
 	struct sk_security_struct *sksec = sk->sk_security;


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

[RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Paul Moore]

Correct a problem where we weren't setting the peer label correctly on
the client end of a pair of connected UNIX sockets.

Signed-off-by: XXX
---
 security/selinux/hooks.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f9545c8..09973e2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4039,34 +4039,30 @@ 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 *sksec_s = sock->sk->sk_security;
+	struct sk_security_struct *sksec_o = other->sk->sk_security;
+	struct sk_security_struct *sksec_n = 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(sksec_s->sid, sksec_o->sid, sksec_o->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);
+	sksec_n->peer_sid = sksec_s->sid;
+	err = security_sid_mls_copy(sksec_o->sid, sksec_s->sid, &sksec_n->sid);
+	if (err)
+		return err;
 
-	return err;
+	/* connecting socket */
+	sksec_s->peer_sid = sksec_n->sid;
+
+	return 0;
 }
 
 static int selinux_socket_unix_may_send(struct socket *sock,


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

[RFC PATCH v1 3/6] selinux: Consolidate sockcreate_sid logic

[Paul Moore]

Consolidate the basic sockcreate_sid logic into a single helper function
which allows us to do some cleanups in the related code.

Signed-off-by: XXX
---
 security/selinux/hooks.c |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 09973e2..4034a3a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3730,6 +3730,12 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid)
 }
 
 /* socket security operations */
+
+static u32 socket_sockcreate_sid(const struct task_security_struct *tsec)
+{
+	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;
+}
+
 static int socket_has_perm(struct task_struct *task, struct socket *sock,
 			   u32 perms)
 {
@@ -3757,21 +3763,15 @@ static int selinux_socket_create(int family, int type,
 {
 	const struct cred *cred = current_cred();
 	const struct task_security_struct *tsec = cred->security;
-	u32 sid, newsid;
+	u32 newsid;
 	u16 secclass;
-	int err = 0;
 
 	if (kern)
-		goto out;
-
-	sid = tsec->sid;
-	newsid = tsec->sockcreate_sid ?: sid;
+		return 0;
 
+	newsid = socket_sockcreate_sid(tsec);
 	secclass = socket_type_to_security_class(family, type, protocol);
-	err = avc_has_perm(sid, newsid, secclass, SOCKET__CREATE, NULL);
-
-out:
-	return err;
+	return avc_has_perm(tsec->sid, newsid, secclass, SOCKET__CREATE, NULL);
 }
 
 static int selinux_socket_post_create(struct socket *sock, int family,
@@ -3779,22 +3779,14 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 {
 	const struct cred *cred = current_cred();
 	const struct task_security_struct *tsec = cred->security;
-	struct inode_security_struct *isec;
+	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
 	struct sk_security_struct *sksec;
-	u32 sid, newsid;
 	int err = 0;
 
-	sid = tsec->sid;
-	newsid = tsec->sockcreate_sid;
-
-	isec = SOCK_INODE(sock)->i_security;
-
 	if (kern)
 		isec->sid = SECINITSID_KERNEL;
-	else if (newsid)
-		isec->sid = newsid;
 	else
-		isec->sid = sid;
+		isec->sid = socket_sockcreate_sid(tsec);
 
 	isec->sclass = socket_type_to_security_class(family, type, protocol);
 	isec->initialized = 1;


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

[RFC PATCH v1 4/6] selinux: Shuffle the sk_security_struct alloc and free routines

[Paul Moore]

The sk_alloc_security() and sk_free_security() functions were only being
called by the selinux_sk_alloc_security() and selinux_sk_free_security()
functions so we just move the guts of the alloc/free routines to the
callers and eliminate a layer of indirection.

Signed-off-by: XXX
---
 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 4034a3a..ce6c017 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -291,32 +291,6 @@ static void superblock_free_security(struct super_block *sb)
 	kfree(sbsec);
 }
 
-static int sk_alloc_security(struct sock *sk, int family, gfp_t priority)
-{
-	struct sk_security_struct *ssec;
-
-	ssec = kzalloc(sizeof(*ssec), priority);
-	if (!ssec)
-		return -ENOMEM;
-
-	ssec->peer_sid = SECINITSID_UNLABELED;
-	ssec->sid = SECINITSID_UNLABELED;
-	sk->sk_security = ssec;
-
-	selinux_netlbl_sk_security_reset(ssec);
-
-	return 0;
-}
-
-static void sk_free_security(struct sock *sk)
-{
-	struct sk_security_struct *ssec = sk->sk_security;
-
-	sk->sk_security = NULL;
-	selinux_netlbl_sk_security_free(ssec);
-	kfree(ssec);
-}
-
 /* The security server must be initialized before
    any labeling or access decisions can be provided. */
 extern int ss_initialized;
@@ -4281,12 +4255,27 @@ out:
 
 static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
 {
-	return sk_alloc_security(sk, family, priority);
+	struct sk_security_struct *sksec;
+
+	sksec = kzalloc(sizeof(*sksec), priority);
+	if (!sksec)
+		return -ENOMEM;
+
+	sksec->peer_sid = SECINITSID_UNLABELED;
+	sksec->sid = SECINITSID_UNLABELED;
+	selinux_netlbl_sk_security_reset(sksec);
+	sk->sk_security = sksec;
+
+	return 0;
 }
 
 static void selinux_sk_free_security(struct sock *sk)
 {
-	sk_free_security(sk);
+	struct sk_security_struct *sksec = sk->sk_security;
+
+	sk->sk_security = NULL;
+	selinux_netlbl_sk_security_free(sksec);
+	kfree(sksec);
 }
 
 static void selinux_sk_clone_security(const struct sock *sk, struct sock *newsk)


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

[RFC PATCH v1 5/6] selinux: Convert socket related access controls to use socket labels

[Paul Moore]

At present, the socket related access controls use a mix of inode and
socket labels; while there should be no practical difference (they
_should_ always be the same), it makes the code more confusing.  This
patch attempts to convert all of the socket related access control
points (with the exception of some of the inode/fd based controls) to
use the socket's own label.  In the process, I also converted the
socket_has_perm() function to take a 'sock' argument instead of a
'socket' since that was adding a bit more overhead in some cases.

Signed-off-by: XXX
---
 security/selinux/hooks.c |  121 +++++++++++++++++-----------------------------
 1 files changed, 46 insertions(+), 75 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ce6c017..4f2c9ec 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,26 +3710,19 @@ static u32 socket_sockcreate_sid(const struct task_security_struct *tsec)
 	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;
 }
 
-static int socket_has_perm(struct task_struct *task, struct socket *sock,
-			   u32 perms)
+static int socket_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
 {
-	struct inode_security_struct *isec;
+	struct sk_security_struct *sksec = sk->sk_security;
 	struct common_audit_data ad;
-	u32 sid;
-	int err = 0;
+	u32 tsid = task_sid(task);
 
-	isec = SOCK_INODE(sock)->i_security;
-
-	if (isec->sid == SECINITSID_KERNEL)
-		goto out;
-	sid = task_sid(task);
+	if (sksec->sid == SECINITSID_KERNEL)
+		return 0;
 
 	COMMON_AUDIT_DATA_INIT(&ad, NET);
-	ad.u.net.sk = sock->sk;
-	err = avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
+	ad.u.net.sk = sk;
 
-out:
-	return err;
+	return avc_has_perm(tsid, sksec->sid, sksec->sclass, perms, &ad);
 }
 
 static int selinux_socket_create(int family, int type,
@@ -3781,10 +3774,11 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 
 static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
 {
+	struct sock *sk = sock->sk;
 	u16 family;
 	int err;
 
-	err = socket_has_perm(current, sock, SOCKET__BIND);
+	err = socket_has_perm(current, sk, SOCKET__BIND);
 	if (err)
 		goto out;
 
@@ -3793,19 +3787,16 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 	 * Multiple address binding for SCTP is not supported yet: we just
 	 * check the first address now.
 	 */
-	family = sock->sk->sk_family;
+	family = sk->sk_family;
 	if (family == PF_INET || family == PF_INET6) {
 		char *addrp;
-		struct inode_security_struct *isec;
+		struct sk_security_struct *sksec = sk->sk_security;
 		struct common_audit_data ad;
 		struct sockaddr_in *addr4 = NULL;
 		struct sockaddr_in6 *addr6 = NULL;
 		unsigned short snum;
-		struct sock *sk = sock->sk;
 		u32 sid, node_perm;
 
-		isec = SOCK_INODE(sock)->i_security;
-
 		if (family == PF_INET) {
 			addr4 = (struct sockaddr_in *)address;
 			snum = ntohs(addr4->sin_port);
@@ -3829,15 +3820,15 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 				COMMON_AUDIT_DATA_INIT(&ad, NET);
 				ad.u.net.sport = htons(snum);
 				ad.u.net.family = family;
-				err = avc_has_perm(isec->sid, sid,
-						   isec->sclass,
+				err = avc_has_perm(sksec->sid, sid,
+						   sksec->sclass,
 						   SOCKET__NAME_BIND, &ad);
 				if (err)
 					goto out;
 			}
 		}
 
-		switch (isec->sclass) {
+		switch (sksec->sclass) {
 		case SECCLASS_TCP_SOCKET:
 			node_perm = TCP_SOCKET__NODE_BIND;
 			break;
@@ -3868,8 +3859,8 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 		else
 			ipv6_addr_copy(&ad.u.net.v6info.saddr, &addr6->sin6_addr);
 
-		err = avc_has_perm(isec->sid, sid,
-				   isec->sclass, node_perm, &ad);
+		err = avc_has_perm(sksec->sid, sid,
+				   sksec->sclass, node_perm, &ad);
 		if (err)
 			goto out;
 	}
@@ -3880,19 +3871,18 @@ out:
 static int selinux_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen)
 {
 	struct sock *sk = sock->sk;
-	struct inode_security_struct *isec;
+	struct sk_security_struct *sksec = sk->sk_security;
 	int err;
 
-	err = socket_has_perm(current, sock, SOCKET__CONNECT);
+	err = socket_has_perm(current, sk, SOCKET__CONNECT);
 	if (err)
 		return err;
 
 	/*
 	 * If a TCP or DCCP socket, check name_connect permission for the port.
 	 */
-	isec = SOCK_INODE(sock)->i_security;
-	if (isec->sclass == SECCLASS_TCP_SOCKET ||
-	    isec->sclass == SECCLASS_DCCP_SOCKET) {
+	if (sksec->sclass == SECCLASS_TCP_SOCKET ||
+	    sksec->sclass == SECCLASS_DCCP_SOCKET) {
 		struct common_audit_data ad;
 		struct sockaddr_in *addr4 = NULL;
 		struct sockaddr_in6 *addr6 = NULL;
@@ -3915,13 +3905,13 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
 		if (err)
 			goto out;
 
-		perm = (isec->sclass == SECCLASS_TCP_SOCKET) ?
+		perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
 		       TCP_SOCKET__NAME_CONNECT : DCCP_SOCKET__NAME_CONNECT;
 
 		COMMON_AUDIT_DATA_INIT(&ad, NET);
 		ad.u.net.dport = htons(snum);
 		ad.u.net.family = sk->sk_family;
-		err = avc_has_perm(isec->sid, sid, isec->sclass, perm, &ad);
+		err = avc_has_perm(sksec->sid, sid, sksec->sclass, perm, &ad);
 		if (err)
 			goto out;
 	}
@@ -3934,7 +3924,7 @@ out:
 
 static int selinux_socket_listen(struct socket *sock, int backlog)
 {
-	return socket_has_perm(current, sock, SOCKET__LISTEN);
+	return socket_has_perm(current, sock->sk, SOCKET__LISTEN);
 }
 
 static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
@@ -3943,7 +3933,7 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
 	struct inode_security_struct *isec;
 	struct inode_security_struct *newisec;
 
-	err = socket_has_perm(current, sock, SOCKET__ACCEPT);
+	err = socket_has_perm(current, sock->sk, SOCKET__ACCEPT);
 	if (err)
 		return err;
 
@@ -3960,30 +3950,30 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
 static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
 				  int size)
 {
-	return socket_has_perm(current, sock, SOCKET__WRITE);
+	return socket_has_perm(current, sock->sk, SOCKET__WRITE);
 }
 
 static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 				  int size, int flags)
 {
-	return socket_has_perm(current, sock, SOCKET__READ);
+	return socket_has_perm(current, sock->sk, SOCKET__READ);
 }
 
 static int selinux_socket_getsockname(struct socket *sock)
 {
-	return socket_has_perm(current, sock, SOCKET__GETATTR);
+	return socket_has_perm(current, sock->sk, SOCKET__GETATTR);
 }
 
 static int selinux_socket_getpeername(struct socket *sock)
 {
-	return socket_has_perm(current, sock, SOCKET__GETATTR);
+	return socket_has_perm(current, sock->sk, SOCKET__GETATTR);
 }
 
 static int selinux_socket_setsockopt(struct socket *sock, int level, int optname)
 {
 	int err;
 
-	err = socket_has_perm(current, sock, SOCKET__SETOPT);
+	err = socket_has_perm(current, sock->sk, SOCKET__SETOPT);
 	if (err)
 		return err;
 
@@ -3993,12 +3983,12 @@ static int selinux_socket_setsockopt(struct socket *sock, int level, int optname
 static int selinux_socket_getsockopt(struct socket *sock, int level,
 				     int optname)
 {
-	return socket_has_perm(current, sock, SOCKET__GETOPT);
+	return socket_has_perm(current, sock->sk, SOCKET__GETOPT);
 }
 
 static int selinux_socket_shutdown(struct socket *sock, int how)
 {
-	return socket_has_perm(current, sock, SOCKET__SHUTDOWN);
+	return socket_has_perm(current, sock->sk, SOCKET__SHUTDOWN);
 }
 
 static int selinux_socket_unix_stream_connect(struct socket *sock,
@@ -4034,23 +4024,15 @@ static int selinux_socket_unix_stream_connect(struct socket *sock,
 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,
@@ -4189,26 +4171,18 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
 	int err = 0;
 	char *scontext;
 	u32 scontext_len;
-	struct sk_security_struct *ssec;
-	struct inode_security_struct *isec;
+	struct sk_security_struct *sksec = sock->sk->sk_security;
 	u32 peer_sid = SECSID_NULL;
 
-	isec = SOCK_INODE(sock)->i_security;
-
-	if (isec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
-	    isec->sclass == SECCLASS_TCP_SOCKET) {
-		ssec = sock->sk->sk_security;
-		peer_sid = ssec->peer_sid;
-	}
-	if (peer_sid == SECSID_NULL) {
-		err = -ENOPROTOOPT;
-		goto out;
-	}
+	if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
+	    sksec->sclass == SECCLASS_TCP_SOCKET)
+		peer_sid = sksec->peer_sid;
+	if (peer_sid == SECSID_NULL)
+		return -ENOPROTOOPT;
 
 	err = security_sid_to_context(peer_sid, &scontext, &scontext_len);
-
 	if (err)
-		goto out;
+		return err;
 
 	if (scontext_len > len) {
 		err = -ERANGE;
@@ -4221,9 +4195,7 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
 out_len:
 	if (put_user(scontext_len, optlen))
 		err = -EFAULT;
-
 	kfree(scontext);
-out:
 	return err;
 }
 
@@ -4435,8 +4407,7 @@ static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 	int err = 0;
 	u32 perm;
 	struct nlmsghdr *nlh;
-	struct socket *sock = sk->sk_socket;
-	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
+	struct sk_security_struct *sksec = sk->sk_security;
 
 	if (skb->len < NLMSG_SPACE(0)) {
 		err = -EINVAL;
@@ -4444,13 +4415,13 @@ static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 	}
 	nlh = nlmsg_hdr(skb);
 
-	err = selinux_nlmsg_lookup(isec->sclass, nlh->nlmsg_type, &perm);
+	err = selinux_nlmsg_lookup(sksec->sclass, nlh->nlmsg_type, &perm);
 	if (err) {
 		if (err == -EINVAL) {
 			audit_log(current->audit_context, GFP_KERNEL, AUDIT_SELINUX_ERR,
 				  "SELinux:  unrecognized netlink message"
 				  " type=%hu for sclass=%hu\n",
-				  nlh->nlmsg_type, isec->sclass);
+				  nlh->nlmsg_type, sksec->sclass);
 			if (!selinux_enforcing || security_get_allow_unknown())
 				err = 0;
 		}
@@ -4461,7 +4432,7 @@ static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 		goto out;
 	}
 
-	err = socket_has_perm(current, sock, perm);
+	err = socket_has_perm(current, sk, perm);
 out:
 	return err;
 }


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

[RFC PATCH v1 6/6] selinux: Use current_security() when possible

[Paul Moore]

There were a number of places using the following code pattern:

  struct cred *cred = current_cred();
  struct task_security_struct *tsec = cred->security;

... which were simplified to the following:

  struct task_security_struct *tsec = current_security();

Signed-off-by: XXX
---
 security/selinux/hooks.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4f2c9ec..ccc8e0e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -193,7 +193,7 @@ static inline u32 task_sid(const struct task_struct *task)
  */
 static inline u32 current_sid(void)
 {
-	const struct task_security_struct *tsec = current_cred()->security;
+	const struct task_security_struct *tsec = current_security();
 
 	return tsec->sid;
 }
@@ -1580,8 +1580,7 @@ static int may_create(struct inode *dir,
 		      struct dentry *dentry,
 		      u16 tclass)
 {
-	const struct cred *cred = current_cred();
-	const struct task_security_struct *tsec = cred->security;
+	const struct task_security_struct *tsec = current_security();
 	struct inode_security_struct *dsec;
 	struct superblock_security_struct *sbsec;
 	u32 sid, newsid;
@@ -2179,8 +2178,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 
 static int selinux_bprm_secureexec(struct linux_binprm *bprm)
 {
-	const struct cred *cred = current_cred();
-	const struct task_security_struct *tsec = cred->security;
+	const struct task_security_struct *tsec = current_security();
 	u32 sid, osid;
 	int atsecure = 0;
 
@@ -2555,8 +2553,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 				       char **name, void **value,
 				       size_t *len)
 {
-	const struct cred *cred = current_cred();
-	const struct task_security_struct *tsec = cred->security;
+	const struct task_security_struct *tsec = current_security();
 	struct inode_security_struct *dsec;
 	struct superblock_security_struct *sbsec;
 	u32 sid, newsid, clen;
@@ -3728,8 +3725,7 @@ static int socket_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
 static int selinux_socket_create(int family, int type,
 				 int protocol, int kern)
 {
-	const struct cred *cred = current_cred();
-	const struct task_security_struct *tsec = cred->security;
+	const struct task_security_struct *tsec = current_security();
 	u32 newsid;
 	u16 secclass;
 
@@ -3744,8 +3740,7 @@ static int selinux_socket_create(int family, int type,
 static int selinux_socket_post_create(struct socket *sock, int family,
 				      int type, int protocol, int kern)
 {
-	const struct cred *cred = current_cred();
-	const struct task_security_struct *tsec = cred->security;
+	const struct task_security_struct *tsec = current_security();
 	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
 	struct sk_security_struct *sksec;
 	int err = 0;


--
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: [RFC PATCH v1 1/6] selinux: Update socket's label alongside inode's label

[Stephen Smalley]

On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> We have always had a potential disconnect between the label on socket and
> the label on the associated inode when a user calls fsetxattr() on a
> socket.  The problem is that the fsetxattr() call would only relabel the
> inode and not the corresponding socket; the good news is that the
> mainstream SELinux policies have always prevented this, but better safe
> than sorry ...
> 
> This patch fixes this problem by adding the necessary socket labeling code
> to selinux_inode_setsecurity() so that if a user did relabel a socket via
> fsetxattr() both the inode and socket would be relabeled.
> 
> Signed-off-by: XXX
> ---
>  security/selinux/hooks.c            |   39 ++++++++++++++++++++++++++++++++++-
>  security/selinux/include/netlabel.h |    5 ++--
>  security/selinux/netlabel.c         |    8 +++++--
>  3 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5feecb4..f9545c8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2920,6 +2920,43 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>  	if (rc)
>  		return rc;
>  
> +	if (S_ISSOCK(inode->i_mode)) {

I think this is the wrong test - it would evaluate to true for both the
socket inode and for the file inode by which the socket is named, which
are separate and distinct objects.  I think you want:
	if (inode->i_sb->s_magic == SOCKFS_MAGIC)
which would only be true for the actual socket inode.

> +		struct sock *sk = SOCKET_I(inode)->sk;
> +		struct sk_security_struct *sksec = sk->sk_security;
> +
> +		/* XXX - In order to safely relabel a socket when labeled IPsec
> +		 *       is in use we need to also change the corresponding
> +		 *       flow secid (if any), if we don't change the flow's
> +		 *       secid then we run the risk of mislabeling traffic which
> +		 *       is not good.  Since the odds of us hitting this code
> +		 *       are very low (actually zero given refpolicy circa 2010)
> +		 *       we're not going to expend the effort in relabeling the
> +		 *       flow, just cause the fsetxattr() operation to fail
> +		 *       which should guarantee labeling safety. */
> +		if (selinux_xfrm_enabled())
> +			return -EPERM;
> +
> +		/* It is worth mentioning here that you could potentially see a
> +		 * labeling race condition if the socket being relabeled is
> +		 * undergoing lots of writes at the same time, as writes sent
> +		 * before the fsetxattr() operation may not receive their
> +		 * on-the-wire security label until after the fsetxattr()
> +		 * completes resulting in pre-fsetxattr() data getting labeled
> +		 * with a post-fsetxattr() security label.  However, we're just
> +		 * going to assume that if someone is silly enough to try and
> +		 * relabel a socket mid-stream then they should bear the
> +		 * responsibility of dealing with the potential problems.  It
> +		 * is also worth mentioning that this operation is forbidden by
> +		 * the 2010 refpolicy for this very reason. */
> +		lock_sock(sk);
> +		sksec->sid = newsid;
> +		selinux_netlbl_sk_security_reset(sksec);
> +		rc = selinux_netlbl_socket_setsid(sk, sk->sk_family);
> +		release_sock(sk);
> +		if (rc)
> +			return rc;

If the netlabel state change fails, do we want to revert the sksec->sid
to its original value?

> +	}
> +
>  	isec->sid = newsid;
>  	isec->initialized = 1;
>  	return 0;

-- 
Stephen Smalley
National Security Agency


--
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: [RFC PATCH v1 3/6] selinux: Consolidate sockcreate_sid logic

[Stephen Smalley]

On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> Consolidate the basic sockcreate_sid logic into a single helper function
> which allows us to do some cleanups in the related code.
> 
> Signed-off-by: XXX
> ---
>  security/selinux/hooks.c |   32 ++++++++++++--------------------
>  1 files changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 09973e2..4034a3a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3730,6 +3730,12 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid)
>  }
>  
>  /* socket security operations */
> +
> +static u32 socket_sockcreate_sid(const struct task_security_struct *tsec)
> +{
> +	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;

Why is this clearer than just:
	return tsec->sockcreate_sid ?: tsec->sid;

> +}
> +
>  static int socket_has_perm(struct task_struct *task, struct socket *sock,
>  			   u32 perms)
>  {
> @@ -3757,21 +3763,15 @@ static int selinux_socket_create(int family, int type,
>  {
>  	const struct cred *cred = current_cred();
>  	const struct task_security_struct *tsec = cred->security;
> -	u32 sid, newsid;
> +	u32 newsid;
>  	u16 secclass;
> -	int err = 0;
>  
>  	if (kern)
> -		goto out;
> -
> -	sid = tsec->sid;
> -	newsid = tsec->sockcreate_sid ?: sid;
> +		return 0;
>  
> +	newsid = socket_sockcreate_sid(tsec);
>  	secclass = socket_type_to_security_class(family, type, protocol);
> -	err = avc_has_perm(sid, newsid, secclass, SOCKET__CREATE, NULL);
> -
> -out:
> -	return err;
> +	return avc_has_perm(tsec->sid, newsid, secclass, SOCKET__CREATE, NULL);
>  }
>  
>  static int selinux_socket_post_create(struct socket *sock, int family,
> @@ -3779,22 +3779,14 @@ static int selinux_socket_post_create(struct socket *sock, int family,
>  {
>  	const struct cred *cred = current_cred();
>  	const struct task_security_struct *tsec = cred->security;
> -	struct inode_security_struct *isec;
> +	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
>  	struct sk_security_struct *sksec;
> -	u32 sid, newsid;
>  	int err = 0;
>  
> -	sid = tsec->sid;
> -	newsid = tsec->sockcreate_sid;
> -
> -	isec = SOCK_INODE(sock)->i_security;
> -
>  	if (kern)
>  		isec->sid = SECINITSID_KERNEL;
> -	else if (newsid)
> -		isec->sid = newsid;
>  	else
> -		isec->sid = sid;
> +		isec->sid = socket_sockcreate_sid(tsec);
>  
>  	isec->sclass = socket_type_to_security_class(family, type, protocol);
>  	isec->initialized = 1;
> 
> 
> --
> 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.
-- 
Stephen Smalley
National Security Agency


--
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: [RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Stephen Smalley]

On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> Correct a problem where we weren't setting the peer label correctly on
> the client end of a pair of connected UNIX sockets.
> 
> Signed-off-by: XXX
> ---
>  security/selinux/hooks.c |   28 ++++++++++++----------------
>  1 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f9545c8..09973e2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4039,34 +4039,30 @@ 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 *sksec_s = sock->sk->sk_security;
> +	struct sk_security_struct *sksec_o = other->sk->sk_security;
> +	struct sk_security_struct *sksec_n = newsk->sk_security;

Don't you find the code using these names (sksec_[son]) to be rather
difficult to read compared to the old code?

Do we really need the sksec_ prefix?  What is this, BCPL?  Hungarian
notation considered harmful.

At the least, can we use more descriptive suffixes, e.g. _sock, _other,
_new, to match the input argument names, or if you prefer, _client,
_listener, _server?

>  	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(sksec_s->sid, sksec_o->sid, sksec_o->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);
> +	sksec_n->peer_sid = sksec_s->sid;
> +	err = security_sid_mls_copy(sksec_o->sid, sksec_s->sid, &sksec_n->sid);
> +	if (err)
> +		return err;
>  
> -	return err;
> +	/* connecting socket */
> +	sksec_s->peer_sid = sksec_n->sid;
> +
> +	return 0;
>  }
>  
>  static int selinux_socket_unix_may_send(struct socket *sock,
> 
> 
> --
> 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.
-- 
Stephen Smalley
National Security Agency


--
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: [RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Eric Paris]

On Tue, 2010-05-04 at 10:05 -0400, Stephen Smalley wrote:
> On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > Correct a problem where we weren't setting the peer label correctly on
> > the client end of a pair of connected UNIX sockets.
> > 
> > Signed-off-by: XXX
> > ---
> >  security/selinux/hooks.c |   28 ++++++++++++----------------
> >  1 files changed, 12 insertions(+), 16 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f9545c8..09973e2 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4039,34 +4039,30 @@ 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 *sksec_s = sock->sk->sk_security;
> > +	struct sk_security_struct *sksec_o = other->sk->sk_security;
> > +	struct sk_security_struct *sksec_n = newsk->sk_security;
> 
> Don't you find the code using these names (sksec_[son]) to be rather
> difficult to read compared to the old code?

He is probably doing this because I just converted everything that was
an sk_security_struck to sksec for easy grepping in a recent patch.  I'd
be in favor of more than _s _o and _n suffix's though....

-Eric


--
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: [RFC PATCH v1 5/6] selinux: Convert socket related access controls to use socket labels

[Eric Paris]

On Mon, May 3, 2010 at 6:11 PM, Paul Moore <paul.moore@hp.com> wrote:
> At present, the socket related access controls use a mix of inode and
> socket labels; while there should be no practical difference (they
> _should_ always be the same), it makes the code more confusing.  This
> patch attempts to convert all of the socket related access control
> points (with the exception of some of the inode/fd based controls) to
> use the socket's own label.  In the process, I also converted the
> socket_has_perm() function to take a 'sock' argument instead of a
> 'socket' since that was adding a bit more overhead in some cases.

Should it be renamed to sock_has_perm() then?

-Eric


--
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: [RFC PATCH v1 1/6] selinux: Update socket's label alongside inode's label

[Paul Moore]

On Tuesday 04 May 2010 09:03:53 am Stephen Smalley wrote:
> On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > We have always had a potential disconnect between the label on socket and
> > the label on the associated inode when a user calls fsetxattr() on a
> > socket.  The problem is that the fsetxattr() call would only relabel the
> > inode and not the corresponding socket; the good news is that the
> > mainstream SELinux policies have always prevented this, but better safe
> > than sorry ...
> > 
> > This patch fixes this problem by adding the necessary socket labeling
> > code to selinux_inode_setsecurity() so that if a user did relabel a
> > socket via fsetxattr() both the inode and socket would be relabeled.
> > 
> > Signed-off-by: XXX
> > ---
> > 
> >  security/selinux/hooks.c            |   39
> >  ++++++++++++++++++++++++++++++++++- security/selinux/include/netlabel.h
> >  |    5 ++--
> >  security/selinux/netlabel.c         |    8 +++++--
> >  3 files changed, 46 insertions(+), 6 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 5feecb4..f9545c8 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2920,6 +2920,43 @@ static int selinux_inode_setsecurity(struct inode
> > *inode, const char *name,
> > 
> >  	if (rc)
> >  	
> >  		return rc;
> > 
> > +	if (S_ISSOCK(inode->i_mode)) {
> 
> I think this is the wrong test - it would evaluate to true for both the
> socket inode and for the file inode by which the socket is named, which
> are separate and distinct objects.  I think you want:
> 	if (inode->i_sb->s_magic == SOCKFS_MAGIC)
> which would only be true for the actual socket inode.

Thanks for the review ...

Sounds reasonable, I wasn't sure this was 100% right but it seemed like a 
reasonable place to start.  I haven't had a chance to test this yet as I need 
to write a little dummy app that does a fsetxattr() on a socket - amazing that 
apps like this don't exist ;)


> > +		lock_sock(sk);
> > +		sksec->sid = newsid;
> > +		selinux_netlbl_sk_security_reset(sksec);
> > +		rc = selinux_netlbl_socket_setsid(sk, sk->sk_family);
> > +		release_sock(sk);
> > +		if (rc)
> > +			return rc;
> 
> If the netlabel state change fails, do we want to revert the sksec->sid
> to its original value?

Funny you mention this, I was going over the patches again this morning and 
noticed this; I also moved the SID assignments outside of the socket lock, 
probably not a big deal, but you never know ... Regardless, I've already made 
the changes and they will be in the next version.

-- 
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: [RFC PATCH v1 3/6] selinux: Consolidate sockcreate_sid logic

[Paul Moore]

On Tuesday 04 May 2010 09:52:25 am Stephen Smalley wrote:
> On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > Consolidate the basic sockcreate_sid logic into a single helper function
> > which allows us to do some cleanups in the related code.
> > 
> > Signed-off-by: XXX
> > ---
> > 
> >  security/selinux/hooks.c |   32 ++++++++++++--------------------
> >  1 files changed, 12 insertions(+), 20 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 09973e2..4034a3a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3730,6 +3730,12 @@ static int selinux_skb_peerlbl_sid(struct sk_buff
> > *skb, u16 family, u32 *sid)
> > 
> >  }
> >  
> >  /* socket security operations */
> > 
> > +
> > +static u32 socket_sockcreate_sid(const struct task_security_struct
> > *tsec) +{
> > +	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;
> 
> Why is this clearer than just:
> 	return tsec->sockcreate_sid ?: tsec->sid;

It is more explicit?

Honestly, it is just a personal preference thing; if you want it the other way 
just say so and I'll change it back.  The value to me is in the 
socket_has_perm() and _post_create() cleanup ...

-- 
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: [RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Paul Moore]

On Tuesday 04 May 2010 10:27:00 am Eric Paris wrote:
> On Tue, 2010-05-04 at 10:05 -0400, Stephen Smalley wrote:
> > On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > > Correct a problem where we weren't setting the peer label correctly on
> > > the client end of a pair of connected UNIX sockets.
> > > 
> > > Signed-off-by: XXX
> > > ---
> > > 
> > >  security/selinux/hooks.c |   28 ++++++++++++----------------
> > >  1 files changed, 12 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index f9545c8..09973e2 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -4039,34 +4039,30 @@ 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 *sksec_s = sock->sk->sk_security;
> > > +	struct sk_security_struct *sksec_o = other->sk->sk_security;
> > > +	struct sk_security_struct *sksec_n = newsk->sk_security;
> > 
> > Don't you find the code using these names (sksec_[son]) to be rather
> > difficult to read compared to the old code?
> 
> He is probably doing this because I just converted everything that was
> an sk_security_struck to sksec for easy grepping in a recent patch.

Bingo.  Eric's patch seemed like a good idea to me so I'm trying to not break 
the convention here ...

> I'd be in favor of more than _s _o and _n suffix's though....

> > At the least, can we use more descriptive suffixes, e.g. _sock, _other,
> > _new, to match the input argument names, or if you prefer, _client,
> > _listener, _server?

I'm not in love with the names either, if you've got suggestions I'm all ears 
(well, I suppose eyes is more apt unless you want to call me to discuss the 
patches).

-- 
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: [RFC PATCH v1 5/6] selinux: Convert socket related access controls to use socket labels

[Paul Moore]

On Tuesday 04 May 2010 10:30:05 am Eric Paris wrote:
> On Mon, May 3, 2010 at 6:11 PM, Paul Moore <paul.moore@hp.com> wrote:
> > At present, the socket related access controls use a mix of inode and
> > socket labels; while there should be no practical difference (they
> > _should_ always be the same), it makes the code more confusing.  This
> > patch attempts to convert all of the socket related access control
> > points (with the exception of some of the inode/fd based controls) to
> > use the socket's own label.  In the process, I also converted the
> > socket_has_perm() function to take a 'sock' argument instead of a
> > 'socket' since that was adding a bit more overhead in some cases.
> 
> Should it be renamed to sock_has_perm() then?

BTW, thanks for the review too ...

Yeah, I can change the name, in fact, I'll do that right now ...

-- 
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: [RFC PATCH v1 3/6] selinux: Consolidate sockcreate_sid logic

[Stephen Smalley]

On Tue, 2010-05-04 at 11:31 -0400, Paul Moore wrote:
> On Tuesday 04 May 2010 09:52:25 am Stephen Smalley wrote:
> > On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > > Consolidate the basic sockcreate_sid logic into a single helper function
> > > which allows us to do some cleanups in the related code.
> > > 
> > > Signed-off-by: XXX
> > > ---
> > > 
> > >  security/selinux/hooks.c |   32 ++++++++++++--------------------
> > >  1 files changed, 12 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 09973e2..4034a3a 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -3730,6 +3730,12 @@ static int selinux_skb_peerlbl_sid(struct sk_buff
> > > *skb, u16 family, u32 *sid)
> > > 
> > >  }
> > >  
> > >  /* socket security operations */
> > > 
> > > +
> > > +static u32 socket_sockcreate_sid(const struct task_security_struct
> > > *tsec) +{
> > > +	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;
> > 
> > Why is this clearer than just:
> > 	return tsec->sockcreate_sid ?: tsec->sid;
> 
> It is more explicit?
> 
> Honestly, it is just a personal preference thing; if you want it the other way 
> just say so and I'll change it back.  The value to me is in the 
> socket_has_perm() and _post_create() cleanup ...

More opportunity for inconsistency, IMHO.  Nothing wrong with the ?:
syntax.

-- 
Stephen Smalley
National Security Agency


--
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: [RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Stephen Smalley]

On Tue, 2010-05-04 at 11:34 -0400, Paul Moore wrote:
> On Tuesday 04 May 2010 10:27:00 am Eric Paris wrote:
> > On Tue, 2010-05-04 at 10:05 -0400, Stephen Smalley wrote:
> > > On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > > > Correct a problem where we weren't setting the peer label correctly on
> > > > the client end of a pair of connected UNIX sockets.
> > > > 
> > > > Signed-off-by: XXX
> > > > ---
> > > > 
> > > >  security/selinux/hooks.c |   28 ++++++++++++----------------
> > > >  1 files changed, 12 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index f9545c8..09973e2 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -4039,34 +4039,30 @@ 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 *sksec_s = sock->sk->sk_security;
> > > > +	struct sk_security_struct *sksec_o = other->sk->sk_security;
> > > > +	struct sk_security_struct *sksec_n = newsk->sk_security;
> > > 
> > > Don't you find the code using these names (sksec_[son]) to be rather
> > > difficult to read compared to the old code?
> > 
> > He is probably doing this because I just converted everything that was
> > an sk_security_struck to sksec for easy grepping in a recent patch.
> 
> Bingo.  Eric's patch seemed like a good idea to me so I'm trying to not break 
> the convention here ...
> 
> > I'd be in favor of more than _s _o and _n suffix's though....
> 
> > > At the least, can we use more descriptive suffixes, e.g. _sock, _other,
> > > _new, to match the input argument names, or if you prefer, _client,
> > > _listener, _server?
> 
> I'm not in love with the names either, if you've got suggestions I'm all ears 
> (well, I suppose eyes is more apt unless you want to call me to discuss the 
> patches).

Take your pick:
sksec_sock, sksec_other, sksec_newsk OR
sksec_client, sksec_listener, sksec_server

-- 
Stephen Smalley
National Security Agency


--
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: [RFC PATCH v1 2/6] selinux: Set the peer label correctly on connected UNIX domain sockets

[Paul Moore]

On Tuesday 04 May 2010 03:47:17 pm Stephen Smalley wrote:
> On Tue, 2010-05-04 at 11:34 -0400, Paul Moore wrote:
> > I'm not in love with the names either, if you've got suggestions I'm all
> > ears (well, I suppose eyes is more apt unless you want to call me to
> > discuss the patches).
> 
> Take your pick:
> sksec_sock, sksec_other, sksec_newsk OR
> sksec_client, sksec_listener, sksec_server

Okey dokey; sksec_{sock,other,new} it is ...

-- 
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: [RFC PATCH v1 3/6] selinux: Consolidate sockcreate_sid logic

[Paul Moore]

On Tuesday 04 May 2010 03:44:30 pm Stephen Smalley wrote:
> On Tue, 2010-05-04 at 11:31 -0400, Paul Moore wrote:
> > On Tuesday 04 May 2010 09:52:25 am Stephen Smalley wrote:
> > > On Mon, 2010-05-03 at 18:11 -0400, Paul Moore wrote:
> > > > +static u32 socket_sockcreate_sid(const struct task_security_struct
> > > > *tsec) +{
> > > > +	return tsec->sockcreate_sid ? tsec->sockcreate_sid : tsec->sid;
> > > 
> > > Why is this clearer than just:
> > > 	return tsec->sockcreate_sid ?: tsec->sid;
> > 
> > It is more explicit?
> > 
> > Honestly, it is just a personal preference thing; if you want it the
> > other way just say so and I'll change it back.  The value to me is in
> > the
> > socket_has_perm() and _post_create() cleanup ...
> 
> More opportunity for inconsistency, IMHO.  Nothing wrong with the ?:
> syntax.

Okay, while technically I suppose you are correct on the "more opportunity for 
inconsistency" you have to admit the argument is a bit laughable considering 
the complexity of statement and the function itself for that matter ;)

Regardless, I'll make the change ...

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