[RFC][PATCH] net/unix: support SCM_SECURITY for stream sockets

[RFC][PATCH] net/unix: support SCM_SECURITY for stream sockets

[Stephen Smalley]

SCM_SECURITY was originally only implemented for datagram sockets,
not for stream sockets.  However, SCM_CREDENTIALS is supported on
Unix stream sockets.  For consistency, implement Unix stream support
for SCM_SECURITY as well.  Also clean up the existing code and get
rid of the superfluous UNIXSID macro.

Motivated by https://bugzilla.redhat.com/show_bug.cgi?id=1224211,
where systemd was using SCM_CREDENTIALS and assumed wrongly that
SCM_SECURITY was also supported on Unix stream sockets.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---

This is an RFC for security folks before I send this along to netdev.
The patch is relative to net-next, not security-next.

 include/net/af_unix.h |  1 -
 net/unix/af_unix.c    | 20 ++++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index a175ba4..4a167b3 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -39,7 +39,6 @@ struct unix_skb_parms {
 };
 
 #define UNIXCB(skb) 	(*(struct unix_skb_parms *)&((skb)->cb))
-#define UNIXSID(skb)	(&UNIXCB((skb)).secid)
 
 #define unix_state_lock(s)	spin_lock(&unix_sk(s)->lock)
 #define unix_state_unlock(s)	spin_unlock(&unix_sk(s)->lock)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f25e167..03ee4d3 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -140,12 +140,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
 #ifdef CONFIG_SECURITY_NETWORK
 static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 {
-	memcpy(UNIXSID(skb), &scm->secid, sizeof(u32));
+	UNIXCB(skb).secid = scm->secid;
 }
 
 static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 {
-	scm->secid = *UNIXSID(skb);
+	scm->secid = UNIXCB(skb).secid;
+}
+
+static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
+{
+	return (scm->secid == UNIXCB(skb).secid);
 }
 #else
 static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
@@ -153,6 +158,11 @@ static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 
 static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 { }
+
+static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
+{
+	return true;
+}
 #endif /* CONFIG_SECURITY_NETWORK */
 
 /*
@@ -1414,6 +1424,7 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
 	UNIXCB(skb).uid = scm->creds.uid;
 	UNIXCB(skb).gid = scm->creds.gid;
 	UNIXCB(skb).fp = NULL;
+	unix_get_secdata(scm, skb);
 	if (scm->fp && send_fds)
 		err = unix_attach_fds(scm, skb);
 
@@ -1509,7 +1520,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (err < 0)
 		goto out_free;
 	max_level = err + 1;
-	unix_get_secdata(&scm, skb);
 
 	skb_put(skb, len - data_len);
 	skb->data_len = data_len;
@@ -2118,11 +2128,13 @@ unlock:
 			/* Never glue messages from different writers */
 			if ((UNIXCB(skb).pid  != scm.pid) ||
 			    !uid_eq(UNIXCB(skb).uid, scm.creds.uid) ||
-			    !gid_eq(UNIXCB(skb).gid, scm.creds.gid))
+			    !gid_eq(UNIXCB(skb).gid, scm.creds.gid) ||
+			    !unix_secdata_eq(&scm, skb))
 				break;
 		} else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
 			/* Copy credentials */
 			scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
+			unix_set_secdata(&scm, skb);
 			check_creds = true;
 		}
 
-- 
2.1.0

Re: [RFC][PATCH] net/unix: support SCM_SECURITY for stream sockets

[Stephen Smalley]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On 06/09/2015 11:47 AM, Stephen Smalley wrote:
> SCM_SECURITY was originally only implemented for datagram sockets,
> not for stream sockets.  However, SCM_CREDENTIALS is supported on
> Unix stream sockets.  For consistency, implement Unix stream support
> for SCM_SECURITY as well.  Also clean up the existing code and get
> rid of the superfluous UNIXSID macro.
> 
> Motivated by https://bugzilla.redhat.com/show_bug.cgi?id=1224211,
> where systemd was using SCM_CREDENTIALS and assumed wrongly that
> SCM_SECURITY was also supported on Unix stream sockets.
> 
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> 
> This is an RFC for security folks before I send this along to netdev.
> The patch is relative to net-next, not security-next.
> 
>  include/net/af_unix.h |  1 -
>  net/unix/af_unix.c    | 20 ++++++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)

Sample test programs attached.

Before:
$ ./server-stream sock1 &
$ ./client-stream sock1
./client-stream: sent   100 bytes
./server-stream: Got SCM_CREDENTIALS=(pid 22067, uid 1000, gid 1000)
./server-stream: Got SCM_SECURITY=system_u:object_r:unlabeled_t:s0

After:
./client-stream: sent   100 bytes
./server-stream: Got SCM_CREDENTIALS=(pid 25490, uid 1000, gid 1000)
./server-stream: Got
SCM_SECURITY=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023



[-- Attachment #2: server-stream.c --]
[-- Type: text/plain, Size: 2698 bytes --]

#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdio.h>

#ifndef SCM_SECURITY
#define SCM_SECURITY 0x03
#endif

static const int on = 1;

int
main(int argc, char **argv)
{
	char buff[65536];
	int sock, newsock;
	int result;
	int sunlen;
	struct sockaddr_un sun, remote_sun;
	socklen_t rsize = sizeof (remote_sun);
	struct iovec iov;
	struct msghdr msg;
	struct cmsghdr *cmsg;
	char label[256];
	union {
		struct cmsghdr cmsghdr;
		char buf[CMSG_SPACE(sizeof(struct ucred)) +
			 CMSG_SPACE(sizeof label)];
	} control;

	if (argc != 2) {
		fprintf(stderr, "usage:  %s socket-name\n", argv[0]);
		exit(1);
	}

	if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
		perror("socket");
		return -1;
	}

	if ((result = setsockopt(sock, SOL_SOCKET, SO_PASSSEC,
				 &on, sizeof (on)))) {
		perror("setsockopt: ");
		close(sock);
		return -1;
	}

	if ((result = setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
				 &on, sizeof (on)))) {
		perror("setsockopt: ");
		close(sock);
		return -1;
	}

	bzero(&sun, sizeof (struct sockaddr_un));
	sun.sun_family = AF_UNIX;
	strcpy(sun.sun_path, argv[1]);
	sunlen = strlen(sun.sun_path) + 1 + sizeof (short);

	unlink(sun.sun_path);
	if (bind(sock, (struct sockaddr *) &sun, sunlen) < 0) {
		perror("bind");
		close(sock);
		return -1;
	}

	if (listen(sock, SOMAXCONN)) {
		perror("listen");
		close(sock);
		return -1;
	}

	newsock = accept(sock, (struct sockaddr *)&remote_sun, &rsize);
	if (newsock < 0) {
		perror("accept");
		close(sock);
		return -1;
	}

	memset(buff, 0, sizeof buff);
	memset(&iov, 0, sizeof iov);
	iov.iov_base = buff;
	iov.iov_len = sizeof(buff);
	memset(&msg, 0, sizeof msg);
	msg.msg_iov = &iov;
	msg.msg_iovlen = 1;
	msg.msg_control = &control;
	msg.msg_controllen = sizeof control;
	result = recvmsg(newsock, &msg, 0);
	if (result < 0) {
		perror("recvmsg");
		exit(1);
	}
	for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
		if (cmsg->cmsg_level == SOL_SOCKET &&
		    cmsg->cmsg_type == SCM_CREDENTIALS &&
		    cmsg->cmsg_len == CMSG_LEN(sizeof(struct ucred))) {
			struct ucred ucred;
			memcpy(&ucred, CMSG_DATA(cmsg), sizeof(struct ucred));
			printf("%s: Got SCM_CREDENTIALS=(pid %u, uid %u, gid %u)\n",
			       argv[0], ucred.pid, ucred.uid, ucred.gid);
		}
		if (cmsg->cmsg_level == SOL_SOCKET &&
		    cmsg->cmsg_type == SCM_SECURITY) {
			size_t len = cmsg->cmsg_len - CMSG_LEN(0);
			if (len > 0) {
				memcpy(label, CMSG_DATA(cmsg), len);
				label[len] = 0;
				printf("%s: Got SCM_SECURITY=%s\n", argv[0],
				       label);
			}

		}
	}
	close(sock);
	unlink(sun.sun_path);
	return (0);
}


[-- Attachment #3: client-stream.c --]
[-- Type: text/plain, Size: 1054 bytes --]

#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>

static const int sendbytes = 100;	/* Send this much data */

int
main(int argc, char **argv)
{
	char data[sendbytes];
	int sock;
	int result;
	struct sockaddr_un sun;
	int sunlen;

	if (argc != 2) {
		fprintf(stderr, "usage:  %s socket-name\n", argv[0]);
		exit(1);
	}

	memset(data, 'X', sendbytes);

	if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
		perror("socket");
		return -1;
	}

	bzero(&sun, sizeof (struct sockaddr_un));
	sun.sun_family = AF_UNIX;
	strcpy(sun.sun_path, argv[1]);
	sunlen = strlen(sun.sun_path) + 1 + sizeof (short);

	result = connect(sock, (struct sockaddr *) &sun, sunlen);
	if (result < 0) {
		perror("connect");
		close(sock);
		return -1;
	}

	result = write(sock, data, sendbytes);
	if (result < 0) {
		perror("write");
		close(sock);
		return -1;
	}

	printf("%s: sent %5d bytes\n", argv[0], result);
	close(sock);
	return (0);
}

Re: [RFC][PATCH] net/unix: support SCM_SECURITY for stream sockets

[Paul Moore]

On Tue, Jun 9, 2015 at 11:47 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> SCM_SECURITY was originally only implemented for datagram sockets,
> not for stream sockets.  However, SCM_CREDENTIALS is supported on
> Unix stream sockets.  For consistency, implement Unix stream support
> for SCM_SECURITY as well.  Also clean up the existing code and get
> rid of the superfluous UNIXSID macro.
>
> Motivated by https://bugzilla.redhat.com/show_bug.cgi?id=1224211,
> where systemd was using SCM_CREDENTIALS and assumed wrongly that
> SCM_SECURITY was also supported on Unix stream sockets.
>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>
> This is an RFC for security folks before I send this along to netdev.
> The patch is relative to net-next, not security-next.
>
>  include/net/af_unix.h |  1 -
>  net/unix/af_unix.c    | 20 ++++++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

Thanks Stephen, it looks reasonable to me, although I suspect you may
get some comments on the direct comparison in unix_secdata_eq() with
respect to LSM stacking.

> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index a175ba4..4a167b3 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -39,7 +39,6 @@ struct unix_skb_parms {
>  };
>
>  #define UNIXCB(skb)    (*(struct unix_skb_parms *)&((skb)->cb))
> -#define UNIXSID(skb)   (&UNIXCB((skb)).secid)
>
>  #define unix_state_lock(s)     spin_lock(&unix_sk(s)->lock)
>  #define unix_state_unlock(s)   spin_unlock(&unix_sk(s)->lock)
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index f25e167..03ee4d3 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -140,12 +140,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
>  #ifdef CONFIG_SECURITY_NETWORK
>  static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       memcpy(UNIXSID(skb), &scm->secid, sizeof(u32));
> +       UNIXCB(skb).secid = scm->secid;
>  }
>
>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       scm->secid = *UNIXSID(skb);
> +       scm->secid = UNIXCB(skb).secid;
> +}
> +
> +static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
> +{
> +       return (scm->secid == UNIXCB(skb).secid);
>  }
>  #else
>  static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> @@ -153,6 +158,11 @@ static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>
>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  { }
> +
> +static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
> +{
> +       return true;
> +}
>  #endif /* CONFIG_SECURITY_NETWORK */
>
>  /*
> @@ -1414,6 +1424,7 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
>         UNIXCB(skb).uid = scm->creds.uid;
>         UNIXCB(skb).gid = scm->creds.gid;
>         UNIXCB(skb).fp = NULL;
> +       unix_get_secdata(scm, skb);
>         if (scm->fp && send_fds)
>                 err = unix_attach_fds(scm, skb);
>
> @@ -1509,7 +1520,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>         if (err < 0)
>                 goto out_free;
>         max_level = err + 1;
> -       unix_get_secdata(&scm, skb);
>
>         skb_put(skb, len - data_len);
>         skb->data_len = data_len;
> @@ -2118,11 +2128,13 @@ unlock:
>                         /* Never glue messages from different writers */
>                         if ((UNIXCB(skb).pid  != scm.pid) ||
>                             !uid_eq(UNIXCB(skb).uid, scm.creds.uid) ||
> -                           !gid_eq(UNIXCB(skb).gid, scm.creds.gid))
> +                           !gid_eq(UNIXCB(skb).gid, scm.creds.gid) ||
> +                           !unix_secdata_eq(&scm, skb))
>                                 break;
>                 } else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
>                         /* Copy credentials */
>                         scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
> +                       unix_set_secdata(&scm, skb);
>                         check_creds = true;
>                 }
>
> --
> 2.1.0
>



-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] net/unix: support SCM_SECURITY for stream sockets

[Casey Schaufler]

On 6/9/2015 10:18 AM, Paul Moore wrote:
> On Tue, Jun 9, 2015 at 11:47 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> SCM_SECURITY was originally only implemented for datagram sockets,
>> not for stream sockets.  However, SCM_CREDENTIALS is supported on
>> Unix stream sockets.  For consistency, implement Unix stream support
>> for SCM_SECURITY as well.  Also clean up the existing code and get
>> rid of the superfluous UNIXSID macro.
>>
>> Motivated by https://bugzilla.redhat.com/show_bug.cgi?id=1224211,
>> where systemd was using SCM_CREDENTIALS and assumed wrongly that
>> SCM_SECURITY was also supported on Unix stream sockets.
>>
>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
>>
>> This is an RFC for security folks before I send this along to netdev.
>> The patch is relative to net-next, not security-next.
>>
>>  include/net/af_unix.h |  1 -
>>  net/unix/af_unix.c    | 20 ++++++++++++++++----
>>  2 files changed, 16 insertions(+), 5 deletions(-)
> Acked-by: Paul Moore <paul@paul-moore.com>
>
> Thanks Stephen, it looks reasonable to me, although I suspect you may
> get some comments on the direct comparison in unix_secdata_eq() with
> respect to LSM stacking.

No objections from stacking. There are enough other issues with
secids that this isn't even in the noise.

>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>> index a175ba4..4a167b3 100644
>> --- a/include/net/af_unix.h
>> +++ b/include/net/af_unix.h
>> @@ -39,7 +39,6 @@ struct unix_skb_parms {
>>  };
>>
>>  #define UNIXCB(skb)    (*(struct unix_skb_parms *)&((skb)->cb))
>> -#define UNIXSID(skb)   (&UNIXCB((skb)).secid)
>>
>>  #define unix_state_lock(s)     spin_lock(&unix_sk(s)->lock)
>>  #define unix_state_unlock(s)   spin_unlock(&unix_sk(s)->lock)
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index f25e167..03ee4d3 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -140,12 +140,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
>>  #ifdef CONFIG_SECURITY_NETWORK
>>  static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>>  {
>> -       memcpy(UNIXSID(skb), &scm->secid, sizeof(u32));
>> +       UNIXCB(skb).secid = scm->secid;
>>  }
>>
>>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>>  {
>> -       scm->secid = *UNIXSID(skb);
>> +       scm->secid = UNIXCB(skb).secid;
>> +}
>> +
>> +static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
>> +{
>> +       return (scm->secid == UNIXCB(skb).secid);
>>  }
>>  #else
>>  static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>> @@ -153,6 +158,11 @@ static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>>
>>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>>  { }
>> +
>> +static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
>> +{
>> +       return true;
>> +}
>>  #endif /* CONFIG_SECURITY_NETWORK */
>>
>>  /*
>> @@ -1414,6 +1424,7 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
>>         UNIXCB(skb).uid = scm->creds.uid;
>>         UNIXCB(skb).gid = scm->creds.gid;
>>         UNIXCB(skb).fp = NULL;
>> +       unix_get_secdata(scm, skb);
>>         if (scm->fp && send_fds)
>>                 err = unix_attach_fds(scm, skb);
>>
>> @@ -1509,7 +1520,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>>         if (err < 0)
>>                 goto out_free;
>>         max_level = err + 1;
>> -       unix_get_secdata(&scm, skb);
>>
>>         skb_put(skb, len - data_len);
>>         skb->data_len = data_len;
>> @@ -2118,11 +2128,13 @@ unlock:
>>                         /* Never glue messages from different writers */
>>                         if ((UNIXCB(skb).pid  != scm.pid) ||
>>                             !uid_eq(UNIXCB(skb).uid, scm.creds.uid) ||
>> -                           !gid_eq(UNIXCB(skb).gid, scm.creds.gid))
>> +                           !gid_eq(UNIXCB(skb).gid, scm.creds.gid) ||
>> +                           !unix_secdata_eq(&scm, skb))
>>                                 break;
>>                 } else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
>>                         /* Copy credentials */
>>                         scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
>> +                       unix_set_secdata(&scm, skb);
>>                         check_creds = true;
>>                 }
>>
>> --
>> 2.1.0
>>
>
>