[RFC,5/5] selinux: Add SCTP support

Commit Message

Richard Haines Oct. 17, 2017, 1:59 p.m. UTC

The SELinux SCTP implementation is explained in:
Documentation/security/SELinux-sctp.txt

Signed-off-by: Richard Haines <[e-mail]>
---
 Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
 security/selinux/hooks.c                | 268 ++++++++++++++++++++++++++++++--
 security/selinux/include/classmap.h     |   3 +-
 security/selinux/include/netlabel.h     |   9 +-
 security/selinux/include/objsec.h       |   5 +
 security/selinux/netlabel.c             |  52 ++++++-
 6 files changed, 427 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/security/SELinux-sctp.txt

Comments

Stephen Smalley Oct. 20, 2017, 7 p.m. UTC | #1

On Tue, 2017-10-17 at 14:59 +0100, Richard Haines wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.txt
> 
> Signed-off-by: Richard Haines <[e-mail]>
> ---
>  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
>  security/selinux/hooks.c                | 268
> ++++++++++++++++++++++++++++++--
>  security/selinux/include/classmap.h     |   3 +-
>  security/selinux/include/netlabel.h     |   9 +-
>  security/selinux/include/objsec.h       |   5 +
>  security/selinux/netlabel.c             |  52 ++++++-
>  6 files changed, 427 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/security/SELinux-sctp.txt
> 
> diff --git a/Documentation/security/SELinux-sctp.txt
> b/Documentation/security/SELinux-sctp.txt
> new file mode 100644
> index 0000000..32e0255
> --- /dev/null
> +++ b/Documentation/security/SELinux-sctp.txt
> @@ -0,0 +1,108 @@
> +                               SCTP SELinux Support
> +                              ======================
> +
> +Security Hooks
> +===============
> +
> +The Documentation/security/LSM-sctp.txt document describes how the
> following
> +sctp security hooks are utilised:
> +    security_sctp_assoc_request()
> +    security_sctp_bind_connect()
> +    security_sctp_sk_clone()
> +
> +    security_inet_conn_established()
> +
> +
> +Policy Statements
> +==================
> +The following class and permissions to support SCTP are available
> within the
> +kernel:
> +    class sctp_socket inherits socket { node_bind }
> +
> +whenever the following policy capability is enabled:
> +    policycap extended_socket_class;
> +
> +The SELinux SCTP support adds the additional permissions that are
> explained
> +in the sections below:
> +    association bindx connectx
> +
> +If userspace tools have been updated, SCTP will support the portcon
> +statement as shown in the following example:
> +    portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> +
> +
> +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> +================================================================
> +The hook security_sctp_bind_connect() is called by SCTP to check
> permissions
> +required for ipv4/ipv6 addresses based on the @optname as follows:
> +
> +  ------------------------------------------------------------------
> +  |                      BINDX Permission Check                    |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                  BIND Permission Checks                        |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6 address       |
> +  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                 CONNECTX Permission Check                      |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                 CONNECT Permission Checks                      |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6 address       |
> +  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses |
> +  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +SCTP Peer Labeling
> +===================
> +An SCTP socket will only have one peer label assigned to it. This
> will be
> +assigned during the establishment of the first association. Once the
> peer
> +label has been assigned, any new associations will have the
> "association"
> +permission validated by checking the socket peer sid against the
> received
> +packets peer sid to determine whether the association should be
> allowed or
> +denied.
> +
> +NOTES:
> +   1) If peer labeling is not enabled, then the peer context will
> always be
> +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> +
> +   2) As SCTP supports multiple endpoints with multi-homing on a
> single socket
> +      it is recommended that peer labels are consistent.
> +
> +   3) getpeercon(3) may be used by userspace to retrieve the sockets
> peer
> +       context.
> +
> +   4) If using NetLabel be aware that if a label is assigned to a
> specific
> +      interface, and that interface 'goes down', then the NetLabel
> service
> +      will remove the entry. Therefore ensure that the network
> startup scripts
> +      call netlabelctl(8) to set the required label (see netlabel-
> config(8)
> +      helper script for details).
> +
> +   5) The NetLabel SCTP peer labeling rules apply as discussed in
> the following
> +      set of posts tagged "netlabel" at: http://www.paul-moore.com/b
> log/t.
> +
> +   6) CIPSO is only supported for IPv4 addressing: socket(AF_INET,
> ...)
> +      CALIPSO is only supported for IPv6 addressing:
> socket(AF_INET6, ...)
> +
> +      Note the following when testing CIPSO/CALIPSO:
> +         a) CIPSO will send an ICMP packet if an SCTP packet cannot
> be
> +            delivered because of an invalid label.
> +         b) CALIPSO does not send an ICMP packet, just silently
> discards it.
> +
> +   7) IPSEC is not supported as rfc3554 - sctp/ipsec support has not
> been
> +      implemented in userspace (racoon(8) or ipsec_pluto(8)),
> although the
> +      kernel supports SCTP/IPSEC.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 33fd061..c3e9600 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -67,6 +67,8 @@
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
>  #include <linux/dccp.h>
> +#include <linux/sctp.h>
> +#include <net/sctp/structs.h>
>  #include <linux/quota.h>
>  #include <linux/un.h>		/* for Unix socket types */
>  #include <net/af_unix.h>	/* for Unix socket types */
> @@ -4119,6 +4121,23 @@ static int selinux_parse_skb_ipv4(struct
> sk_buff *skb,
>  		break;
>  	}
>  
> +#if IS_ENABLED(CONFIG_IP_SCTP)
> +	case IPPROTO_SCTP: {
> +		struct sctphdr _sctph, *sh;
> +
> +		if (ntohs(ih->frag_off) & IP_OFFSET)
> +			break;
> +
> +		offset += ihlen;
> +		sh = skb_header_pointer(skb, offset, sizeof(_sctph),
> &_sctph);
> +		if (sh == NULL)
> +			break;
> +
> +		ad->u.net->sport = sh->source;
> +		ad->u.net->dport = sh->dest;
> +		break;
> +	}
> +#endif
>  	default:
>  		break;
>  	}
> @@ -4192,6 +4211,19 @@ static int selinux_parse_skb_ipv6(struct
> sk_buff *skb,
>  		break;
>  	}
>  
> +#if IS_ENABLED(CONFIG_IP_SCTP)
> +	case IPPROTO_SCTP: {
> +		struct sctphdr _sctph, *sh;
> +
> +		sh = skb_header_pointer(skb, offset, sizeof(_sctph),
> &_sctph);
> +		if (sh == NULL)
> +			break;
> +
> +		ad->u.net->sport = sh->source;
> +		ad->u.net->dport = sh->dest;
> +		break;
> +	}
> +#endif
>  	/* includes fragments */
>  	default:
>  		break;
> @@ -4381,6 +4413,10 @@ static int selinux_socket_post_create(struct
> socket *sock, int family,
>  		sksec = sock->sk->sk_security;
>  		sksec->sclass = sclass;
>  		sksec->sid = sid;
> +		/* Allows detection of the first association on this
> socket */
> +		if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> +			sksec->sctp_assoc_state = SCTP_ASSOC_UNSET;
> +

What prevents this from interleaving with selinux_sctp_assoc_request()
accesses to sctp_assoc_state?

>  		err = selinux_netlbl_socket_post_create(sock->sk,
> family);
>  	}
>  
> @@ -4401,11 +4437,7 @@ static int selinux_socket_bind(struct socket
> *sock, struct sockaddr *address, in
>  	if (err)
>  		goto out;
>  
> -	/*
> -	 * If PF_INET or PF_INET6, check name_bind permission for
> the port.
> -	 * Multiple address binding for SCTP is not supported yet:
> we just
> -	 * check the first address now.
> -	 */
> +	/* If PF_INET or PF_INET6, check name_bind permission for
> the port. */
>  	family = sk->sk_family;
>  	if (family == PF_INET || family == PF_INET6) {
>  		char *addrp;
> @@ -4417,7 +4449,13 @@ static int selinux_socket_bind(struct socket
> *sock, struct sockaddr *address, in
>  		unsigned short snum;
>  		u32 sid, node_perm;
>  
> -		if (family == PF_INET) {
> +		/*
> +		 * sctp_bindx(3) calls via
> selinux_sctp_bind_connect()
> +		 * that validates multiple binding addresses.
> Because of this
> +		 * need to check address->sa_family as it is
> possible to have
> +		 * sk->sk_family = PF_INET6 with addr->sa_family =
> AF_INET.
> +		 */
> +		if (family == PF_INET || address->sa_family ==
> AF_INET) {
>  			if (addrlen < sizeof(struct sockaddr_in)) {
>  				err = -EINVAL;
>  				goto out;
> @@ -4471,6 +4509,10 @@ static int selinux_socket_bind(struct socket
> *sock, struct sockaddr *address, in
>  			node_perm = DCCP_SOCKET__NODE_BIND;
>  			break;
>  
> +		case SECCLASS_SCTP_SOCKET:
> +			node_perm = SCTP_SOCKET__NODE_BIND;
> +			break;
> +
>  		default:
>  			node_perm = RAWIP_SOCKET__NODE_BIND;
>  			break;
> @@ -4485,7 +4527,7 @@ static int selinux_socket_bind(struct socket
> *sock, struct sockaddr *address, in
>  		ad.u.net->sport = htons(snum);
>  		ad.u.net->family = family;
>  
> -		if (family == PF_INET)
> +		if (family == PF_INET || address->sa_family ==
> AF_INET)
>  			ad.u.net->v4info.saddr = addr4-
> >sin_addr.s_addr;
>  		else
>  			ad.u.net->v6info.saddr = addr6->sin6_addr;
> @@ -4510,10 +4552,12 @@ static int selinux_socket_connect(struct
> socket *sock, struct sockaddr *address,
>  		return err;
>  
>  	/*
> -	 * If a TCP or DCCP socket, check name_connect permission
> for the port.
> +	 * If a TCP, DCCP or SCTP socket, check name_connect
> permission
> +	 * for the port.
>  	 */
>  	if (sksec->sclass == SECCLASS_TCP_SOCKET ||
> -	    sksec->sclass == SECCLASS_DCCP_SOCKET) {
> +	    sksec->sclass == SECCLASS_DCCP_SOCKET ||
> +	    sksec->sclass == SECCLASS_SCTP_SOCKET) {
>  		struct common_audit_data ad;
>  		struct lsm_network_audit net = {0,};
>  		struct sockaddr_in *addr4 = NULL;
> @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct
> socket *sock, struct sockaddr *address,
>  		unsigned short snum;
>  		u32 sid, perm;
>  
> -		if (sk->sk_family == PF_INET) {
> +		/* sctp_connectx(3) calls via
> +		 *selinux_sctp_bind_connect() that validates
> multiple
> +		 * connect addresses. Because of this need to check
> +		 * address->sa_family as it is possible to have
> +		 * sk->sk_family = PF_INET6 with addr->sa_family =
> AF_INET.
> +		 */
> +		if (sk->sk_family == PF_INET ||
> +					address->sa_family ==
> AF_INET) {
>  			addr4 = (struct sockaddr_in *)address;
>  			if (addrlen < sizeof(struct sockaddr_in))
>  				return -EINVAL;
> @@ -4534,11 +4585,21 @@ static int selinux_socket_connect(struct
> socket *sock, struct sockaddr *address,
>  		}
>  
>  		err = sel_netport_sid(sk->sk_protocol, snum, &sid);
> +
>  		if (err)
>  			goto out;
>  
> -		perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
> -		       TCP_SOCKET__NAME_CONNECT :
> DCCP_SOCKET__NAME_CONNECT;
> +		switch (sksec->sclass) {
> +		case SECCLASS_TCP_SOCKET:
> +			perm = TCP_SOCKET__NAME_CONNECT;
> +			break;
> +		case SECCLASS_DCCP_SOCKET:
> +			perm = DCCP_SOCKET__NAME_CONNECT;
> +			break;
> +		case SECCLASS_SCTP_SOCKET:
> +			perm = SCTP_SOCKET__NAME_CONNECT;
> +			break;
> +		}
>  
>  		ad.type = LSM_AUDIT_DATA_NET;
>  		ad.u.net = &net;
> @@ -4815,7 +4876,8 @@ static int
> selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
>  	u32 peer_sid = SECSID_NULL;
>  
>  	if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
> -	    sksec->sclass == SECCLASS_TCP_SOCKET)
> +	    sksec->sclass == SECCLASS_TCP_SOCKET ||
> +	    sksec->sclass == SECCLASS_SCTP_SOCKET)
>  		peer_sid = sksec->peer_sid;
>  	if (peer_sid == SECSID_NULL)
>  		return -ENOPROTOOPT;
> @@ -4928,6 +4990,183 @@ static void selinux_sock_graft(struct sock
> *sk, struct socket *parent)
>  	sksec->sclass = isec->sclass;
>  }
>  
> +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
> +				      struct sk_buff *skb,
> +				      int sctp_cid)
> +{
> +	struct sk_security_struct *sksec = ep->base.sk->sk_security;
> +	struct common_audit_data ad;
> +	struct lsm_network_audit net = {0,};
> +	u8 peerlbl_active;
> +	u32 peer_sid = SECINITSID_UNLABELED;
> +	u32 conn_sid;
> +	int err;
> +
> +	if (!selinux_policycap_extsockclass)
> +		return 0;
> +
> +	peerlbl_active = selinux_peerlbl_enabled();
> +
> +	if (peerlbl_active) {
> +		/* This will return peer_sid = SECSID_NULL if there
> are
> +		 * no peer labels, see
> security_net_peersid_resolve().
> +		 */
> +		err = selinux_skb_peerlbl_sid(skb, ep->base.sk-
> >sk_family,
> +					      &peer_sid);
> +
> +		if (err)
> +			return err;
> +
> +		if (peer_sid == SECSID_NULL)
> +			peer_sid = SECINITSID_UNLABELED;
> +	}
> +
> +	if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> +		sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> +
> +		/* Here as first association on socket. As the peer
> SID
> +		 * was allowed by peer recv (and the netif/node
> checks),
> +		 * then it is approved by policy and used as the
> primary
> +		 * peer SID for getpeercon(3).
> +		 */
> +		sksec->peer_sid = peer_sid;
> +	} else if  (sksec->peer_sid != peer_sid) {
> +		/* Other association peer SIDs are checked to
> enforce
> +		 * consistency among the peer SIDs.
> +		 */
> +		ad.type = LSM_AUDIT_DATA_NET;
> +		ad.u.net = &net;
> +		ad.u.net->sk = ep->base.sk;
> +		err = avc_has_perm(sksec->peer_sid, peer_sid, sksec-
> >sclass,
> +				   SCTP_SOCKET__ASSOCIATION, &ad);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (sctp_cid == SCTP_CID_INIT) {
> +		/* Have INIT when incoming connect(2),
> sctp_connectx(3)
> +		 * or sctp_sendmsg(3) (with no association already
> present),
> +		 * so compute the MLS component for the connection
> and store
> +		 * the information in ep. This will be used by SCTP
> TCP type
> +		 * sockets and peeled off connections as they cause
> a new
> +		 * socket to be generated. selinux_sctp_sk_clone()
> will then
> +		 * plug this into the new socket.
> +		 */
> +		err = selinux_conn_sid(sksec->sid, peer_sid,
> &conn_sid);
> +		if (err)
> +			return err;
> +
> +		ep->secid = conn_sid;
> +		ep->peer_secid = peer_sid;
> +
> +		/* Set any NetLabel labels including CIPSO/CALIPSO
> options. */
> +		return selinux_netlbl_sctp_assoc_request(ep, skb);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if sctp IPv4/IPv6 addresses are valid for binding or
> connecting
> + * based on their @optname.
> + */
> +static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> +				     struct sockaddr *address,
> +				     int addrlen)
> +{
> +	int len, err = 0, walk_size = 0;
> +	void *addr_buf;
> +	struct sockaddr *addr;
> +	struct socket *sock;
> +
> +	if (!selinux_policycap_extsockclass)
> +		return 0;
> +
> +	switch (optname) {
> +	case SCTP_SOCKOPT_BINDX_ADD:
> +		err = sock_has_perm(sk, SCTP_SOCKET__BINDX);
> +		break;
> +	case SCTP_SOCKOPT_CONNECTX:
> +		err = sock_has_perm(sk, SCTP_SOCKET__CONNECTX);
> +		break;
> +	/* These need SOCKET__BIND or SOCKET__CONNECT permissions
> that will
> +	 * be checked later.
> +	 */
> +	case SCTP_PRIMARY_ADDR:
> +	case SCTP_SET_PEER_PRIMARY_ADDR:
> +	case SCTP_PARAM_SET_PRIMARY:
> +	case SCTP_PARAM_ADD_IP:
> +	case SCTP_SENDMSG_CONNECT:
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +	if (err)
> +		return err;
> +
> +	/* Process one or more addresses that may be IPv4 or IPv6 */
> +	sock = sk->sk_socket;
> +	addr_buf = address;
> +
> +	while (walk_size < addrlen) {
> +		addr = addr_buf;
> +		switch (addr->sa_family) {
> +		case AF_INET:
> +			len = sizeof(struct sockaddr_in);
> +			break;
> +		case AF_INET6:
> +			len = sizeof(struct sockaddr_in6);
> +			break;
> +		default:
> +			return -EAFNOSUPPORT;
> +		}
> +
> +		err = -EINVAL;
> +		switch (optname) {
> +		/* Bind checks */
> +		case SCTP_PRIMARY_ADDR:
> +		case SCTP_SET_PEER_PRIMARY_ADDR:
> +		case SCTP_SOCKOPT_BINDX_ADD:
> +			err = selinux_socket_bind(sock, addr, len);
> +			break;
> +		/* Connect checks */
> +		case SCTP_SOCKOPT_CONNECTX:
> +		case SCTP_PARAM_SET_PRIMARY:
> +		case SCTP_PARAM_ADD_IP:
> +		case SCTP_SENDMSG_CONNECT:
> +			err = selinux_socket_connect(sock, addr,
> len);
> +			break;
> +		}
> +
> +		if (err)
> +			return err;
> +
> +		addr_buf += len;
> +		walk_size += len;
> +	}
> +	return 0;
> +}
> +
> +/* Called whenever a new socket is created by accept(2) or
> sctp_peeloff(3). */
> +static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct
> sock *sk,
> +				  struct sock *newsk)
> +{
> +	struct sk_security_struct *sksec = sk->sk_security;
> +	struct sk_security_struct *newsksec = newsk->sk_security;
> +
> +	/* If policy does not support SECCLASS_SCTP_SOCKET then call
> +	 * the non-sctp clone version.
> +	 */
> +	if (!selinux_policycap_extsockclass)
> +		return selinux_sk_clone_security(sk, newsk);
> +
> +	newsksec->sid = ep->secid;
> +	newsksec->peer_sid = ep->peer_secid;
> +	newsksec->sclass = sksec->sclass;
> +	newsksec->nlbl_state = sksec->nlbl_state;
> +}
> +
>  static int selinux_inet_conn_request(struct sock *sk, struct sk_buff
> *skb,
>  				     struct request_sock *req)
>  {
> @@ -6416,6 +6655,9 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
>  	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
>  	LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
> +	LSM_HOOK_INIT(sctp_assoc_request,
> selinux_sctp_assoc_request),
> +	LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
> +	LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect),
>  	LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request),
>  	LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
>  	LSM_HOOK_INIT(inet_conn_established,
> selinux_inet_conn_established),
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index b9fe343..b4b10da 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -173,7 +173,8 @@ struct security_class_mapping secclass_map[] = {
>  	  { COMMON_CAP2_PERMS, NULL } },
>  	{ "sctp_socket",
>  	  { COMMON_SOCK_PERMS,
> -	    "node_bind", NULL } },
> +	    "node_bind", "name_connect", "association", "bindx",
> +	    "connectx", NULL } },
>  	{ "icmp_socket",
>  	  { COMMON_SOCK_PERMS,
>  	    "node_bind", NULL } },
> diff --git a/security/selinux/include/netlabel.h
> b/security/selinux/include/netlabel.h
> index 75686d5..835a0d6 100644
> --- a/security/selinux/include/netlabel.h
> +++ b/security/selinux/include/netlabel.h
> @@ -33,6 +33,7 @@
>  #include <linux/skbuff.h>
>  #include <net/sock.h>
>  #include <net/request_sock.h>
> +#include <net/sctp/structs.h>
>  
>  #include "avc.h"
>  #include "objsec.h"
> @@ -53,7 +54,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff
> *skb,
>  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
>  				 u16 family,
>  				 u32 sid);
> -
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +				     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);
> @@ -114,6 +116,11 @@ static inline int
> selinux_netlbl_conn_setsid(struct sock *sk,
>  	return 0;
>  }
>  
> +static inline int selinux_netlbl_sctp_assoc_request(struct
> sctp_endpoint *ep,
> +						    struct sk_buff
> *skb)
> +{
> +	return 0;
> +}
>  static inline int selinux_netlbl_inet_conn_request(struct
> request_sock *req,
>  						   u16 family)
>  {
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 6ebc61e..660f270 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -130,6 +130,11 @@ struct sk_security_struct {
>  	u32 sid;			/* SID of this object */
>  	u32 peer_sid;			/* SID of peer */
>  	u16 sclass;			/* sock security class */
> +
> +	enum {				/* SCTP association
> state */
> +		SCTP_ASSOC_UNSET = 0,
> +		SCTP_ASSOC_SET,
> +	} sctp_assoc_state;
>  };
>  
>  struct tun_security_struct {
> diff --git a/security/selinux/netlabel.c
> b/security/selinux/netlabel.c
> index aaba667..7d5aa15 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -250,6 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff
> *skb,
>  	sk = skb_to_full_sk(skb);
>  	if (sk != NULL) {
>  		struct sk_security_struct *sksec = sk->sk_security;
> +
>  		if (sksec->nlbl_state != NLBL_REQSKB)
>  			return 0;
>  		secattr = selinux_netlbl_sock_getattr(sk, sid);
> @@ -271,6 +272,41 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff
> *skb,
>  }
>  
>  /**
> + * selinux_netlbl_sctp_assoc_request - Label an incoming sctp
> association.
> + * @ep: incoming association endpoint.
> + * @skb: the packet.
> + *
> + * Description:
> + * A new incoming connection is represented by @ep, ......
> + * Returns zero on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +				     struct sk_buff *skb)
> +{
> +	int rc;
> +	struct netlbl_lsm_secattr secattr;
> +	struct sk_security_struct *sksec = ep->base.sk->sk_security;
> +
> +	if (ep->base.sk->sk_family != PF_INET &&
> +				ep->base.sk->sk_family != PF_INET6)
> +		return 0;
> +
> +	netlbl_secattr_init(&secattr);
> +	rc = security_netlbl_sid_to_secattr(ep->secid, &secattr);
> +	if (rc != 0)
> +		goto assoc_request_return;
> +
> +	rc = netlbl_sctp_setattr(ep->base.sk, skb, &secattr);
> +	if (rc == 0)
> +		sksec->nlbl_state = NLBL_LABELED;
> +
> +assoc_request_return:
> +	netlbl_secattr_destroy(&secattr);
> +	return rc;
> +}
> +
> +/**
>   * selinux_netlbl_inet_conn_request - Label an incoming stream
> connection
>   * @req: incoming connection request socket
>   *
> @@ -481,7 +517,7 @@ int selinux_netlbl_socket_setsockopt(struct
> socket *sock,
>   */
>  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr
> *addr)
>  {
> -	int rc;
> +	int rc, already_owned_by_user = 0;
>  	struct sk_security_struct *sksec = sk->sk_security;
>  	struct netlbl_lsm_secattr *secattr;
>  
> @@ -489,7 +525,16 @@ int selinux_netlbl_socket_connect(struct sock
> *sk, struct sockaddr *addr)
>  	    sksec->nlbl_state != NLBL_CONNLABELED)
>  		return 0;
>  
> -	lock_sock(sk);
> +	/* Note: When called via connect(2) this happens before the
> socket
> +	 * protocol layer connect operation and @sk is not locked,
> HOWEVER,
> +	 * when called by the SCTP protocol layer via
> sctp_connectx(3),
> +	 * sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore
> check if
> +	 * @sk owned already.
> +	 */
> +	if (sock_owned_by_user(sk) && sksec->sclass ==
> SECCLASS_SCTP_SOCKET)
> +		already_owned_by_user = 1;
> +	else
> +		lock_sock(sk);

Conditional locking is generally considered harmful.  I'd split the
cases for the different callers, and use a common helper for both.

>  
>  	/* connected sockets are allowed to disconnect when the
> address family
>  	 * is set to AF_UNSPEC, if that is what is happening we want
> to reset
> @@ -510,6 +555,7 @@ int selinux_netlbl_socket_connect(struct sock
> *sk, struct sockaddr *addr)
>  		sksec->nlbl_state = NLBL_CONNLABELED;
>  
>  socket_connect_return:
> -	release_sock(sk);
> +	if (!already_owned_by_user)
> +		release_sock(sk);
>  	return rc;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Richard Haines Oct. 24, 2017, 3:50 p.m. UTC | #2

On Fri, 2017-10-20 at 15:00 -0400, Stephen Smalley wrote:
> On Tue, 2017-10-17 at 14:59 +0100, Richard Haines wrote:
> > The SELinux SCTP implementation is explained in:
> > Documentation/security/SELinux-sctp.txt
> > 
> > Signed-off-by: Richard Haines <[e-mail]>
> > ---
> >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
> >  security/selinux/hooks.c                | 268
> > ++++++++++++++++++++++++++++++--
> >  security/selinux/include/classmap.h     |   3 +-
> >  security/selinux/include/netlabel.h     |   9 +-
> >  security/selinux/include/objsec.h       |   5 +
> >  security/selinux/netlabel.c             |  52 ++++++-
> >  6 files changed, 427 insertions(+), 18 deletions(-)
> >  create mode 100644 Documentation/security/SELinux-sctp.txt
> > 
> > diff --git a/Documentation/security/SELinux-sctp.txt
> > b/Documentation/security/SELinux-sctp.txt
> > new file mode 100644
> > index 0000000..32e0255
> > --- /dev/null
> > +++ b/Documentation/security/SELinux-sctp.txt
> > @@ -0,0 +1,108 @@
> > +                               SCTP SELinux Support
> > +                              ======================
> > +
> > +Security Hooks
> > +===============
> > +
> > +The Documentation/security/LSM-sctp.txt document describes how the
> > following
> > +sctp security hooks are utilised:
> > +    security_sctp_assoc_request()
> > +    security_sctp_bind_connect()
> > +    security_sctp_sk_clone()
> > +
> > +    security_inet_conn_established()
> > +
> > +
> > +Policy Statements
> > +==================
> > +The following class and permissions to support SCTP are available
> > within the
> > +kernel:
> > +    class sctp_socket inherits socket { node_bind }
> > +
> > +whenever the following policy capability is enabled:
> > +    policycap extended_socket_class;
> > +
> > +The SELinux SCTP support adds the additional permissions that are
> > explained
> > +in the sections below:
> > +    association bindx connectx
> > +
> > +If userspace tools have been updated, SCTP will support the
> > portcon
> > +statement as shown in the following example:
> > +    portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> > +
> > +
> > +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> > +================================================================
> > +The hook security_sctp_bind_connect() is called by SCTP to check
> > permissions
> > +required for ipv4/ipv6 addresses based on the @optname as follows:
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                      BINDX Permission
> > Check                    |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses
> > |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                  BIND Permission
> > Checks                        |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6
> > address       |
> > +  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6
> > address       |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                 CONNECTX Permission
> > Check                      |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses
> > |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                 CONNECT Permission
> > Checks                      |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6
> > address       |
> > +  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses
> > |
> > +  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6
> > address       |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +SCTP Peer Labeling
> > +===================
> > +An SCTP socket will only have one peer label assigned to it. This
> > will be
> > +assigned during the establishment of the first association. Once
> > the
> > peer
> > +label has been assigned, any new associations will have the
> > "association"
> > +permission validated by checking the socket peer sid against the
> > received
> > +packets peer sid to determine whether the association should be
> > allowed or
> > +denied.
> > +
> > +NOTES:
> > +   1) If peer labeling is not enabled, then the peer context will
> > always be
> > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> > +
> > +   2) As SCTP supports multiple endpoints with multi-homing on a
> > single socket
> > +      it is recommended that peer labels are consistent.
> > +
> > +   3) getpeercon(3) may be used by userspace to retrieve the
> > sockets
> > peer
> > +       context.
> > +
> > +   4) If using NetLabel be aware that if a label is assigned to a
> > specific
> > +      interface, and that interface 'goes down', then the NetLabel
> > service
> > +      will remove the entry. Therefore ensure that the network
> > startup scripts
> > +      call netlabelctl(8) to set the required label (see netlabel-
> > config(8)
> > +      helper script for details).
> > +
> > +   5) The NetLabel SCTP peer labeling rules apply as discussed in
> > the following
> > +      set of posts tagged "netlabel" at: http://www.paul-moore.com
> > /b
> > log/t.
> > +
> > +   6) CIPSO is only supported for IPv4 addressing: socket(AF_INET,
> > ...)
> > +      CALIPSO is only supported for IPv6 addressing:
> > socket(AF_INET6, ...)
> > +
> > +      Note the following when testing CIPSO/CALIPSO:
> > +         a) CIPSO will send an ICMP packet if an SCTP packet
> > cannot
> > be
> > +            delivered because of an invalid label.
> > +         b) CALIPSO does not send an ICMP packet, just silently
> > discards it.
> > +
> > +   7) IPSEC is not supported as rfc3554 - sctp/ipsec support has
> > not
> > been
> > +      implemented in userspace (racoon(8) or ipsec_pluto(8)),
> > although the
> > +      kernel supports SCTP/IPSEC.
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 33fd061..c3e9600 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -67,6 +67,8 @@
> >  #include <linux/tcp.h>
> >  #include <linux/udp.h>
> >  #include <linux/dccp.h>
> > +#include <linux/sctp.h>
> > +#include <net/sctp/structs.h>
> >  #include <linux/quota.h>
> >  #include <linux/un.h>		/* for Unix socket types */
> >  #include <net/af_unix.h>	/* for Unix socket types */
> > @@ -4119,6 +4121,23 @@ static int selinux_parse_skb_ipv4(struct
> > sk_buff *skb,
> >  		break;
> >  	}
> >  
> > +#if IS_ENABLED(CONFIG_IP_SCTP)
> > +	case IPPROTO_SCTP: {
> > +		struct sctphdr _sctph, *sh;
> > +
> > +		if (ntohs(ih->frag_off) & IP_OFFSET)
> > +			break;
> > +
> > +		offset += ihlen;
> > +		sh = skb_header_pointer(skb, offset,
> > sizeof(_sctph),
> > &_sctph);
> > +		if (sh == NULL)
> > +			break;
> > +
> > +		ad->u.net->sport = sh->source;
> > +		ad->u.net->dport = sh->dest;
> > +		break;
> > +	}
> > +#endif
> >  	default:
> >  		break;
> >  	}
> > @@ -4192,6 +4211,19 @@ static int selinux_parse_skb_ipv6(struct
> > sk_buff *skb,
> >  		break;
> >  	}
> >  
> > +#if IS_ENABLED(CONFIG_IP_SCTP)
> > +	case IPPROTO_SCTP: {
> > +		struct sctphdr _sctph, *sh;
> > +
> > +		sh = skb_header_pointer(skb, offset,
> > sizeof(_sctph),
> > &_sctph);
> > +		if (sh == NULL)
> > +			break;
> > +
> > +		ad->u.net->sport = sh->source;
> > +		ad->u.net->dport = sh->dest;
> > +		break;
> > +	}
> > +#endif
> >  	/* includes fragments */
> >  	default:
> >  		break;
> > @@ -4381,6 +4413,10 @@ static int selinux_socket_post_create(struct
> > socket *sock, int family,
> >  		sksec = sock->sk->sk_security;
> >  		sksec->sclass = sclass;
> >  		sksec->sid = sid;
> > +		/* Allows detection of the first association on
> > this
> > socket */
> > +		if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> > +			sksec->sctp_assoc_state =
> > SCTP_ASSOC_UNSET;
> > +
> 
> What prevents this from interleaving with
> selinux_sctp_assoc_request()
> accesses to sctp_assoc_state?

I've added a spinlock in selinux_sctp_assoc_request()

> 
> >  		err = selinux_netlbl_socket_post_create(sock->sk,
> > family);
> >  	}
> >  
> > @@ -4401,11 +4437,7 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >  	if (err)
> >  		goto out;
> >  
> > -	/*
> > -	 * If PF_INET or PF_INET6, check name_bind permission for
> > the port.
> > -	 * Multiple address binding for SCTP is not supported yet:
> > we just
> > -	 * check the first address now.
> > -	 */
> > +	/* If PF_INET or PF_INET6, check name_bind permission for
> > the port. */
> >  	family = sk->sk_family;
> >  	if (family == PF_INET || family == PF_INET6) {
> >  		char *addrp;
> > @@ -4417,7 +4449,13 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >  		unsigned short snum;
> >  		u32 sid, node_perm;
> >  
> > -		if (family == PF_INET) {
> > +		/*
> > +		 * sctp_bindx(3) calls via
> > selinux_sctp_bind_connect()
> > +		 * that validates multiple binding addresses.
> > Because of this
> > +		 * need to check address->sa_family as it is
> > possible to have
> > +		 * sk->sk_family = PF_INET6 with addr->sa_family =
> > AF_INET.
> > +		 */
> > +		if (family == PF_INET || address->sa_family ==
> > AF_INET) {
> >  			if (addrlen < sizeof(struct sockaddr_in))
> > {
> >  				err = -EINVAL;
> >  				goto out;
> > @@ -4471,6 +4509,10 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >  			node_perm = DCCP_SOCKET__NODE_BIND;
> >  			break;
> >  
> > +		case SECCLASS_SCTP_SOCKET:
> > +			node_perm = SCTP_SOCKET__NODE_BIND;
> > +			break;
> > +
> >  		default:
> >  			node_perm = RAWIP_SOCKET__NODE_BIND;
> >  			break;
> > @@ -4485,7 +4527,7 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >  		ad.u.net->sport = htons(snum);
> >  		ad.u.net->family = family;
> >  
> > -		if (family == PF_INET)
> > +		if (family == PF_INET || address->sa_family ==
> > AF_INET)
> >  			ad.u.net->v4info.saddr = addr4-
> > > sin_addr.s_addr;
> > 
> >  		else
> >  			ad.u.net->v6info.saddr = addr6->sin6_addr;
> > @@ -4510,10 +4552,12 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >  		return err;
> >  
> >  	/*
> > -	 * If a TCP or DCCP socket, check name_connect permission
> > for the port.
> > +	 * If a TCP, DCCP or SCTP socket, check name_connect
> > permission
> > +	 * for the port.
> >  	 */
> >  	if (sksec->sclass == SECCLASS_TCP_SOCKET ||
> > -	    sksec->sclass == SECCLASS_DCCP_SOCKET) {
> > +	    sksec->sclass == SECCLASS_DCCP_SOCKET ||
> > +	    sksec->sclass == SECCLASS_SCTP_SOCKET) {
> >  		struct common_audit_data ad;
> >  		struct lsm_network_audit net = {0,};
> >  		struct sockaddr_in *addr4 = NULL;
> > @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >  		unsigned short snum;
> >  		u32 sid, perm;
> >  
> > -		if (sk->sk_family == PF_INET) {
> > +		/* sctp_connectx(3) calls via
> > +		 *selinux_sctp_bind_connect() that validates
> > multiple
> > +		 * connect addresses. Because of this need to
> > check
> > +		 * address->sa_family as it is possible to have
> > +		 * sk->sk_family = PF_INET6 with addr->sa_family =
> > AF_INET.
> > +		 */
> > +		if (sk->sk_family == PF_INET ||
> > +					address->sa_family ==
> > AF_INET) {
> >  			addr4 = (struct sockaddr_in *)address;
> >  			if (addrlen < sizeof(struct sockaddr_in))
> >  				return -EINVAL;
> > @@ -4534,11 +4585,21 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >  		}
> >  
> >  		err = sel_netport_sid(sk->sk_protocol, snum,
> > &sid);
> > +
> >  		if (err)
> >  			goto out;
> >  
> > -		perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
> > -		       TCP_SOCKET__NAME_CONNECT :
> > DCCP_SOCKET__NAME_CONNECT;
> > +		switch (sksec->sclass) {
> > +		case SECCLASS_TCP_SOCKET:
> > +			perm = TCP_SOCKET__NAME_CONNECT;
> > +			break;
> > +		case SECCLASS_DCCP_SOCKET:
> > +			perm = DCCP_SOCKET__NAME_CONNECT;
> > +			break;
> > +		case SECCLASS_SCTP_SOCKET:
> > +			perm = SCTP_SOCKET__NAME_CONNECT;
> > +			break;
> > +		}
> >  
> >  		ad.type = LSM_AUDIT_DATA_NET;
> >  		ad.u.net = &net;
> > @@ -4815,7 +4876,8 @@ static int
> > selinux_socket_getpeersec_stream(struct socket *sock, char __user
> > *op
> >  	u32 peer_sid = SECSID_NULL;
> >  
> >  	if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
> > -	    sksec->sclass == SECCLASS_TCP_SOCKET)
> > +	    sksec->sclass == SECCLASS_TCP_SOCKET ||
> > +	    sksec->sclass == SECCLASS_SCTP_SOCKET)
> >  		peer_sid = sksec->peer_sid;
> >  	if (peer_sid == SECSID_NULL)
> >  		return -ENOPROTOOPT;
> > @@ -4928,6 +4990,183 @@ static void selinux_sock_graft(struct sock
> > *sk, struct socket *parent)
> >  	sksec->sclass = isec->sclass;
> >  }
> >  
> > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> > +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
> > +				      struct sk_buff *skb,
> > +				      int sctp_cid)
> > +{
> > +	struct sk_security_struct *sksec = ep->base.sk-
> > >sk_security;
> > +	struct common_audit_data ad;
> > +	struct lsm_network_audit net = {0,};
> > +	u8 peerlbl_active;
> > +	u32 peer_sid = SECINITSID_UNLABELED;
> > +	u32 conn_sid;
> > +	int err;
> > +
> > +	if (!selinux_policycap_extsockclass)
> > +		return 0;
> > +
> > +	peerlbl_active = selinux_peerlbl_enabled();
> > +
> > +	if (peerlbl_active) {
> > +		/* This will return peer_sid = SECSID_NULL if
> > there
> > are
> > +		 * no peer labels, see
> > security_net_peersid_resolve().
> > +		 */
> > +		err = selinux_skb_peerlbl_sid(skb, ep->base.sk-
> > > sk_family,
> > 
> > +					      &peer_sid);
> > +
> > +		if (err)
> > +			return err;
> > +
> > +		if (peer_sid == SECSID_NULL)
> > +			peer_sid = SECINITSID_UNLABELED;
> > +	}
> > +
> > +	if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> > +		sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> > +
> > +		/* Here as first association on socket. As the
> > peer
> > SID
> > +		 * was allowed by peer recv (and the netif/node
> > checks),
> > +		 * then it is approved by policy and used as the
> > primary
> > +		 * peer SID for getpeercon(3).
> > +		 */
> > +		sksec->peer_sid = peer_sid;
> > +	} else if  (sksec->peer_sid != peer_sid) {
> > +		/* Other association peer SIDs are checked to
> > enforce
> > +		 * consistency among the peer SIDs.
> > +		 */
> > +		ad.type = LSM_AUDIT_DATA_NET;
> > +		ad.u.net = &net;
> > +		ad.u.net->sk = ep->base.sk;
> > +		err = avc_has_perm(sksec->peer_sid, peer_sid,
> > sksec-
> > > sclass,
> > 
> > +				   SCTP_SOCKET__ASSOCIATION, &ad);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	if (sctp_cid == SCTP_CID_INIT) {
> > +		/* Have INIT when incoming connect(2),
> > sctp_connectx(3)
> > +		 * or sctp_sendmsg(3) (with no association already
> > present),
> > +		 * so compute the MLS component for the connection
> > and store
> > +		 * the information in ep. This will be used by
> > SCTP
> > TCP type
> > +		 * sockets and peeled off connections as they
> > cause
> > a new
> > +		 * socket to be generated. selinux_sctp_sk_clone()
> > will then
> > +		 * plug this into the new socket.
> > +		 */
> > +		err = selinux_conn_sid(sksec->sid, peer_sid,
> > &conn_sid);
> > +		if (err)
> > +			return err;
> > +
> > +		ep->secid = conn_sid;
> > +		ep->peer_secid = peer_sid;
> > +
> > +		/* Set any NetLabel labels including CIPSO/CALIPSO
> > options. */
> > +		return selinux_netlbl_sctp_assoc_request(ep, skb);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check if sctp IPv4/IPv6 addresses are valid for binding or
> > connecting
> > + * based on their @optname.
> > + */
> > +static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> > +				     struct sockaddr *address,
> > +				     int addrlen)
> > +{
> > +	int len, err = 0, walk_size = 0;
> > +	void *addr_buf;
> > +	struct sockaddr *addr;
> > +	struct socket *sock;
> > +
> > +	if (!selinux_policycap_extsockclass)
> > +		return 0;
> > +
> > +	switch (optname) {
> > +	case SCTP_SOCKOPT_BINDX_ADD:
> > +		err = sock_has_perm(sk, SCTP_SOCKET__BINDX);
> > +		break;
> > +	case SCTP_SOCKOPT_CONNECTX:
> > +		err = sock_has_perm(sk, SCTP_SOCKET__CONNECTX);
> > +		break;
> > +	/* These need SOCKET__BIND or SOCKET__CONNECT permissions
> > that will
> > +	 * be checked later.
> > +	 */
> > +	case SCTP_PRIMARY_ADDR:
> > +	case SCTP_SET_PEER_PRIMARY_ADDR:
> > +	case SCTP_PARAM_SET_PRIMARY:
> > +	case SCTP_PARAM_ADD_IP:
> > +	case SCTP_SENDMSG_CONNECT:
> > +		break;
> > +	default:
> > +		err = -EINVAL;
> > +	}
> > +	if (err)
> > +		return err;
> > +
> > +	/* Process one or more addresses that may be IPv4 or IPv6
> > */
> > +	sock = sk->sk_socket;
> > +	addr_buf = address;
> > +
> > +	while (walk_size < addrlen) {
> > +		addr = addr_buf;
> > +		switch (addr->sa_family) {
> > +		case AF_INET:
> > +			len = sizeof(struct sockaddr_in);
> > +			break;
> > +		case AF_INET6:
> > +			len = sizeof(struct sockaddr_in6);
> > +			break;
> > +		default:
> > +			return -EAFNOSUPPORT;
> > +		}
> > +
> > +		err = -EINVAL;
> > +		switch (optname) {
> > +		/* Bind checks */
> > +		case SCTP_PRIMARY_ADDR:
> > +		case SCTP_SET_PEER_PRIMARY_ADDR:
> > +		case SCTP_SOCKOPT_BINDX_ADD:
> > +			err = selinux_socket_bind(sock, addr,
> > len);
> > +			break;
> > +		/* Connect checks */
> > +		case SCTP_SOCKOPT_CONNECTX:
> > +		case SCTP_PARAM_SET_PRIMARY:
> > +		case SCTP_PARAM_ADD_IP:
> > +		case SCTP_SENDMSG_CONNECT:
> > +			err = selinux_socket_connect(sock, addr,
> > len);
> > +			break;
> > +		}
> > +
> > +		if (err)
> > +			return err;
> > +
> > +		addr_buf += len;
> > +		walk_size += len;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Called whenever a new socket is created by accept(2) or
> > sctp_peeloff(3). */
> > +static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct
> > sock *sk,
> > +				  struct sock *newsk)
> > +{
> > +	struct sk_security_struct *sksec = sk->sk_security;
> > +	struct sk_security_struct *newsksec = newsk->sk_security;
> > +
> > +	/* If policy does not support SECCLASS_SCTP_SOCKET then
> > call
> > +	 * the non-sctp clone version.
> > +	 */
> > +	if (!selinux_policycap_extsockclass)
> > +		return selinux_sk_clone_security(sk, newsk);
> > +
> > +	newsksec->sid = ep->secid;
> > +	newsksec->peer_sid = ep->peer_secid;
> > +	newsksec->sclass = sksec->sclass;
> > +	newsksec->nlbl_state = sksec->nlbl_state;
> > +}
> > +
> >  static int selinux_inet_conn_request(struct sock *sk, struct
> > sk_buff
> > *skb,
> >  				     struct request_sock *req)
> >  {
> > @@ -6416,6 +6655,9 @@ static struct security_hook_list
> > selinux_hooks[] __lsm_ro_after_init = {
> >  	LSM_HOOK_INIT(sk_clone_security,
> > selinux_sk_clone_security),
> >  	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
> >  	LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
> > +	LSM_HOOK_INIT(sctp_assoc_request,
> > selinux_sctp_assoc_request),
> > +	LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
> > +	LSM_HOOK_INIT(sctp_bind_connect,
> > selinux_sctp_bind_connect),
> >  	LSM_HOOK_INIT(inet_conn_request,
> > selinux_inet_conn_request),
> >  	LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
> >  	LSM_HOOK_INIT(inet_conn_established,
> > selinux_inet_conn_established),
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index b9fe343..b4b10da 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -173,7 +173,8 @@ struct security_class_mapping secclass_map[] =
> > {
> >  	  { COMMON_CAP2_PERMS, NULL } },
> >  	{ "sctp_socket",
> >  	  { COMMON_SOCK_PERMS,
> > -	    "node_bind", NULL } },
> > +	    "node_bind", "name_connect", "association", "bindx",
> > +	    "connectx", NULL } },
> >  	{ "icmp_socket",
> >  	  { COMMON_SOCK_PERMS,
> >  	    "node_bind", NULL } },
> > diff --git a/security/selinux/include/netlabel.h
> > b/security/selinux/include/netlabel.h
> > index 75686d5..835a0d6 100644
> > --- a/security/selinux/include/netlabel.h
> > +++ b/security/selinux/include/netlabel.h
> > @@ -33,6 +33,7 @@
> >  #include <linux/skbuff.h>
> >  #include <net/sock.h>
> >  #include <net/request_sock.h>
> > +#include <net/sctp/structs.h>
> >  
> >  #include "avc.h"
> >  #include "objsec.h"
> > @@ -53,7 +54,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff
> > *skb,
> >  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> >  				 u16 family,
> >  				 u32 sid);
> > -
> > +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> > +				     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);
> > @@ -114,6 +116,11 @@ static inline int
> > selinux_netlbl_conn_setsid(struct sock *sk,
> >  	return 0;
> >  }
> >  
> > +static inline int selinux_netlbl_sctp_assoc_request(struct
> > sctp_endpoint *ep,
> > +						    struct sk_buff
> > *skb)
> > +{
> > +	return 0;
> > +}
> >  static inline int selinux_netlbl_inet_conn_request(struct
> > request_sock *req,
> >  						   u16 family)
> >  {
> > diff --git a/security/selinux/include/objsec.h
> > b/security/selinux/include/objsec.h
> > index 6ebc61e..660f270 100644
> > --- a/security/selinux/include/objsec.h
> > +++ b/security/selinux/include/objsec.h
> > @@ -130,6 +130,11 @@ struct sk_security_struct {
> >  	u32 sid;			/* SID of this object */
> >  	u32 peer_sid;			/* SID of peer */
> >  	u16 sclass;			/* sock security class
> > */
> > +
> > +	enum {				/* SCTP association
> > state */
> > +		SCTP_ASSOC_UNSET = 0,
> > +		SCTP_ASSOC_SET,
> > +	} sctp_assoc_state;
> >  };
> >  
> >  struct tun_security_struct {
> > diff --git a/security/selinux/netlabel.c
> > b/security/selinux/netlabel.c
> > index aaba667..7d5aa15 100644
> > --- a/security/selinux/netlabel.c
> > +++ b/security/selinux/netlabel.c
> > @@ -250,6 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff
> > *skb,
> >  	sk = skb_to_full_sk(skb);
> >  	if (sk != NULL) {
> >  		struct sk_security_struct *sksec = sk-
> > >sk_security;
> > +
> >  		if (sksec->nlbl_state != NLBL_REQSKB)
> >  			return 0;
> >  		secattr = selinux_netlbl_sock_getattr(sk, sid);
> > @@ -271,6 +272,41 @@ int selinux_netlbl_skbuff_setsid(struct
> > sk_buff
> > *skb,
> >  }
> >  
> >  /**
> > + * selinux_netlbl_sctp_assoc_request - Label an incoming sctp
> > association.
> > + * @ep: incoming association endpoint.
> > + * @skb: the packet.
> > + *
> > + * Description:
> > + * A new incoming connection is represented by @ep, ......
> > + * Returns zero on success, negative values on failure.
> > + *
> > + */
> > +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> > +				     struct sk_buff *skb)
> > +{
> > +	int rc;
> > +	struct netlbl_lsm_secattr secattr;
> > +	struct sk_security_struct *sksec = ep->base.sk-
> > >sk_security;
> > +
> > +	if (ep->base.sk->sk_family != PF_INET &&
> > +				ep->base.sk->sk_family !=
> > PF_INET6)
> > +		return 0;
> > +
> > +	netlbl_secattr_init(&secattr);
> > +	rc = security_netlbl_sid_to_secattr(ep->secid, &secattr);
> > +	if (rc != 0)
> > +		goto assoc_request_return;
> > +
> > +	rc = netlbl_sctp_setattr(ep->base.sk, skb, &secattr);
> > +	if (rc == 0)
> > +		sksec->nlbl_state = NLBL_LABELED;
> > +
> > +assoc_request_return:
> > +	netlbl_secattr_destroy(&secattr);
> > +	return rc;
> > +}
> > +
> > +/**
> >   * selinux_netlbl_inet_conn_request - Label an incoming stream
> > connection
> >   * @req: incoming connection request socket
> >   *
> > @@ -481,7 +517,7 @@ int selinux_netlbl_socket_setsockopt(struct
> > socket *sock,
> >   */
> >  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr
> > *addr)
> >  {
> > -	int rc;
> > +	int rc, already_owned_by_user = 0;
> >  	struct sk_security_struct *sksec = sk->sk_security;
> >  	struct netlbl_lsm_secattr *secattr;
> >  
> > @@ -489,7 +525,16 @@ int selinux_netlbl_socket_connect(struct sock
> > *sk, struct sockaddr *addr)
> >  	    sksec->nlbl_state != NLBL_CONNLABELED)
> >  		return 0;
> >  
> > -	lock_sock(sk);
> > +	/* Note: When called via connect(2) this happens before
> > the
> > socket
> > +	 * protocol layer connect operation and @sk is not locked,
> > HOWEVER,
> > +	 * when called by the SCTP protocol layer via
> > sctp_connectx(3),
> > +	 * sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore
> > check if
> > +	 * @sk owned already.
> > +	 */
> > +	if (sock_owned_by_user(sk) && sksec->sclass ==
> > SECCLASS_SCTP_SOCKET)
> > +		already_owned_by_user = 1;
> > +	else
> > +		lock_sock(sk);
> 
> Conditional locking is generally considered harmful.  I'd split the
> cases for the different callers, and use a common helper for both.

I've now split this as suggested.
> 
> >  
> >  	/* connected sockets are allowed to disconnect when the
> > address family
> >  	 * is set to AF_UNSPEC, if that is what is happening we
> > want
> > to reset
> > @@ -510,6 +555,7 @@ int selinux_netlbl_socket_connect(struct sock
> > *sk, struct sockaddr *addr)
> >  		sksec->nlbl_state = NLBL_CONNLABELED;
> >  
> >  socket_connect_return:
> > -	release_sock(sk);
> > +	if (!already_owned_by_user)
> > +		release_sock(sk);
> >  	return rc;
> >  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" 
> in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marcelo Ricardo Leitner Oct. 31, 2017, 5:16 p.m. UTC | #3

On Tue, Oct 17, 2017 at 02:59:53PM +0100, Richard Haines wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.txt
> 
> Signed-off-by: Richard Haines <[e-mail]>
> ---
...
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 33fd061..c3e9600 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
...
> @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
>  		unsigned short snum;
>  		u32 sid, perm;
>  
> -		if (sk->sk_family == PF_INET) {
> +		/* sctp_connectx(3) calls via
> +		 *selinux_sctp_bind_connect() that validates multiple
> +		 * connect addresses. Because of this need to check
> +		 * address->sa_family as it is possible to have
> +		 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> +		 */
> +		if (sk->sk_family == PF_INET ||
> +					address->sa_family == AF_INET) {

Not sure which code style applies on this file but the if () above
looks odd. At least, checkpatch.pl complained about it.

  Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Richard Haines Nov. 1, 2017, 9:34 p.m. UTC | #4

On Tue, 2017-10-31 at 15:16 -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Oct 17, 2017 at 02:59:53PM +0100, Richard Haines wrote:
> > The SELinux SCTP implementation is explained in:
> > Documentation/security/SELinux-sctp.txt
> > 
> > Signed-off-by: Richard Haines <[e-mail]>
> > ---
> 
> ...
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 33fd061..c3e9600 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> 
> ...
> > @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >  		unsigned short snum;
> >  		u32 sid, perm;
> >  
> > -		if (sk->sk_family == PF_INET) {
> > +		/* sctp_connectx(3) calls via
> > +		 *selinux_sctp_bind_connect() that validates
> > multiple
> > +		 * connect addresses. Because of this need to
> > check
> > +		 * address->sa_family as it is possible to have
> > +		 * sk->sk_family = PF_INET6 with addr->sa_family =
> > AF_INET.
> > +		 */
> > +		if (sk->sk_family == PF_INET ||
> > +					address->sa_family ==
> > AF_INET) {
> 
> Not sure which code style applies on this file but the if () above
> looks odd. At least, checkpatch.pl complained about it.
Changed to read:
		if (sk->sk_family == PF_INET ||
		    address->sa_family == AF_INET) {

> 
>   Marcelo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Nov. 7, 2017, 12:09 a.m. UTC | #5

On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
<[e-mail]> wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.txt
>
> Signed-off-by: Richard Haines <[e-mail]>
> ---
>  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
>  security/selinux/hooks.c                | 268 ++++++++++++++++++++++++++++++--
>  security/selinux/include/classmap.h     |   3 +-
>  security/selinux/include/netlabel.h     |   9 +-
>  security/selinux/include/objsec.h       |   5 +
>  security/selinux/netlabel.c             |  52 ++++++-
>  6 files changed, 427 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/security/SELinux-sctp.txt
>
> diff --git a/Documentation/security/SELinux-sctp.txt b/Documentation/security/SELinux-sctp.txt
> new file mode 100644
> index 0000000..32e0255
> --- /dev/null
> +++ b/Documentation/security/SELinux-sctp.txt
> @@ -0,0 +1,108 @@

See my previous comments about moving to reStructuredText for these docs.

> +                               SCTP SELinux Support
> +                              ======================
> +
> +Security Hooks
> +===============
> +
> +The Documentation/security/LSM-sctp.txt document describes how the following
> +sctp security hooks are utilised:
> +    security_sctp_assoc_request()
> +    security_sctp_bind_connect()
> +    security_sctp_sk_clone()
> +
> +    security_inet_conn_established()
> +
> +
> +Policy Statements
> +==================
> +The following class and permissions to support SCTP are available within the
> +kernel:
> +    class sctp_socket inherits socket { node_bind }
> +
> +whenever the following policy capability is enabled:
> +    policycap extended_socket_class;
> +
> +The SELinux SCTP support adds the additional permissions that are explained
> +in the sections below:
> +    association bindx connectx

Is the distinction between bind and bindx significant?  The same
question applies to connect/connectx.  I think we can probably just
reuse bind and connect in these cases.

See my question on sctp_socket:association below.

> +If userspace tools have been updated, SCTP will support the portcon
> +statement as shown in the following example:
> +    portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> +
> +
> +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> +================================================================
> +The hook security_sctp_bind_connect() is called by SCTP to check permissions
> +required for ipv4/ipv6 addresses based on the @optname as follows:
> +
> +  ------------------------------------------------------------------
> +  |                      BINDX Permission Check                    |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                  BIND Permission Checks                        |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6 address       |
> +  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                 CONNECTX Permission Check                      |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                 CONNECT Permission Checks                      |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6 address       |
> +  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses |
> +  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +SCTP Peer Labeling
> +===================
> +An SCTP socket will only have one peer label assigned to it. This will be
> +assigned during the establishment of the first association. Once the peer
> +label has been assigned, any new associations will have the "association"
> +permission validated by checking the socket peer sid against the received
> +packets peer sid to determine whether the association should be allowed or
> +denied.
> +
> +NOTES:
> +   1) If peer labeling is not enabled, then the peer context will always be
> +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> +
> +   2) As SCTP supports multiple endpoints with multi-homing on a single socket
> +      it is recommended that peer labels are consistent.

My apologies if I'm confused, but I thought it was multiple
associations per-endpoint, with the associations providing the
multi-homing functionality, no?

> +   3) getpeercon(3) may be used by userspace to retrieve the sockets peer
> +       context.
> +
> +   4) If using NetLabel be aware that if a label is assigned to a specific
> +      interface, and that interface 'goes down', then the NetLabel service
> +      will remove the entry. Therefore ensure that the network startup scripts
> +      call netlabelctl(8) to set the required label (see netlabel-config(8)
> +      helper script for details).

Maybe this will be made clear as I work my way through this patch, but
how is point #4 SCTP specific?

> +   5) The NetLabel SCTP peer labeling rules apply as discussed in the following
> +      set of posts tagged "netlabel" at: http://www.paul-moore.com/blog/t.
> +
> +   6) CIPSO is only supported for IPv4 addressing: socket(AF_INET, ...)
> +      CALIPSO is only supported for IPv6 addressing: socket(AF_INET6, ...)
> +
> +      Note the following when testing CIPSO/CALIPSO:
> +         a) CIPSO will send an ICMP packet if an SCTP packet cannot be
> +            delivered because of an invalid label.
> +         b) CALIPSO does not send an ICMP packet, just silently discards it.

Okay, this answers one of my questions from earlier in the patchset.

> +   7) IPSEC is not supported as rfc3554 - sctp/ipsec support has not been
> +      implemented in userspace (racoon(8) or ipsec_pluto(8)), although the
> +      kernel supports SCTP/IPSEC.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 33fd061..c3e9600 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -67,6 +67,8 @@
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
>  #include <linux/dccp.h>
> +#include <linux/sctp.h>
> +#include <net/sctp/structs.h>
>  #include <linux/quota.h>
>  #include <linux/un.h>          /* for Unix socket types */
>  #include <net/af_unix.h>       /* for Unix socket types */
> @@ -4119,6 +4121,23 @@ static int selinux_parse_skb_ipv4(struct sk_buff *skb,
>                 break;
>         }
>
> +#if IS_ENABLED(CONFIG_IP_SCTP)
> +       case IPPROTO_SCTP: {
> +               struct sctphdr _sctph, *sh;
> +
> +               if (ntohs(ih->frag_off) & IP_OFFSET)
> +                       break;
> +
> +               offset += ihlen;
> +               sh = skb_header_pointer(skb, offset, sizeof(_sctph), &_sctph);
> +               if (sh == NULL)
> +                       break;
> +
> +               ad->u.net->sport = sh->source;
> +               ad->u.net->dport = sh->dest;
> +               break;
> +       }
> +#endif
>         default:
>                 break;
>         }
> @@ -4192,6 +4211,19 @@ static int selinux_parse_skb_ipv6(struct sk_buff *skb,
>                 break;
>         }
>
> +#if IS_ENABLED(CONFIG_IP_SCTP)
> +       case IPPROTO_SCTP: {
> +               struct sctphdr _sctph, *sh;
> +
> +               sh = skb_header_pointer(skb, offset, sizeof(_sctph), &_sctph);
> +               if (sh == NULL)
> +                       break;
> +
> +               ad->u.net->sport = sh->source;
> +               ad->u.net->dport = sh->dest;
> +               break;
> +       }
> +#endif
>         /* includes fragments */
>         default:
>                 break;
> @@ -4381,6 +4413,10 @@ static int selinux_socket_post_create(struct socket *sock, int family,
>                 sksec = sock->sk->sk_security;
>                 sksec->sclass = sclass;
>                 sksec->sid = sid;
> +               /* Allows detection of the first association on this socket */
> +               if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> +                       sksec->sctp_assoc_state = SCTP_ASSOC_UNSET;
> +
>                 err = selinux_netlbl_socket_post_create(sock->sk, family);
>         }
>
> @@ -4401,11 +4437,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>         if (err)
>                 goto out;
>
> -       /*
> -        * If PF_INET or PF_INET6, check name_bind permission for the port.
> -        * Multiple address binding for SCTP is not supported yet: we just
> -        * check the first address now.
> -        */
> +       /* If PF_INET or PF_INET6, check name_bind permission for the port. */
>         family = sk->sk_family;

Instead of checking both family and address->sa_family multiple times
in this function, let's just set the correct family value once and
store it in the family variable as we do today; for example:

  family = sk->sk_family;
  if (family == PF_INET6 && address->sa_family == PF_INET)
    family = PF_INET;

... or I wonder if it is just safe to ignore the address family in the
socket, and simply use the value provided in address?  Perhaps that is
the better option?

>         if (family == PF_INET || family == PF_INET6) {
>                 char *addrp;
> @@ -4417,7 +4449,13 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                 unsigned short snum;
>                 u32 sid, node_perm;
>
> -               if (family == PF_INET) {
> +               /*
> +                * sctp_bindx(3) calls via selinux_sctp_bind_connect()
> +                * that validates multiple binding addresses. Because of this
> +                * need to check address->sa_family as it is possible to have
> +                * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> +                */
> +               if (family == PF_INET || address->sa_family == AF_INET) {
>                         if (addrlen < sizeof(struct sockaddr_in)) {
>                                 err = -EINVAL;
>                                 goto out;
> @@ -4471,6 +4509,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                         node_perm = DCCP_SOCKET__NODE_BIND;
>                         break;
>
> +               case SECCLASS_SCTP_SOCKET:
> +                       node_perm = SCTP_SOCKET__NODE_BIND;
> +                       break;
> +
>                 default:
>                         node_perm = RAWIP_SOCKET__NODE_BIND;
>                         break;
> @@ -4485,7 +4527,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                 ad.u.net->sport = htons(snum);
>                 ad.u.net->family = family;
>
> -               if (family == PF_INET)
> +               if (family == PF_INET || address->sa_family == AF_INET)
>                         ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>                 else
>                         ad.u.net->v6info.saddr = addr6->sin6_addr;
> @@ -4510,10 +4552,12 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
>                 return err;
>
>         /*
> -        * If a TCP or DCCP socket, check name_connect permission for the port.
> +        * If a TCP, DCCP or SCTP socket, check name_connect permission
> +        * for the port.
>          */
>         if (sksec->sclass == SECCLASS_TCP_SOCKET ||
> -           sksec->sclass == SECCLASS_DCCP_SOCKET) {
> +           sksec->sclass == SECCLASS_DCCP_SOCKET ||
> +           sksec->sclass == SECCLASS_SCTP_SOCKET) {
>                 struct common_audit_data ad;
>                 struct lsm_network_audit net = {0,};
>                 struct sockaddr_in *addr4 = NULL;
> @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
>                 unsigned short snum;
>                 u32 sid, perm;
>
> -               if (sk->sk_family == PF_INET) {
> +               /* sctp_connectx(3) calls via
> +                *selinux_sctp_bind_connect() that validates multiple

Typo.  A space is needed after the "*" in the comment line above.

> +                * connect addresses. Because of this need to check
> +                * address->sa_family as it is possible to have
> +                * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> +                */
> +               if (sk->sk_family == PF_INET ||
> +                                       address->sa_family == AF_INET) {

Once again, I wonder if we just check sa_family and skip the sk_family?

>                         addr4 = (struct sockaddr_in *)address;
>                         if (addrlen < sizeof(struct sockaddr_in))
>                                 return -EINVAL;
> @@ -4534,11 +4585,21 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
>                 }
>
>                 err = sel_netport_sid(sk->sk_protocol, snum, &sid);
> +

No added vertical whitespace here please.

>                 if (err)
>                         goto out;
>
> -               perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
> -                      TCP_SOCKET__NAME_CONNECT : DCCP_SOCKET__NAME_CONNECT;
> +               switch (sksec->sclass) {
> +               case SECCLASS_TCP_SOCKET:
> +                       perm = TCP_SOCKET__NAME_CONNECT;
> +                       break;
> +               case SECCLASS_DCCP_SOCKET:
> +                       perm = DCCP_SOCKET__NAME_CONNECT;
> +                       break;
> +               case SECCLASS_SCTP_SOCKET:
> +                       perm = SCTP_SOCKET__NAME_CONNECT;
> +                       break;
> +               }
>
>                 ad.type = LSM_AUDIT_DATA_NET;
>                 ad.u.net = &net;
> @@ -4815,7 +4876,8 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
>         u32 peer_sid = SECSID_NULL;
>
>         if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
> -           sksec->sclass == SECCLASS_TCP_SOCKET)
> +           sksec->sclass == SECCLASS_TCP_SOCKET ||
> +           sksec->sclass == SECCLASS_SCTP_SOCKET)
>                 peer_sid = sksec->peer_sid;
>         if (peer_sid == SECSID_NULL)
>                 return -ENOPROTOOPT;
> @@ -4928,6 +4990,183 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
>         sksec->sclass = isec->sclass;
>  }
>
> +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
> +                                     struct sk_buff *skb,
> +                                     int sctp_cid)
> +{
> +       struct sk_security_struct *sksec = ep->base.sk->sk_security;
> +       struct common_audit_data ad;
> +       struct lsm_network_audit net = {0,};
> +       u8 peerlbl_active;
> +       u32 peer_sid = SECINITSID_UNLABELED;
> +       u32 conn_sid;
> +       int err;
> +
> +       if (!selinux_policycap_extsockclass)
> +               return 0;

We *may* need to protect a lot of the new SCTP code with a new policy
capability, I think reusing extsockclass here could be problematic.

> +       peerlbl_active = selinux_peerlbl_enabled();
> +
> +       if (peerlbl_active) {
> +               /* This will return peer_sid = SECSID_NULL if there are
> +                * no peer labels, see security_net_peersid_resolve().
> +                */
> +               err = selinux_skb_peerlbl_sid(skb, ep->base.sk->sk_family,
> +                                             &peer_sid);
> +

Drop the extra vertical whitespace between the function call and the
return code check please.

> +               if (err)
> +                       return err;
> +
> +               if (peer_sid == SECSID_NULL)
> +                       peer_sid = SECINITSID_UNLABELED;
> +       }
> +
> +       if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> +               sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> +
> +               /* Here as first association on socket. As the peer SID
> +                * was allowed by peer recv (and the netif/node checks),
> +                * then it is approved by policy and used as the primary
> +                * peer SID for getpeercon(3).
> +                */
> +               sksec->peer_sid = peer_sid;
> +       } else if  (sksec->peer_sid != peer_sid) {
> +               /* Other association peer SIDs are checked to enforce
> +                * consistency among the peer SIDs.
> +                */
> +               ad.type = LSM_AUDIT_DATA_NET;
> +               ad.u.net = &net;
> +               ad.u.net->sk = ep->base.sk;
> +               err = avc_has_perm(sksec->peer_sid, peer_sid, sksec->sclass,
> +                                  SCTP_SOCKET__ASSOCIATION, &ad);

Can anyone think of any reason why we would ever want to allow an
association that doesn't have the same label as the existing
associations?  Maybe I'm thinking about this wrong, but I can't
imagine this being a good idea ...

> +               if (err)
> +                       return err;
> +       }
> +
> +       if (sctp_cid == SCTP_CID_INIT) {
> +               /* Have INIT when incoming connect(2), sctp_connectx(3)
> +                * or sctp_sendmsg(3) (with no association already present),
> +                * so compute the MLS component for the connection and store
> +                * the information in ep. This will be used by SCTP TCP type
> +                * sockets and peeled off connections as they cause a new
> +                * socket to be generated. selinux_sctp_sk_clone() will then
> +                * plug this into the new socket.
> +                */
> +               err = selinux_conn_sid(sksec->sid, peer_sid, &conn_sid);
> +               if (err)
> +                       return err;
> +
> +               ep->secid = conn_sid;
> +               ep->peer_secid = peer_sid;
> +
> +               /* Set any NetLabel labels including CIPSO/CALIPSO options. */
> +               return selinux_netlbl_sctp_assoc_request(ep, skb);
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * Check if sctp IPv4/IPv6 addresses are valid for binding or connecting
> + * based on their @optname.
> + */
> +static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> +                                    struct sockaddr *address,
> +                                    int addrlen)
> +{
> +       int len, err = 0, walk_size = 0;
> +       void *addr_buf;
> +       struct sockaddr *addr;
> +       struct socket *sock;
> +
> +       if (!selinux_policycap_extsockclass)
> +               return 0;
> +
> +       switch (optname) {
> +       case SCTP_SOCKOPT_BINDX_ADD:
> +               err = sock_has_perm(sk, SCTP_SOCKET__BINDX);
> +               break;
> +       case SCTP_SOCKOPT_CONNECTX:
> +               err = sock_has_perm(sk, SCTP_SOCKET__CONNECTX);
> +               break;
> +       /* These need SOCKET__BIND or SOCKET__CONNECT permissions that will
> +        * be checked later.
> +        */
> +       case SCTP_PRIMARY_ADDR:
> +       case SCTP_SET_PEER_PRIMARY_ADDR:
> +       case SCTP_PARAM_SET_PRIMARY:
> +       case SCTP_PARAM_ADD_IP:
> +       case SCTP_SENDMSG_CONNECT:
> +               break;
> +       default:
> +               err = -EINVAL;
> +       }
> +       if (err)
> +               return err;
> +
> +       /* Process one or more addresses that may be IPv4 or IPv6 */
> +       sock = sk->sk_socket;
> +       addr_buf = address;
> +
> +       while (walk_size < addrlen) {
> +               addr = addr_buf;
> +               switch (addr->sa_family) {
> +               case AF_INET:
> +                       len = sizeof(struct sockaddr_in);
> +                       break;
> +               case AF_INET6:
> +                       len = sizeof(struct sockaddr_in6);
> +                       break;
> +               default:
> +                       return -EAFNOSUPPORT;
> +               }
> +
> +               err = -EINVAL;
> +               switch (optname) {
> +               /* Bind checks */
> +               case SCTP_PRIMARY_ADDR:
> +               case SCTP_SET_PEER_PRIMARY_ADDR:
> +               case SCTP_SOCKOPT_BINDX_ADD:
> +                       err = selinux_socket_bind(sock, addr, len);
> +                       break;
> +               /* Connect checks */
> +               case SCTP_SOCKOPT_CONNECTX:
> +               case SCTP_PARAM_SET_PRIMARY:
> +               case SCTP_PARAM_ADD_IP:
> +               case SCTP_SENDMSG_CONNECT:
> +                       err = selinux_socket_connect(sock, addr, len);
> +                       break;
> +               }
> +
> +               if (err)
> +                       return err;
> +
> +               addr_buf += len;
> +               walk_size += len;
> +       }
> +       return 0;
> +}
> +
> +/* Called whenever a new socket is created by accept(2) or sctp_peeloff(3). */
> +static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> +                                 struct sock *newsk)
> +{
> +       struct sk_security_struct *sksec = sk->sk_security;
> +       struct sk_security_struct *newsksec = newsk->sk_security;
> +
> +       /* If policy does not support SECCLASS_SCTP_SOCKET then call
> +        * the non-sctp clone version.
> +        */
> +       if (!selinux_policycap_extsockclass)
> +               return selinux_sk_clone_security(sk, newsk);
> +
> +       newsksec->sid = ep->secid;
> +       newsksec->peer_sid = ep->peer_secid;
> +       newsksec->sclass = sksec->sclass;
> +       newsksec->nlbl_state = sksec->nlbl_state;
> +}
> +
>  static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
>                                      struct request_sock *req)
>  {
> @@ -6416,6 +6655,9 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
>         LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
>         LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
> +       LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request),
> +       LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
> +       LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect),
>         LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request),
>         LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
>         LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established),
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index b9fe343..b4b10da 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -173,7 +173,8 @@ struct security_class_mapping secclass_map[] = {
>           { COMMON_CAP2_PERMS, NULL } },
>         { "sctp_socket",
>           { COMMON_SOCK_PERMS,
> -           "node_bind", NULL } },
> +           "node_bind", "name_connect", "association", "bindx",
> +           "connectx", NULL } },
>         { "icmp_socket",
>           { COMMON_SOCK_PERMS,
>             "node_bind", NULL } },
> diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
> index 75686d5..835a0d6 100644
> --- a/security/selinux/include/netlabel.h
> +++ b/security/selinux/include/netlabel.h
> @@ -33,6 +33,7 @@
>  #include <linux/skbuff.h>
>  #include <net/sock.h>
>  #include <net/request_sock.h>
> +#include <net/sctp/structs.h>
>
>  #include "avc.h"
>  #include "objsec.h"
> @@ -53,7 +54,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
>  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
>                                  u16 family,
>                                  u32 sid);
> -
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +                                    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);
> @@ -114,6 +116,11 @@ static inline int selinux_netlbl_conn_setsid(struct sock *sk,
>         return 0;
>  }
>
> +static inline int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +                                                   struct sk_buff *skb)
> +{
> +       return 0;
> +}
>  static inline int selinux_netlbl_inet_conn_request(struct request_sock *req,
>                                                    u16 family)
>  {
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 6ebc61e..660f270 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -130,6 +130,11 @@ struct sk_security_struct {
>         u32 sid;                        /* SID of this object */
>         u32 peer_sid;                   /* SID of peer */
>         u16 sclass;                     /* sock security class */
> +

Extra vertical whitespace is not needed.

> +       enum {                          /* SCTP association state */
> +               SCTP_ASSOC_UNSET = 0,
> +               SCTP_ASSOC_SET,
> +       } sctp_assoc_state;
>  };
>
>  struct tun_security_struct {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index aaba667..7d5aa15 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -250,6 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
>         sk = skb_to_full_sk(skb);
>         if (sk != NULL) {
>                 struct sk_security_struct *sksec = sk->sk_security;
> +
>                 if (sksec->nlbl_state != NLBL_REQSKB)
>                         return 0;
>                 secattr = selinux_netlbl_sock_getattr(sk, sid);
> @@ -271,6 +272,41 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
>  }
>
>  /**
> + * selinux_netlbl_sctp_assoc_request - Label an incoming sctp association.
> + * @ep: incoming association endpoint.
> + * @skb: the packet.
> + *
> + * Description:
> + * A new incoming connection is represented by @ep, ......
> + * Returns zero on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +                                    struct sk_buff *skb)
> +{
> +       int rc;
> +       struct netlbl_lsm_secattr secattr;
> +       struct sk_security_struct *sksec = ep->base.sk->sk_security;
> +
> +       if (ep->base.sk->sk_family != PF_INET &&
> +                               ep->base.sk->sk_family != PF_INET6)
> +               return 0;
> +
> +       netlbl_secattr_init(&secattr);
> +       rc = security_netlbl_sid_to_secattr(ep->secid, &secattr);
> +       if (rc != 0)
> +               goto assoc_request_return;
> +
> +       rc = netlbl_sctp_setattr(ep->base.sk, skb, &secattr);
> +       if (rc == 0)
> +               sksec->nlbl_state = NLBL_LABELED;
> +
> +assoc_request_return:
> +       netlbl_secattr_destroy(&secattr);
> +       return rc;
> +}
> +
> +/**
>   * selinux_netlbl_inet_conn_request - Label an incoming stream connection
>   * @req: incoming connection request socket
>   *
> @@ -481,7 +517,7 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock,
>   */
>  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
>  {
> -       int rc;
> +       int rc, already_owned_by_user = 0;
>         struct sk_security_struct *sksec = sk->sk_security;
>         struct netlbl_lsm_secattr *secattr;
>
> @@ -489,7 +525,16 @@ int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
>             sksec->nlbl_state != NLBL_CONNLABELED)
>                 return 0;
>
> -       lock_sock(sk);
> +       /* Note: When called via connect(2) this happens before the socket
> +        * protocol layer connect operation and @sk is not locked, HOWEVER,
> +        * when called by the SCTP protocol layer via sctp_connectx(3),
> +        * sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore check if
> +        * @sk owned already.
> +        */
> +       if (sock_owned_by_user(sk) && sksec->sclass == SECCLASS_SCTP_SOCKET)
> +               already_owned_by_user = 1;
> +       else
> +               lock_sock(sk);
>
>         /* connected sockets are allowed to disconnect when the address family
>          * is set to AF_UNSPEC, if that is what is happening we want to reset
> @@ -510,6 +555,7 @@ int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
>                 sksec->nlbl_state = NLBL_CONNLABELED;
>
>  socket_connect_return:
> -       release_sock(sk);
> +       if (!already_owned_by_user)
> +               release_sock(sk);
>         return rc;
>  }
> --
> 2.13.6
>

Richard Haines Nov. 13, 2017, 10:05 p.m. UTC | #6

On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> <[e-mail]> wrote:
> > The SELinux SCTP implementation is explained in:
> > Documentation/security/SELinux-sctp.txt
> > 
> > Signed-off-by: Richard Haines <[e-mail]>
> > ---
> >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
> >  security/selinux/hooks.c                | 268
> > ++++++++++++++++++++++++++++++--
> >  security/selinux/include/classmap.h     |   3 +-
> >  security/selinux/include/netlabel.h     |   9 +-
> >  security/selinux/include/objsec.h       |   5 +
> >  security/selinux/netlabel.c             |  52 ++++++-
> >  6 files changed, 427 insertions(+), 18 deletions(-)
> >  create mode 100644 Documentation/security/SELinux-sctp.txt
> > 
> > diff --git a/Documentation/security/SELinux-sctp.txt
> > b/Documentation/security/SELinux-sctp.txt
> > new file mode 100644
> > index 0000000..32e0255
> > --- /dev/null
> > +++ b/Documentation/security/SELinux-sctp.txt
> > @@ -0,0 +1,108 @@
> 
> See my previous comments about moving to reStructuredText for these
> docs.

Done
> 
> > +                               SCTP SELinux Support
> > +                              ======================
> > +
> > +Security Hooks
> > +===============
> > +
> > +The Documentation/security/LSM-sctp.txt document describes how the
> > following
> > +sctp security hooks are utilised:
> > +    security_sctp_assoc_request()
> > +    security_sctp_bind_connect()
> > +    security_sctp_sk_clone()
> > +
> > +    security_inet_conn_established()
> > +
> > +
> > +Policy Statements
> > +==================
> > +The following class and permissions to support SCTP are available
> > within the
> > +kernel:
> > +    class sctp_socket inherits socket { node_bind }
> > +
> > +whenever the following policy capability is enabled:
> > +    policycap extended_socket_class;
> > +
> > +The SELinux SCTP support adds the additional permissions that are
> > explained
> > +in the sections below:
> > +    association bindx connectx
> 
> Is the distinction between bind and bindx significant?  The same
> question applies to connect/connectx.  I think we can probably just
> reuse bind and connect in these cases.

This has been discussed before with Marcelo and keeping bindx/connectx
is a useful distinction.
> 
> See my question on sctp_socket:association below.
> 
> > +If userspace tools have been updated, SCTP will support the
> > portcon
> > +statement as shown in the following example:
> > +    portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> > +
> > +
> > +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> > +================================================================
> > +The hook security_sctp_bind_connect() is called by SCTP to check
> > permissions
> > +required for ipv4/ipv6 addresses based on the @optname as follows:
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                      BINDX Permission
> > Check                    |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses
> > |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                  BIND Permission
> > Checks                        |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6
> > address       |
> > +  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6
> > address       |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                 CONNECTX Permission
> > Check                      |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses
> > |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +  --------------------------------------------------------------
> > ----
> > +  |                 CONNECT Permission
> > Checks                      |
> > +  |       @optname             |         @address
> > contains         |
> > +  |----------------------------|--------------------------------
> > ---|
> > +  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6
> > address       |
> > +  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses
> > |
> > +  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6
> > address       |
> > +  --------------------------------------------------------------
> > ----
> > +
> > +SCTP Peer Labeling
> > +===================
> > +An SCTP socket will only have one peer label assigned to it. This
> > will be
> > +assigned during the establishment of the first association. Once
> > the peer
> > +label has been assigned, any new associations will have the
> > "association"
> > +permission validated by checking the socket peer sid against the
> > received
> > +packets peer sid to determine whether the association should be
> > allowed or
> > +denied.
> > +
> > +NOTES:
> > +   1) If peer labeling is not enabled, then the peer context will
> > always be
> > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> > +
> > +   2) As SCTP supports multiple endpoints with multi-homing on a
> > single socket
> > +      it is recommended that peer labels are consistent.
> 
> My apologies if I'm confused, but I thought it was multiple
> associations per-endpoint, with the associations providing the
> multi-homing functionality, no?

I've reworded to:
As SCTP can support more than one transport address per endpoint
(multi-homing) on a single socket, it is possible to configure policy
and NetLabel to provide different peer labels for each of these. As the
socket peer label is determined by the first associations transport
address, it is recommended that all peer labels are consistent.

> 
> > +   3) getpeercon(3) may be used by userspace to retrieve the
> > sockets peer
> > +       context.
> > +
> > +   4) If using NetLabel be aware that if a label is assigned to a
> > specific
> > +      interface, and that interface 'goes down', then the NetLabel
> > service
> > +      will remove the entry. Therefore ensure that the network
> > startup scripts
> > +      call netlabelctl(8) to set the required label (see netlabel-
> > config(8)
> > +      helper script for details).
> 
> Maybe this will be made clear as I work my way through this patch,
> but
> how is point #4 SCTP specific?

It's not, I added this as a useful hint as I keep forgetting about it,
I'll reword to: While not SCTP specific, be aware .....

> 
> > +   5) The NetLabel SCTP peer labeling rules apply as discussed in
> > the following
> > +      set of posts tagged "netlabel" at: http://www.paul-moore.com
> > /blog/t.
> > +
> > +   6) CIPSO is only supported for IPv4 addressing: socket(AF_INET,
> > ...)
> > +      CALIPSO is only supported for IPv6 addressing:
> > socket(AF_INET6, ...)
> > +
> > +      Note the following when testing CIPSO/CALIPSO:
> > +         a) CIPSO will send an ICMP packet if an SCTP packet
> > cannot be
> > +            delivered because of an invalid label.
> > +         b) CALIPSO does not send an ICMP packet, just silently
> > discards it.
> 
> Okay, this answers one of my questions from earlier in the patchset.
> 
> > +   7) IPSEC is not supported as rfc3554 - sctp/ipsec support has
> > not been
> > +      implemented in userspace (racoon(8) or ipsec_pluto(8)),
> > although the
> > +      kernel supports SCTP/IPSEC.
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 33fd061..c3e9600 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -67,6 +67,8 @@
> >  #include <linux/tcp.h>
> >  #include <linux/udp.h>
> >  #include <linux/dccp.h>
> > +#include <linux/sctp.h>
> > +#include <net/sctp/structs.h>
> >  #include <linux/quota.h>
> >  #include <linux/un.h>          /* for Unix socket types */
> >  #include <net/af_unix.h>       /* for Unix socket types */
> > @@ -4119,6 +4121,23 @@ static int selinux_parse_skb_ipv4(struct
> > sk_buff *skb,
> >                 break;
> >         }
> > 
> > +#if IS_ENABLED(CONFIG_IP_SCTP)
> > +       case IPPROTO_SCTP: {
> > +               struct sctphdr _sctph, *sh;
> > +
> > +               if (ntohs(ih->frag_off) & IP_OFFSET)
> > +                       break;
> > +
> > +               offset += ihlen;
> > +               sh = skb_header_pointer(skb, offset,
> > sizeof(_sctph), &_sctph);
> > +               if (sh == NULL)
> > +                       break;
> > +
> > +               ad->u.net->sport = sh->source;
> > +               ad->u.net->dport = sh->dest;
> > +               break;
> > +       }
> > +#endif
> >         default:
> >                 break;
> >         }
> > @@ -4192,6 +4211,19 @@ static int selinux_parse_skb_ipv6(struct
> > sk_buff *skb,
> >                 break;
> >         }
> > 
> > +#if IS_ENABLED(CONFIG_IP_SCTP)
> > +       case IPPROTO_SCTP: {
> > +               struct sctphdr _sctph, *sh;
> > +
> > +               sh = skb_header_pointer(skb, offset,
> > sizeof(_sctph), &_sctph);
> > +               if (sh == NULL)
> > +                       break;
> > +
> > +               ad->u.net->sport = sh->source;
> > +               ad->u.net->dport = sh->dest;
> > +               break;
> > +       }
> > +#endif
> >         /* includes fragments */
> >         default:
> >                 break;
> > @@ -4381,6 +4413,10 @@ static int selinux_socket_post_create(struct
> > socket *sock, int family,
> >                 sksec = sock->sk->sk_security;
> >                 sksec->sclass = sclass;
> >                 sksec->sid = sid;
> > +               /* Allows detection of the first association on
> > this socket */
> > +               if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> > +                       sksec->sctp_assoc_state = SCTP_ASSOC_UNSET;
> > +
> >                 err = selinux_netlbl_socket_post_create(sock->sk,
> > family);
> >         }
> > 
> > @@ -4401,11 +4437,7 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >         if (err)
> >                 goto out;
> > 
> > -       /*
> > -        * If PF_INET or PF_INET6, check name_bind permission for
> > the port.
> > -        * Multiple address binding for SCTP is not supported yet:
> > we just
> > -        * check the first address now.
> > -        */
> > +       /* If PF_INET or PF_INET6, check name_bind permission for
> > the port. */
> >         family = sk->sk_family;
> 
> Instead of checking both family and address->sa_family multiple times
> in this function, let's just set the correct family value once and
> store it in the family variable as we do today; for example:
> 
>   family = sk->sk_family;
>   if (family == PF_INET6 && address->sa_family == PF_INET)
>     family = PF_INET;
> 
> ... or I wonder if it is just safe to ignore the address family in
> the
> socket, and simply use the value provided in address?  Perhaps that
> is
> the better option?

Done - Implemented the "ignore the socket address family" option

> 
> >         if (family == PF_INET || family == PF_INET6) {
> >                 char *addrp;
> > @@ -4417,7 +4449,13 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >                 unsigned short snum;
> >                 u32 sid, node_perm;
> > 
> > -               if (family == PF_INET) {
> > +               /*
> > +                * sctp_bindx(3) calls via
> > selinux_sctp_bind_connect()
> > +                * that validates multiple binding addresses.
> > Because of this
> > +                * need to check address->sa_family as it is
> > possible to have
> > +                * sk->sk_family = PF_INET6 with addr->sa_family =
> > AF_INET.
> > +                */
> > +               if (family == PF_INET || address->sa_family ==
> > AF_INET) {
> >                         if (addrlen < sizeof(struct sockaddr_in)) {
> >                                 err = -EINVAL;
> >                                 goto out;
> > @@ -4471,6 +4509,10 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >                         node_perm = DCCP_SOCKET__NODE_BIND;
> >                         break;
> > 
> > +               case SECCLASS_SCTP_SOCKET:
> > +                       node_perm = SCTP_SOCKET__NODE_BIND;
> > +                       break;
> > +
> >                 default:
> >                         node_perm = RAWIP_SOCKET__NODE_BIND;
> >                         break;
> > @@ -4485,7 +4527,7 @@ static int selinux_socket_bind(struct socket
> > *sock, struct sockaddr *address, in
> >                 ad.u.net->sport = htons(snum);
> >                 ad.u.net->family = family;
> > 
> > -               if (family == PF_INET)
> > +               if (family == PF_INET || address->sa_family ==
> > AF_INET)
> >                         ad.u.net->v4info.saddr = addr4-
> > >sin_addr.s_addr;
> >                 else
> >                         ad.u.net->v6info.saddr = addr6->sin6_addr;
> > @@ -4510,10 +4552,12 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >                 return err;
> > 
> >         /*
> > -        * If a TCP or DCCP socket, check name_connect permission
> > for the port.
> > +        * If a TCP, DCCP or SCTP socket, check name_connect
> > permission
> > +        * for the port.
> >          */
> >         if (sksec->sclass == SECCLASS_TCP_SOCKET ||
> > -           sksec->sclass == SECCLASS_DCCP_SOCKET) {
> > +           sksec->sclass == SECCLASS_DCCP_SOCKET ||
> > +           sksec->sclass == SECCLASS_SCTP_SOCKET) {
> >                 struct common_audit_data ad;
> >                 struct lsm_network_audit net = {0,};
> >                 struct sockaddr_in *addr4 = NULL;
> > @@ -4521,7 +4565,14 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >                 unsigned short snum;
> >                 u32 sid, perm;
> > 
> > -               if (sk->sk_family == PF_INET) {
> > +               /* sctp_connectx(3) calls via
> > +                *selinux_sctp_bind_connect() that validates
> > multiple
> 
> Typo.  A space is needed after the "*" in the comment line above.
> 
Done

> > +                * connect addresses. Because of this need to check
> > +                * address->sa_family as it is possible to have
> > +                * sk->sk_family = PF_INET6 with addr->sa_family =
> > AF_INET.
> > +                */
> > +               if (sk->sk_family == PF_INET ||
> > +                                       address->sa_family ==
> > AF_INET) {
> 
> Once again, I wonder if we just check sa_family and skip the
> sk_family?

Done

> 
> >                         addr4 = (struct sockaddr_in *)address;
> >                         if (addrlen < sizeof(struct sockaddr_in))
> >                                 return -EINVAL;
> > @@ -4534,11 +4585,21 @@ static int selinux_socket_connect(struct
> > socket *sock, struct sockaddr *address,
> >                 }
> > 
> >                 err = sel_netport_sid(sk->sk_protocol, snum, &sid);
> > +
> 
> No added vertical whitespace here please.

Done
> 
> >                 if (err)
> >                         goto out;
> > 
> > -               perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
> > -                      TCP_SOCKET__NAME_CONNECT :
> > DCCP_SOCKET__NAME_CONNECT;
> > +               switch (sksec->sclass) {
> > +               case SECCLASS_TCP_SOCKET:
> > +                       perm = TCP_SOCKET__NAME_CONNECT;
> > +                       break;
> > +               case SECCLASS_DCCP_SOCKET:
> > +                       perm = DCCP_SOCKET__NAME_CONNECT;
> > +                       break;
> > +               case SECCLASS_SCTP_SOCKET:
> > +                       perm = SCTP_SOCKET__NAME_CONNECT;
> > +                       break;
> > +               }
> > 
> >                 ad.type = LSM_AUDIT_DATA_NET;
> >                 ad.u.net = &net;
> > @@ -4815,7 +4876,8 @@ static int
> > selinux_socket_getpeersec_stream(struct socket *sock, char __user
> > *op
> >         u32 peer_sid = SECSID_NULL;
> > 
> >         if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
> > -           sksec->sclass == SECCLASS_TCP_SOCKET)
> > +           sksec->sclass == SECCLASS_TCP_SOCKET ||
> > +           sksec->sclass == SECCLASS_SCTP_SOCKET)
> >                 peer_sid = sksec->peer_sid;
> >         if (peer_sid == SECSID_NULL)
> >                 return -ENOPROTOOPT;
> > @@ -4928,6 +4990,183 @@ static void selinux_sock_graft(struct sock
> > *sk, struct socket *parent)
> >         sksec->sclass = isec->sclass;
> >  }
> > 
> > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> > +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
> > +                                     struct sk_buff *skb,
> > +                                     int sctp_cid)
> > +{
> > +       struct sk_security_struct *sksec = ep->base.sk-
> > >sk_security;
> > +       struct common_audit_data ad;
> > +       struct lsm_network_audit net = {0,};
> > +       u8 peerlbl_active;
> > +       u32 peer_sid = SECINITSID_UNLABELED;
> > +       u32 conn_sid;
> > +       int err;
> > +
> > +       if (!selinux_policycap_extsockclass)
> > +               return 0;
> 
> We *may* need to protect a lot of the new SCTP code with a new policy
> capability, I think reusing extsockclass here could be problematic.

I hope not. I will need some direction here as I've not had problems
(yet). 

> 
> > +       peerlbl_active = selinux_peerlbl_enabled();
> > +
> > +       if (peerlbl_active) {
> > +               /* This will return peer_sid = SECSID_NULL if there
> > are
> > +                * no peer labels, see
> > security_net_peersid_resolve().
> > +                */
> > +               err = selinux_skb_peerlbl_sid(skb, ep->base.sk-
> > >sk_family,
> > +                                             &peer_sid);
> > +
> 
> Drop the extra vertical whitespace between the function call and the
> return code check please.

Done

> 
> > +               if (err)
> > +                       return err;
> > +
> > +               if (peer_sid == SECSID_NULL)
> > +                       peer_sid = SECINITSID_UNLABELED;
> > +       }
> > +
> > +       if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> > +               sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> > +
> > +               /* Here as first association on socket. As the peer
> > SID
> > +                * was allowed by peer recv (and the netif/node
> > checks),
> > +                * then it is approved by policy and used as the
> > primary
> > +                * peer SID for getpeercon(3).
> > +                */
> > +               sksec->peer_sid = peer_sid;
> > +       } else if  (sksec->peer_sid != peer_sid) {
> > +               /* Other association peer SIDs are checked to
> > enforce
> > +                * consistency among the peer SIDs.
> > +                */
> > +               ad.type = LSM_AUDIT_DATA_NET;
> > +               ad.u.net = &net;
> > +               ad.u.net->sk = ep->base.sk;
> > +               err = avc_has_perm(sksec->peer_sid, peer_sid,
> > sksec->sclass,
> > +                                  SCTP_SOCKET__ASSOCIATION, &ad);
> 
> Can anyone think of any reason why we would ever want to allow an
> association that doesn't have the same label as the existing
> associations?  Maybe I'm thinking about this wrong, but I can't
> imagine this being a good idea ...

This has been discussed a number of times and evolved to this. Maybe
the reworded bullet 2) in SELinux-sctp.txt clarifies this:

As SCTP can support more than one transport address per endpoint
(multi-homing) on a single socket, it is possible to configure policy
and NetLabel to provide different peer labels for each of these. As the
socket peer label is determined by the first associations transport
address, it is recommended that all peer labels are consistent.

> 
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       if (sctp_cid == SCTP_CID_INIT) {
> > +               /* Have INIT when incoming connect(2),
> > sctp_connectx(3)
> > +                * or sctp_sendmsg(3) (with no association already
> > present),
> > +                * so compute the MLS component for the connection
> > and store
> > +                * the information in ep. This will be used by SCTP
> > TCP type
> > +                * sockets and peeled off connections as they cause
> > a new
> > +                * socket to be generated. selinux_sctp_sk_clone()
> > will then
> > +                * plug this into the new socket.
> > +                */
> > +               err = selinux_conn_sid(sksec->sid, peer_sid,
> > &conn_sid);
> > +               if (err)
> > +                       return err;
> > +
> > +               ep->secid = conn_sid;
> > +               ep->peer_secid = peer_sid;
> > +
> > +               /* Set any NetLabel labels including CIPSO/CALIPSO
> > options. */
> > +               return selinux_netlbl_sctp_assoc_request(ep, skb);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Check if sctp IPv4/IPv6 addresses are valid for binding or
> > connecting
> > + * based on their @optname.
> > + */
> > +static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> > +                                    struct sockaddr *address,
> > +                                    int addrlen)
> > +{
> > +       int len, err = 0, walk_size = 0;
> > +       void *addr_buf;
> > +       struct sockaddr *addr;
> > +       struct socket *sock;
> > +
> > +       if (!selinux_policycap_extsockclass)
> > +               return 0;
> > +
> > +       switch (optname) {
> > +       case SCTP_SOCKOPT_BINDX_ADD:
> > +               err = sock_has_perm(sk, SCTP_SOCKET__BINDX);
> > +               break;
> > +       case SCTP_SOCKOPT_CONNECTX:
> > +               err = sock_has_perm(sk, SCTP_SOCKET__CONNECTX);
> > +               break;
> > +       /* These need SOCKET__BIND or SOCKET__CONNECT permissions
> > that will
> > +        * be checked later.
> > +        */
> > +       case SCTP_PRIMARY_ADDR:
> > +       case SCTP_SET_PEER_PRIMARY_ADDR:
> > +       case SCTP_PARAM_SET_PRIMARY:
> > +       case SCTP_PARAM_ADD_IP:
> > +       case SCTP_SENDMSG_CONNECT:
> > +               break;
> > +       default:
> > +               err = -EINVAL;
> > +       }
> > +       if (err)
> > +               return err;
> > +
> > +       /* Process one or more addresses that may be IPv4 or IPv6
> > */
> > +       sock = sk->sk_socket;
> > +       addr_buf = address;
> > +
> > +       while (walk_size < addrlen) {
> > +               addr = addr_buf;
> > +               switch (addr->sa_family) {
> > +               case AF_INET:
> > +                       len = sizeof(struct sockaddr_in);
> > +                       break;
> > +               case AF_INET6:
> > +                       len = sizeof(struct sockaddr_in6);
> > +                       break;
> > +               default:
> > +                       return -EAFNOSUPPORT;
> > +               }
> > +
> > +               err = -EINVAL;
> > +               switch (optname) {
> > +               /* Bind checks */
> > +               case SCTP_PRIMARY_ADDR:
> > +               case SCTP_SET_PEER_PRIMARY_ADDR:
> > +               case SCTP_SOCKOPT_BINDX_ADD:
> > +                       err = selinux_socket_bind(sock, addr, len);
> > +                       break;
> > +               /* Connect checks */
> > +               case SCTP_SOCKOPT_CONNECTX:
> > +               case SCTP_PARAM_SET_PRIMARY:
> > +               case SCTP_PARAM_ADD_IP:
> > +               case SCTP_SENDMSG_CONNECT:
> > +                       err = selinux_socket_connect(sock, addr,
> > len);
> > +                       break;
> > +               }
> > +
> > +               if (err)
> > +                       return err;
> > +
> > +               addr_buf += len;
> > +               walk_size += len;
> > +       }
> > +       return 0;
> > +}
> > +
> > +/* Called whenever a new socket is created by accept(2) or
> > sctp_peeloff(3). */
> > +static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct
> > sock *sk,
> > +                                 struct sock *newsk)
> > +{
> > +       struct sk_security_struct *sksec = sk->sk_security;
> > +       struct sk_security_struct *newsksec = newsk->sk_security;
> > +
> > +       /* If policy does not support SECCLASS_SCTP_SOCKET then
> > call
> > +        * the non-sctp clone version.
> > +        */
> > +       if (!selinux_policycap_extsockclass)
> > +               return selinux_sk_clone_security(sk, newsk);
> > +
> > +       newsksec->sid = ep->secid;
> > +       newsksec->peer_sid = ep->peer_secid;
> > +       newsksec->sclass = sksec->sclass;
> > +       newsksec->nlbl_state = sksec->nlbl_state;
> > +}
> > +
> >  static int selinux_inet_conn_request(struct sock *sk, struct
> > sk_buff *skb,
> >                                      struct request_sock *req)
> >  {
> > @@ -6416,6 +6655,9 @@ static struct security_hook_list
> > selinux_hooks[] __lsm_ro_after_init = {
> >         LSM_HOOK_INIT(sk_clone_security,
> > selinux_sk_clone_security),
> >         LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
> >         LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
> > +       LSM_HOOK_INIT(sctp_assoc_request,
> > selinux_sctp_assoc_request),
> > +       LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
> > +       LSM_HOOK_INIT(sctp_bind_connect,
> > selinux_sctp_bind_connect),
> >         LSM_HOOK_INIT(inet_conn_request,
> > selinux_inet_conn_request),
> >         LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
> >         LSM_HOOK_INIT(inet_conn_established,
> > selinux_inet_conn_established),
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index b9fe343..b4b10da 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -173,7 +173,8 @@ struct security_class_mapping secclass_map[] =
> > {
> >           { COMMON_CAP2_PERMS, NULL } },
> >         { "sctp_socket",
> >           { COMMON_SOCK_PERMS,
> > -           "node_bind", NULL } },
> > +           "node_bind", "name_connect", "association", "bindx",
> > +           "connectx", NULL } },
> >         { "icmp_socket",
> >           { COMMON_SOCK_PERMS,
> >             "node_bind", NULL } },
> > diff --git a/security/selinux/include/netlabel.h
> > b/security/selinux/include/netlabel.h
> > index 75686d5..835a0d6 100644
> > --- a/security/selinux/include/netlabel.h
> > +++ b/security/selinux/include/netlabel.h
> > @@ -33,6 +33,7 @@
> >  #include <linux/skbuff.h>
> >  #include <net/sock.h>
> >  #include <net/request_sock.h>
> > +#include <net/sctp/structs.h>
> > 
> >  #include "avc.h"
> >  #include "objsec.h"
> > @@ -53,7 +54,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff
> > *skb,
> >  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> >                                  u16 family,
> >                                  u32 sid);
> > -
> > +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> > +                                    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);
> > @@ -114,6 +116,11 @@ static inline int
> > selinux_netlbl_conn_setsid(struct sock *sk,
> >         return 0;
> >  }
> > 
> > +static inline int selinux_netlbl_sctp_assoc_request(struct
> > sctp_endpoint *ep,
> > +                                                   struct sk_buff
> > *skb)
> > +{
> > +       return 0;
> > +}
> >  static inline int selinux_netlbl_inet_conn_request(struct
> > request_sock *req,
> >                                                    u16 family)
> >  {
> > diff --git a/security/selinux/include/objsec.h
> > b/security/selinux/include/objsec.h
> > index 6ebc61e..660f270 100644
> > --- a/security/selinux/include/objsec.h
> > +++ b/security/selinux/include/objsec.h
> > @@ -130,6 +130,11 @@ struct sk_security_struct {
> >         u32 sid;                        /* SID of this object */
> >         u32 peer_sid;                   /* SID of peer */
> >         u16 sclass;                     /* sock security class */
> > +
> 
> Extra vertical whitespace is not needed.

Done
> 
> > +       enum {                          /* SCTP association state
> > */
> > +               SCTP_ASSOC_UNSET = 0,
> > +               SCTP_ASSOC_SET,
> > +       } sctp_assoc_state;
> >  };
> > 
> >  struct tun_security_struct {
> > diff --git a/security/selinux/netlabel.c
> > b/security/selinux/netlabel.c
> > index aaba667..7d5aa15 100644
> > --- a/security/selinux/netlabel.c
> > +++ b/security/selinux/netlabel.c
> > @@ -250,6 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff
> > *skb,
> >         sk = skb_to_full_sk(skb);
> >         if (sk != NULL) {
> >                 struct sk_security_struct *sksec = sk->sk_security;
> > +
> >                 if (sksec->nlbl_state != NLBL_REQSKB)
> >                         return 0;
> >                 secattr = selinux_netlbl_sock_getattr(sk, sid);
> > @@ -271,6 +272,41 @@ int selinux_netlbl_skbuff_setsid(struct
> > sk_buff *skb,
> >  }
> > 
> >  /**
> > + * selinux_netlbl_sctp_assoc_request - Label an incoming sctp
> > association.
> > + * @ep: incoming association endpoint.
> > + * @skb: the packet.
> > + *
> > + * Description:
> > + * A new incoming connection is represented by @ep, ......
> > + * Returns zero on success, negative values on failure.
> > + *
> > + */
> > +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> > +                                    struct sk_buff *skb)
> > +{
> > +       int rc;
> > +       struct netlbl_lsm_secattr secattr;
> > +       struct sk_security_struct *sksec = ep->base.sk-
> > >sk_security;
> > +
> > +       if (ep->base.sk->sk_family != PF_INET &&
> > +                               ep->base.sk->sk_family != PF_INET6)
> > +               return 0;
> > +
> > +       netlbl_secattr_init(&secattr);
> > +       rc = security_netlbl_sid_to_secattr(ep->secid, &secattr);
> > +       if (rc != 0)
> > +               goto assoc_request_return;
> > +
> > +       rc = netlbl_sctp_setattr(ep->base.sk, skb, &secattr);
> > +       if (rc == 0)
> > +               sksec->nlbl_state = NLBL_LABELED;
> > +
> > +assoc_request_return:
> > +       netlbl_secattr_destroy(&secattr);
> > +       return rc;
> > +}
> > +
> > +/**
> >   * selinux_netlbl_inet_conn_request - Label an incoming stream
> > connection
> >   * @req: incoming connection request socket
> >   *
> > @@ -481,7 +517,7 @@ int selinux_netlbl_socket_setsockopt(struct
> > socket *sock,
> >   */
> >  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr
> > *addr)
> >  {
> > -       int rc;
> > +       int rc, already_owned_by_user = 0;
> >         struct sk_security_struct *sksec = sk->sk_security;
> >         struct netlbl_lsm_secattr *secattr;
> > 
> > @@ -489,7 +525,16 @@ int selinux_netlbl_socket_connect(struct sock
> > *sk, struct sockaddr *addr)
> >             sksec->nlbl_state != NLBL_CONNLABELED)
> >                 return 0;
> > 
> > -       lock_sock(sk);
> > +       /* Note: When called via connect(2) this happens before the
> > socket
> > +        * protocol layer connect operation and @sk is not locked,
> > HOWEVER,
> > +        * when called by the SCTP protocol layer via
> > sctp_connectx(3),
> > +        * sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore
> > check if
> > +        * @sk owned already.
> > +        */
> > +       if (sock_owned_by_user(sk) && sksec->sclass ==
> > SECCLASS_SCTP_SOCKET)
> > +               already_owned_by_user = 1;
> > +       else
> > +               lock_sock(sk);
> > 
> >         /* connected sockets are allowed to disconnect when the
> > address family
> >          * is set to AF_UNSPEC, if that is what is happening we
> > want to reset
> > @@ -510,6 +555,7 @@ int selinux_netlbl_socket_connect(struct sock
> > *sk, struct sockaddr *addr)
> >                 sksec->nlbl_state = NLBL_CONNLABELED;
> > 
> >  socket_connect_return:
> > -       release_sock(sk);
> > +       if (!already_owned_by_user)
> > +               release_sock(sk);
> >         return rc;
> >  }
> > --
> > 2.13.6
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Nov. 13, 2017, 10:40 p.m. UTC | #7

On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
<[e-mail]> wrote:
> On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
>> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
>> <[e-mail]> wrote:
>> > The SELinux SCTP implementation is explained in:
>> > Documentation/security/SELinux-sctp.txt
>> >
>> > Signed-off-by: Richard Haines <[e-mail]>
>> > ---
>> >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
>> >  security/selinux/hooks.c                | 268
>> > ++++++++++++++++++++++++++++++--
>> >  security/selinux/include/classmap.h     |   3 +-
>> >  security/selinux/include/netlabel.h     |   9 +-
>> >  security/selinux/include/objsec.h       |   5 +
>> >  security/selinux/netlabel.c             |  52 ++++++-
>> >  6 files changed, 427 insertions(+), 18 deletions(-)
>> >  create mode 100644 Documentation/security/SELinux-sctp.txt

...

>> > +Policy Statements
>> > +==================
>> > +The following class and permissions to support SCTP are available
>> > within the
>> > +kernel:
>> > +    class sctp_socket inherits socket { node_bind }
>> > +
>> > +whenever the following policy capability is enabled:
>> > +    policycap extended_socket_class;
>> > +
>> > +The SELinux SCTP support adds the additional permissions that are
>> > explained
>> > +in the sections below:
>> > +    association bindx connectx
>>
>> Is the distinction between bind and bindx significant?  The same
>> question applies to connect/connectx.  I think we can probably just
>> reuse bind and connect in these cases.
>
> This has been discussed before with Marcelo and keeping bindx/connectx
> is a useful distinction.

My apologies, I must have forgotten/missed that discussion.  Do you
have an archive pointer?

>> > +SCTP Peer Labeling
>> > +===================
>> > +An SCTP socket will only have one peer label assigned to it. This
>> > will be
>> > +assigned during the establishment of the first association. Once
>> > the peer
>> > +label has been assigned, any new associations will have the
>> > "association"
>> > +permission validated by checking the socket peer sid against the
>> > received
>> > +packets peer sid to determine whether the association should be
>> > allowed or
>> > +denied.
>> > +
>> > +NOTES:
>> > +   1) If peer labeling is not enabled, then the peer context will
>> > always be
>> > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
>> > +
>> > +   2) As SCTP supports multiple endpoints with multi-homing on a
>> > single socket
>> > +      it is recommended that peer labels are consistent.
>>
>> My apologies if I'm confused, but I thought it was multiple
>> associations per-endpoint, with the associations providing the
>> multi-homing functionality, no?
>
> I've reworded to:
> As SCTP can support more than one transport address per endpoint
> (multi-homing) on a single socket, it is possible to configure policy
> and NetLabel to provide different peer labels for each of these. As the
> socket peer label is determined by the first associations transport
> address, it is recommended that all peer labels are consistent.

I'm still not sure this makes complete sense to me, but since I'm
still not 100% confident in my understanding of SCTP I'm willing to
punt on this for the moment.

>> > +   3) getpeercon(3) may be used by userspace to retrieve the
>> > sockets peer
>> > +       context.
>> > +
>> > +   4) If using NetLabel be aware that if a label is assigned to a
>> > specific
>> > +      interface, and that interface 'goes down', then the NetLabel
>> > service
>> > +      will remove the entry. Therefore ensure that the network
>> > startup scripts
>> > +      call netlabelctl(8) to set the required label (see netlabel-
>> > config(8)
>> > +      helper script for details).
>>
>> Maybe this will be made clear as I work my way through this patch,
>> but
>> how is point #4 SCTP specific?
>
> It's not, I added this as a useful hint as I keep forgetting about it,
> I'll reword to: While not SCTP specific, be aware .....

Okay.  Better.

>> > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
>> > +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
>> > +                                     struct sk_buff *skb,
>> > +                                     int sctp_cid)
>> > +{
>> > +       struct sk_security_struct *sksec = ep->base.sk-
>> > >sk_security;
>> > +       struct common_audit_data ad;
>> > +       struct lsm_network_audit net = {0,};
>> > +       u8 peerlbl_active;
>> > +       u32 peer_sid = SECINITSID_UNLABELED;
>> > +       u32 conn_sid;
>> > +       int err;
>> > +
>> > +       if (!selinux_policycap_extsockclass)
>> > +               return 0;
>>
>> We *may* need to protect a lot of the new SCTP code with a new policy
>> capability, I think reusing extsockclass here could be problematic.
>
> I hope not. I will need some direction here as I've not had problems
> (yet).

It's actually not that bad, take a look at the NNP/nosuid patch from
Stephen, af63f4193f9f ("selinux: Generalize support for NNP/nosuid
SELinux domain transitions"), pay attention to the
"selinux_policycap_nnp_nosuid_transition" variable.

>> > +               if (err)
>> > +                       return err;
>> > +
>> > +               if (peer_sid == SECSID_NULL)
>> > +                       peer_sid = SECINITSID_UNLABELED;
>> > +       }
>> > +
>> > +       if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
>> > +               sksec->sctp_assoc_state = SCTP_ASSOC_SET;
>> > +
>> > +               /* Here as first association on socket. As the peer
>> > SID
>> > +                * was allowed by peer recv (and the netif/node
>> > checks),
>> > +                * then it is approved by policy and used as the
>> > primary
>> > +                * peer SID for getpeercon(3).
>> > +                */
>> > +               sksec->peer_sid = peer_sid;
>> > +       } else if  (sksec->peer_sid != peer_sid) {
>> > +               /* Other association peer SIDs are checked to
>> > enforce
>> > +                * consistency among the peer SIDs.
>> > +                */
>> > +               ad.type = LSM_AUDIT_DATA_NET;
>> > +               ad.u.net = &net;
>> > +               ad.u.net->sk = ep->base.sk;
>> > +               err = avc_has_perm(sksec->peer_sid, peer_sid,
>> > sksec->sclass,
>> > +                                  SCTP_SOCKET__ASSOCIATION, &ad);
>>
>> Can anyone think of any reason why we would ever want to allow an
>> association that doesn't have the same label as the existing
>> associations?  Maybe I'm thinking about this wrong, but I can't
>> imagine this being a good idea ...
>
> This has been discussed a number of times and evolved to this ...

Yes, I think my comment was the result of faulty SCTP understanding on my part.

Stephen Smalley Nov. 14, 2017, 1:41 p.m. UTC | #8

On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
> <[e-mail]> wrote:
> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> > > <[e-mail]> wrote:
> > > > The SELinux SCTP implementation is explained in:
> > > > Documentation/security/SELinux-sctp.txt
> > > > 
> > > > Signed-off-by: Richard Haines <[e-mail]>
> > > > ---
> > > >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
> > > >  security/selinux/hooks.c                | 268
> > > > ++++++++++++++++++++++++++++++--
> > > >  security/selinux/include/classmap.h     |   3 +-
> > > >  security/selinux/include/netlabel.h     |   9 +-
> > > >  security/selinux/include/objsec.h       |   5 +
> > > >  security/selinux/netlabel.c             |  52 ++++++-
> > > >  6 files changed, 427 insertions(+), 18 deletions(-)
> > > >  create mode 100644 Documentation/security/SELinux-sctp.txt
> 
> ...
> 
> > > > +Policy Statements
> > > > +==================
> > > > +The following class and permissions to support SCTP are
> > > > available
> > > > within the
> > > > +kernel:
> > > > +    class sctp_socket inherits socket { node_bind }
> > > > +
> > > > +whenever the following policy capability is enabled:
> > > > +    policycap extended_socket_class;
> > > > +
> > > > +The SELinux SCTP support adds the additional permissions that
> > > > are
> > > > explained
> > > > +in the sections below:
> > > > +    association bindx connectx
> > > 
> > > Is the distinction between bind and bindx significant?  The same
> > > question applies to connect/connectx.  I think we can probably
> > > just
> > > reuse bind and connect in these cases.
> > 
> > This has been discussed before with Marcelo and keeping
> > bindx/connectx
> > is a useful distinction.
> 
> My apologies, I must have forgotten/missed that discussion.  Do you
> have an archive pointer?
> 
> > > > +SCTP Peer Labeling
> > > > +===================
> > > > +An SCTP socket will only have one peer label assigned to it.
> > > > This
> > > > will be
> > > > +assigned during the establishment of the first association.
> > > > Once
> > > > the peer
> > > > +label has been assigned, any new associations will have the
> > > > "association"
> > > > +permission validated by checking the socket peer sid against
> > > > the
> > > > received
> > > > +packets peer sid to determine whether the association should
> > > > be
> > > > allowed or
> > > > +denied.
> > > > +
> > > > +NOTES:
> > > > +   1) If peer labeling is not enabled, then the peer context
> > > > will
> > > > always be
> > > > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> > > > +
> > > > +   2) As SCTP supports multiple endpoints with multi-homing on
> > > > a
> > > > single socket
> > > > +      it is recommended that peer labels are consistent.
> > > 
> > > My apologies if I'm confused, but I thought it was multiple
> > > associations per-endpoint, with the associations providing the
> > > multi-homing functionality, no?
> > 
> > I've reworded to:
> > As SCTP can support more than one transport address per endpoint
> > (multi-homing) on a single socket, it is possible to configure
> > policy
> > and NetLabel to provide different peer labels for each of these. As
> > the
> > socket peer label is determined by the first associations transport
> > address, it is recommended that all peer labels are consistent.
> 
> I'm still not sure this makes complete sense to me, but since I'm
> still not 100% confident in my understanding of SCTP I'm willing to
> punt on this for the moment.
> 
> > > > +   3) getpeercon(3) may be used by userspace to retrieve the
> > > > sockets peer
> > > > +       context.
> > > > +
> > > > +   4) If using NetLabel be aware that if a label is assigned
> > > > to a
> > > > specific
> > > > +      interface, and that interface 'goes down', then the
> > > > NetLabel
> > > > service
> > > > +      will remove the entry. Therefore ensure that the network
> > > > startup scripts
> > > > +      call netlabelctl(8) to set the required label (see
> > > > netlabel-
> > > > config(8)
> > > > +      helper script for details).
> > > 
> > > Maybe this will be made clear as I work my way through this
> > > patch,
> > > but
> > > how is point #4 SCTP specific?
> > 
> > It's not, I added this as a useful hint as I keep forgetting about
> > it,
> > I'll reword to: While not SCTP specific, be aware .....
> 
> Okay.  Better.
> 
> > > > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> > > > +static int selinux_sctp_assoc_request(struct sctp_endpoint
> > > > *ep,
> > > > +                                     struct sk_buff *skb,
> > > > +                                     int sctp_cid)
> > > > +{
> > > > +       struct sk_security_struct *sksec = ep->base.sk-
> > > > > sk_security;
> > > > 
> > > > +       struct common_audit_data ad;
> > > > +       struct lsm_network_audit net = {0,};
> > > > +       u8 peerlbl_active;
> > > > +       u32 peer_sid = SECINITSID_UNLABELED;
> > > > +       u32 conn_sid;
> > > > +       int err;
> > > > +
> > > > +       if (!selinux_policycap_extsockclass)
> > > > +               return 0;
> > > 
> > > We *may* need to protect a lot of the new SCTP code with a new
> > > policy
> > > capability, I think reusing extsockclass here could be
> > > problematic.
> > 
> > I hope not. I will need some direction here as I've not had
> > problems
> > (yet).
> 
> It's actually not that bad, take a look at the NNP/nosuid patch from
> Stephen, af63f4193f9f ("selinux: Generalize support for NNP/nosuid
> SELinux domain transitions"), pay attention to the
> "selinux_policycap_nnp_nosuid_transition" variable.

AFAICT, Fedora has not yet enabled extended_socket_class policy
capability in their policy, not even in rawhide, so no breakage should
occur in Fedora.  Upstream refpolicy did enable it in the 20170805
release, so it could possibly be enabled in other distributions, but
only if using the latest refpolicy + libsepol.  Android enabled it in
the Android 8.0 policy, although few Android kernels are likely to
include the support yet (Android common kernel appears to be <= 4.9),
so it should presently be a no-op.

> 
> > > > +               if (err)
> > > > +                       return err;
> > > > +
> > > > +               if (peer_sid == SECSID_NULL)
> > > > +                       peer_sid = SECINITSID_UNLABELED;
> > > > +       }
> > > > +
> > > > +       if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> > > > +               sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> > > > +
> > > > +               /* Here as first association on socket. As the
> > > > peer
> > > > SID
> > > > +                * was allowed by peer recv (and the netif/node
> > > > checks),
> > > > +                * then it is approved by policy and used as
> > > > the
> > > > primary
> > > > +                * peer SID for getpeercon(3).
> > > > +                */
> > > > +               sksec->peer_sid = peer_sid;
> > > > +       } else if  (sksec->peer_sid != peer_sid) {
> > > > +               /* Other association peer SIDs are checked to
> > > > enforce
> > > > +                * consistency among the peer SIDs.
> > > > +                */
> > > > +               ad.type = LSM_AUDIT_DATA_NET;
> > > > +               ad.u.net = &net;
> > > > +               ad.u.net->sk = ep->base.sk;
> > > > +               err = avc_has_perm(sksec->peer_sid, peer_sid,
> > > > sksec->sclass,
> > > > +                                  SCTP_SOCKET__ASSOCIATION,
> > > > &ad);
> > > 
> > > Can anyone think of any reason why we would ever want to allow an
> > > association that doesn't have the same label as the existing
> > > associations?  Maybe I'm thinking about this wrong, but I can't
> > > imagine this being a good idea ...
> > 
> > This has been discussed a number of times and evolved to this ...
> 
> Yes, I think my comment was the result of faulty SCTP understanding
> on my part.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Richard Haines Nov. 14, 2017, 9:52 p.m. UTC | #9

On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
> <[e-mail]> wrote:
> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> > > <[e-mail]> wrote:
> > > > The SELinux SCTP implementation is explained in:
> > > > Documentation/security/SELinux-sctp.txt
> > > > 
> > > > Signed-off-by: Richard Haines <[e-mail]>
> > > > ---
> > > >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
> > > >  security/selinux/hooks.c                | 268
> > > > ++++++++++++++++++++++++++++++--
> > > >  security/selinux/include/classmap.h     |   3 +-
> > > >  security/selinux/include/netlabel.h     |   9 +-
> > > >  security/selinux/include/objsec.h       |   5 +
> > > >  security/selinux/netlabel.c             |  52 ++++++-
> > > >  6 files changed, 427 insertions(+), 18 deletions(-)
> > > >  create mode 100644 Documentation/security/SELinux-sctp.txt
> 
> ...
> 
> > > > +Policy Statements
> > > > +==================
> > > > +The following class and permissions to support SCTP are
> > > > available
> > > > within the
> > > > +kernel:
> > > > +    class sctp_socket inherits socket { node_bind }
> > > > +
> > > > +whenever the following policy capability is enabled:
> > > > +    policycap extended_socket_class;
> > > > +
> > > > +The SELinux SCTP support adds the additional permissions that
> > > > are
> > > > explained
> > > > +in the sections below:
> > > > +    association bindx connectx
> > > 
> > > Is the distinction between bind and bindx significant?  The same
> > > question applies to connect/connectx.  I think we can probably
> > > just
> > > reuse bind and connect in these cases.
> > 
> > This has been discussed before with Marcelo and keeping
> > bindx/connectx
> > is a useful distinction.
> 
> My apologies, I must have forgotten/missed that discussion.  Do you
> have an archive pointer?

No this was off list, however I've copied the relevant bits:

> SCTP Socket Option Permissions
> ===============================
> Permissions that are validated on setsockopt(2) calls (note that the
> sctp_socket SETOPT permission must be allowed):
>
> This option requires the BINDX_ADDR permission:
> SCTP_SOCKOPT_BINDX_REM - Remove additional bind address.

Can't see an usage for this one.

>
> These options require the SET_PARAMS permission:
> SCTP_PEER_ADDR_PARAMS  - Set heartbeats and address max
> retransmissions.
> SCTP_PEER_ADDR_THLDS  - Set thresholds.
> SCTP_ASSOCINFO        - Set association / endpoint parameters.

Also for these, considering we are not willing to go as deep as to only
allow these if within a given threshold. But still even then, sounds
like too much.

>
>
> SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> ==============================================================
> The hook security_sctp_addr_list() is called by SCTP when processing
> various options (@optname) to check permissions required for the list
> of ipv4/ipv6 addresses (@address) as follows:
> --------------------------------------------------------------------
> |                sctp_socket BIND type permission checks          |
> |            (The socket must also have the BIND permission)      |
> |      @optname            | Permission  |  @address              |
> |--------------------------|-------------|-------------------------|
> |SCTP_SOCKOPT_BINDX_ADD    |BINDX_ADDRS  |One or more ipv4/ipv6 adr|

This one can be useful, for that privilege-dropping case.

Paul note: I later changed BINDX_ADDRS to just BINDX

> |SCTP_PRIMARY_ADDR        |SET_PRI_ADDR |Single ipv4 or ipv6 adr  |
> |SCTP_SET_PEER_PRIMARY_ADDR|SET_PEER_ADDR|Single ipv4 or ipv6 adr  |

But these, can't use an use-case.

> --------------------------------------------------------------------
> --------------------------------------------------------------------
> |                sctp_socket CONNECT type permission checks        |
> |            (The socket must also have the CONNECT permission)    |
> |      @optname            | Permission  |  @address              |
> |--------------------------|-------------|-------------------------|
> |SCTP_SOCKOPT_CONNECTX    |CONNECTX    |One or more ipv4/ipv6 adr|
> |SCTP_PARAM_ADD_IP        |BINDX_ADDRS  |One or more ipv4/ipv6 adr|

The 2 above, can be useful.

> |SCTP_PARAM_DEL_IP        |BINDX_ADDRS  |One or more ipv4/ipv6 adr|
> |SCTP_PARAM_SET_PRIMARY    |SET_PRI_ADDR |Single ipv4 or ipv6 adr  |

But not these two..

> --------------------------------------------------------------------
>
> SCTP_SOCKOPT_BINDX_ADD - Allows additional bind addresses to be
> associated after (optionally) calling
> bind(3).
> sctp_bindx(3) adds a set of bind
> addresses on a socket.
>
> SCTP_SOCKOPT_CONNECTX - Allows the allocation of multiple
> addresses for reaching a peer
> (multi-homed).
> sctp_connectx(3) initiates a connection
> on an SCTP socket using multiple
> destination addresses.
>
> SCTP_PRIMARY_ADDR    - Set local primary address.
>
> SCTP_SET_PEER_PRIMARY_ADDR - Request peer sets address as
> association primary.

No and no for the 2 above.



> 
> > > > +SCTP Peer Labeling
> > > > +===================
> > > > +An SCTP socket will only have one peer label assigned to it.
> > > > This
> > > > will be
> > > > +assigned during the establishment of the first association.
> > > > Once
> > > > the peer
> > > > +label has been assigned, any new associations will have the
> > > > "association"
> > > > +permission validated by checking the socket peer sid against
> > > > the
> > > > received
> > > > +packets peer sid to determine whether the association should
> > > > be
> > > > allowed or
> > > > +denied.
> > > > +
> > > > +NOTES:
> > > > +   1) If peer labeling is not enabled, then the peer context
> > > > will
> > > > always be
> > > > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> > > > +
> > > > +   2) As SCTP supports multiple endpoints with multi-homing on
> > > > a
> > > > single socket
> > > > +      it is recommended that peer labels are consistent.
> > > 
> > > My apologies if I'm confused, but I thought it was multiple
> > > associations per-endpoint, with the associations providing the
> > > multi-homing functionality, no?
> > 
> > I've reworded to:
> > As SCTP can support more than one transport address per endpoint
> > (multi-homing) on a single socket, it is possible to configure
> > policy
> > and NetLabel to provide different peer labels for each of these. As
> > the
> > socket peer label is determined by the first associations transport
> > address, it is recommended that all peer labels are consistent.
> 
> I'm still not sure this makes complete sense to me, but since I'm
> still not 100% confident in my understanding of SCTP I'm willing to
> punt on this for the moment.

I would like you to be happy with this so I've added what I think is
some clarification.

The code that handles this is the selinux_sctp_assoc_request() in
hooks.c. If not the first association, then the other association peer
SIDs are validated to enforce consistency among the peer SIDs. However
what I recommend is that all peer labels should be the same. For
example:

Don't configure this (although valid):
netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
    label:system_u:object_r:netlabel_peer_lo_t:s0
netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
    label:system_u:object_r:netlabel_peer_wlan_t:s0
netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
    label:system_u:object_r:netlabel_peer_eth_t:s0

As this is recommended:
netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
    label:system_u:object_r:netlabel_peer_t:s0
netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
    label:system_u:object_r:netlabel_peer_t:s0
netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
    label:system_u:object_r:netlabel_peer_t:s0

Would you prefer me to delete this section ?
> 
> > > > +   3) getpeercon(3) may be used by userspace to retrieve the
> > > > sockets peer
> > > > +       context.
> > > > +
> > > > +   4) If using NetLabel be aware that if a label is assigned
> > > > to a
> > > > specific
> > > > +      interface, and that interface 'goes down', then the
> > > > NetLabel
> > > > service
> > > > +      will remove the entry. Therefore ensure that the network
> > > > startup scripts
> > > > +      call netlabelctl(8) to set the required label (see
> > > > netlabel-
> > > > config(8)
> > > > +      helper script for details).
> > > 
> > > Maybe this will be made clear as I work my way through this
> > > patch,
> > > but
> > > how is point #4 SCTP specific?
> > 
> > It's not, I added this as a useful hint as I keep forgetting about
> > it,
> > I'll reword to: While not SCTP specific, be aware .....
> 
> Okay.  Better.
> 
> > > > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> > > > +static int selinux_sctp_assoc_request(struct sctp_endpoint
> > > > *ep,
> > > > +                                     struct sk_buff *skb,
> > > > +                                     int sctp_cid)
> > > > +{
> > > > +       struct sk_security_struct *sksec = ep->base.sk-
> > > > > sk_security;
> > > > 
> > > > +       struct common_audit_data ad;
> > > > +       struct lsm_network_audit net = {0,};
> > > > +       u8 peerlbl_active;
> > > > +       u32 peer_sid = SECINITSID_UNLABELED;
> > > > +       u32 conn_sid;
> > > > +       int err;
> > > > +
> > > > +       if (!selinux_policycap_extsockclass)
> > > > +               return 0;
> > > 
> > > We *may* need to protect a lot of the new SCTP code with a new
> > > policy
> > > capability, I think reusing extsockclass here could be
> > > problematic.
> > 
> > I hope not. I will need some direction here as I've not had
> > problems
> > (yet).
> 
> It's actually not that bad, take a look at the NNP/nosuid patch from
> Stephen, af63f4193f9f ("selinux: Generalize support for NNP/nosuid
> SELinux domain transitions"), pay attention to the
> "selinux_policycap_nnp_nosuid_transition" variable.
> 
> > > > +               if (err)
> > > > +                       return err;
> > > > +
> > > > +               if (peer_sid == SECSID_NULL)
> > > > +                       peer_sid = SECINITSID_UNLABELED;
> > > > +       }
> > > > +
> > > > +       if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
> > > > +               sksec->sctp_assoc_state = SCTP_ASSOC_SET;
> > > > +
> > > > +               /* Here as first association on socket. As the
> > > > peer
> > > > SID
> > > > +                * was allowed by peer recv (and the netif/node
> > > > checks),
> > > > +                * then it is approved by policy and used as
> > > > the
> > > > primary
> > > > +                * peer SID for getpeercon(3).
> > > > +                */
> > > > +               sksec->peer_sid = peer_sid;
> > > > +       } else if  (sksec->peer_sid != peer_sid) {
> > > > +               /* Other association peer SIDs are checked to
> > > > enforce
> > > > +                * consistency among the peer SIDs.
> > > > +                */
> > > > +               ad.type = LSM_AUDIT_DATA_NET;
> > > > +               ad.u.net = &net;
> > > > +               ad.u.net->sk = ep->base.sk;
> > > > +               err = avc_has_perm(sksec->peer_sid, peer_sid,
> > > > sksec->sclass,
> > > > +                                  SCTP_SOCKET__ASSOCIATION,
> > > > &ad);
> > > 
> > > Can anyone think of any reason why we would ever want to allow an
> > > association that doesn't have the same label as the existing
> > > associations?  Maybe I'm thinking about this wrong, but I can't
> > > imagine this being a good idea ...
> > 
> > This has been discussed a number of times and evolved to this ...
> 
> Yes, I think my comment was the result of faulty SCTP understanding
> on my part.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore Nov. 20, 2017, 9:55 p.m. UTC | #10

On Tue, Nov 14, 2017 at 4:52 PM, Richard Haines
<[e-mail]> wrote:
> On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
>> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
>> <[e-mail]> wrote:
>> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
>> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
>> > > <[e-mail]> wrote:
>> > > > The SELinux SCTP implementation is explained in:
>> > > > Documentation/security/SELinux-sctp.txt
>> > > >
>> > > > Signed-off-by: Richard Haines <[e-mail]>
>> > > > ---
>> > > >  Documentation/security/SELinux-sctp.txt | 108 +++++++++++++
>> > > >  security/selinux/hooks.c                | 268
>> > > > ++++++++++++++++++++++++++++++--
>> > > >  security/selinux/include/classmap.h     |   3 +-
>> > > >  security/selinux/include/netlabel.h     |   9 +-
>> > > >  security/selinux/include/objsec.h       |   5 +
>> > > >  security/selinux/netlabel.c             |  52 ++++++-
>> > > >  6 files changed, 427 insertions(+), 18 deletions(-)
>> > > >  create mode 100644 Documentation/security/SELinux-sctp.txt
>>
>> ...
>>
>> > > > +Policy Statements
>> > > > +==================
>> > > > +The following class and permissions to support SCTP are
>> > > > available
>> > > > within the
>> > > > +kernel:
>> > > > +    class sctp_socket inherits socket { node_bind }
>> > > > +
>> > > > +whenever the following policy capability is enabled:
>> > > > +    policycap extended_socket_class;
>> > > > +
>> > > > +The SELinux SCTP support adds the additional permissions that
>> > > > are
>> > > > explained
>> > > > +in the sections below:
>> > > > +    association bindx connectx
>> > >
>> > > Is the distinction between bind and bindx significant?  The same
>> > > question applies to connect/connectx.  I think we can probably
>> > > just
>> > > reuse bind and connect in these cases.
>> >
>> > This has been discussed before with Marcelo and keeping
>> > bindx/connectx
>> > is a useful distinction.
>>
>> My apologies, I must have forgotten/missed that discussion.  Do you
>> have an archive pointer?
>
> No this was off list, however I've copied the relevant bits:
>
>> SCTP Socket Option Permissions
>> ===============================
>> Permissions that are validated on setsockopt(2) calls (note that the
>> sctp_socket SETOPT permission must be allowed):
>>
>> This option requires the BINDX_ADDR permission:
>> SCTP_SOCKOPT_BINDX_REM - Remove additional bind address.
>
> Can't see an usage for this one.
>
>>
>> These options require the SET_PARAMS permission:
>> SCTP_PEER_ADDR_PARAMS  - Set heartbeats and address max
>> retransmissions.
>> SCTP_PEER_ADDR_THLDS  - Set thresholds.
>> SCTP_ASSOCINFO        - Set association / endpoint parameters.
>
> Also for these, considering we are not willing to go as deep as to only
> allow these if within a given threshold. But still even then, sounds
> like too much.
>
>>
>>
>> SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
>> ==============================================================
>> The hook security_sctp_addr_list() is called by SCTP when processing
>> various options (@optname) to check permissions required for the list
>> of ipv4/ipv6 addresses (@address) as follows:
>> --------------------------------------------------------------------
>> |                sctp_socket BIND type permission checks          |
>> |            (The socket must also have the BIND permission)      |
>> |      @optname            | Permission  |  @address              |
>> |--------------------------|-------------|-------------------------|
>> |SCTP_SOCKOPT_BINDX_ADD    |BINDX_ADDRS  |One or more ipv4/ipv6 adr|
>
> This one can be useful, for that privilege-dropping case.
>
> Paul note: I later changed BINDX_ADDRS to just BINDX
>
>> |SCTP_PRIMARY_ADDR        |SET_PRI_ADDR |Single ipv4 or ipv6 adr  |
>> |SCTP_SET_PEER_PRIMARY_ADDR|SET_PEER_ADDR|Single ipv4 or ipv6 adr  |
>
> But these, can't use an use-case.
>
>> --------------------------------------------------------------------
>> --------------------------------------------------------------------
>> |                sctp_socket CONNECT type permission checks        |
>> |            (The socket must also have the CONNECT permission)    |
>> |      @optname            | Permission  |  @address              |
>> |--------------------------|-------------|-------------------------|
>> |SCTP_SOCKOPT_CONNECTX    |CONNECTX    |One or more ipv4/ipv6 adr|
>> |SCTP_PARAM_ADD_IP        |BINDX_ADDRS  |One or more ipv4/ipv6 adr|
>
> The 2 above, can be useful.
>
>> |SCTP_PARAM_DEL_IP        |BINDX_ADDRS  |One or more ipv4/ipv6 adr|
>> |SCTP_PARAM_SET_PRIMARY    |SET_PRI_ADDR |Single ipv4 or ipv6 adr  |
>
> But not these two..
>
>> --------------------------------------------------------------------
>>
>> SCTP_SOCKOPT_BINDX_ADD - Allows additional bind addresses to be
>> associated after (optionally) calling
>> bind(3).
>> sctp_bindx(3) adds a set of bind
>> addresses on a socket.
>>
>> SCTP_SOCKOPT_CONNECTX - Allows the allocation of multiple
>> addresses for reaching a peer
>> (multi-homed).
>> sctp_connectx(3) initiates a connection
>> on an SCTP socket using multiple
>> destination addresses.
>>
>> SCTP_PRIMARY_ADDR    - Set local primary address.
>>
>> SCTP_SET_PEER_PRIMARY_ADDR - Request peer sets address as
>> association primary.
>
> No and no for the 2 above.

I still don't undetstand how, from a security perspective,
sctp_socket:bindx is different from sctp_socket:bind.  I understand
the distinction is important from a SCTP perspective, but from a
SELinux perspective isn't sctp_socket:bindx simply sctp_socket:bind?
How is bindx different from bind?

The same applies for connect/connectx.

>> > > > +SCTP Peer Labeling
>> > > > +===================
>> > > > +An SCTP socket will only have one peer label assigned to it.
>> > > > This
>> > > > will be
>> > > > +assigned during the establishment of the first association.
>> > > > Once
>> > > > the peer
>> > > > +label has been assigned, any new associations will have the
>> > > > "association"
>> > > > +permission validated by checking the socket peer sid against
>> > > > the
>> > > > received
>> > > > +packets peer sid to determine whether the association should
>> > > > be
>> > > > allowed or
>> > > > +denied.
>> > > > +
>> > > > +NOTES:
>> > > > +   1) If peer labeling is not enabled, then the peer context
>> > > > will
>> > > > always be
>> > > > +      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
>> > > > +
>> > > > +   2) As SCTP supports multiple endpoints with multi-homing on
>> > > > a
>> > > > single socket
>> > > > +      it is recommended that peer labels are consistent.
>> > >
>> > > My apologies if I'm confused, but I thought it was multiple
>> > > associations per-endpoint, with the associations providing the
>> > > multi-homing functionality, no?
>> >
>> > I've reworded to:
>> > As SCTP can support more than one transport address per endpoint
>> > (multi-homing) on a single socket, it is possible to configure
>> > policy
>> > and NetLabel to provide different peer labels for each of these. As
>> > the
>> > socket peer label is determined by the first associations transport
>> > address, it is recommended that all peer labels are consistent.
>>
>> I'm still not sure this makes complete sense to me, but since I'm
>> still not 100% confident in my understanding of SCTP I'm willing to
>> punt on this for the moment.
>
> I would like you to be happy with this so I've added what I think is
> some clarification.
>
> The code that handles this is the selinux_sctp_assoc_request() in
> hooks.c. If not the first association, then the other association peer
> SIDs are validated to enforce consistency among the peer SIDs. However
> what I recommend is that all peer labels should be the same. For
> example:
>
> Don't configure this (although valid):
> netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
>     label:system_u:object_r:netlabel_peer_lo_t:s0
> netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
>     label:system_u:object_r:netlabel_peer_wlan_t:s0
> netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
>     label:system_u:object_r:netlabel_peer_eth_t:s0
>
> As this is recommended:
> netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
>     label:system_u:object_r:netlabel_peer_t:s0
> netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
>     label:system_u:object_r:netlabel_peer_t:s0
> netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
>     label:system_u:object_r:netlabel_peer_t:s0
>
> Would you prefer me to delete this section ?

Okay, I think I'm getting a better idea of this ... let me walk
through some of this, slowly.

On the send/egress side of things we don't really need to worry about
the remote peer label as we only do access control checks using the
sender's label (the local process/socket).  There is a slight
exception with respect to forwarded traffic, but I think we handle
that correctly for SCTP by substituting the traffic's label for the
local process/socket label (current behavior).

On the receive/ingress side we could receive traffic from any of the
associations, with the peer:recv permission checking the remote peer
label (aka the sending association's label) against the local
receiving socket.  This seems right to me.  As far as userspace is
concerned, *_getpeersec_dgram() should work correctly, but
*_getpeersec_stream() will only return the peer label we stored during
the initial association setup, yes?  This seems ... not the best.
Although I'm not sure how to solve this.

Is this why the requirement exists for each association to have an
equivalent label?

Richard Haines Nov. 21, 2017, 5:37 p.m. UTC | #11

On Mon, 2017-11-20 at 16:55 -0500, Paul Moore wrote:
> On Tue, Nov 14, 2017 at 4:52 PM, Richard Haines
> <[e-mail]> wrote:
> > On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
> > > On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
> > > <[e-mail]> wrote:
> > > > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> > > > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> > > > > <[e-mail]> wrote:
> > > > > > The SELinux SCTP implementation is explained in:
> > > > > > Documentation/security/SELinux-sctp.txt
> > > > > > 
> > > > > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.
> > > > > > com>
> > > > > > ---
> > > > > >  Documentation/security/SELinux-sctp.txt | 108
> > > > > > +++++++++++++
> > > > > >  security/selinux/hooks.c                | 268
> > > > > > ++++++++++++++++++++++++++++++--
> > > > > >  security/selinux/include/classmap.h     |   3 +-
> > > > > >  security/selinux/include/netlabel.h     |   9 +-
> > > > > >  security/selinux/include/objsec.h       |   5 +
> > > > > >  security/selinux/netlabel.c             |  52 ++++++-
> > > > > >  6 files changed, 427 insertions(+), 18 deletions(-)
> > > > > >  create mode 100644 Documentation/security/SELinux-sctp.txt
> > > 
> > > ...
> > > 
> > > > > > +Policy Statements
> > > > > > +==================
> > > > > > +The following class and permissions to support SCTP are
> > > > > > available
> > > > > > within the
> > > > > > +kernel:
> > > > > > +    class sctp_socket inherits socket { node_bind }
> > > > > > +
> > > > > > +whenever the following policy capability is enabled:
> > > > > > +    policycap extended_socket_class;
> > > > > > +
> > > > > > +The SELinux SCTP support adds the additional permissions
> > > > > > that
> > > > > > are
> > > > > > explained
> > > > > > +in the sections below:
> > > > > > +    association bindx connectx
> > > > > 
> > > > > Is the distinction between bind and bindx significant?  The
> > > > > same
> > > > > question applies to connect/connectx.  I think we can
> > > > > probably
> > > > > just
> > > > > reuse bind and connect in these cases.
> > > > 
> > > > This has been discussed before with Marcelo and keeping
> > > > bindx/connectx
> > > > is a useful distinction.
> > > 
> > > My apologies, I must have forgotten/missed that discussion.  Do
> > > you
> > > have an archive pointer?
> > 
> > No this was off list, however I've copied the relevant bits:
> > 
> > > SCTP Socket Option Permissions
> > > ===============================
> > > Permissions that are validated on setsockopt(2) calls (note that
> > > the
> > > sctp_socket SETOPT permission must be allowed):
> > > 
> > > This option requires the BINDX_ADDR permission:
> > > SCTP_SOCKOPT_BINDX_REM - Remove additional bind address.
> > 
> > Can't see an usage for this one.
> > 
> > > 
> > > These options require the SET_PARAMS permission:
> > > SCTP_PEER_ADDR_PARAMS  - Set heartbeats and address max
> > > retransmissions.
> > > SCTP_PEER_ADDR_THLDS  - Set thresholds.
> > > SCTP_ASSOCINFO        - Set association / endpoint parameters.
> > 
> > Also for these, considering we are not willing to go as deep as to
> > only
> > allow these if within a given threshold. But still even then,
> > sounds
> > like too much.
> > 
> > > 
> > > 
> > > SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> > > ==============================================================
> > > The hook security_sctp_addr_list() is called by SCTP when
> > > processing
> > > various options (@optname) to check permissions required for the
> > > list
> > > of ipv4/ipv6 addresses (@address) as follows:
> > > ---------------------------------------------------------------
> > > -----
> > > >                sctp_socket BIND type permission
> > > > checks          |
> > > >            (The socket must also have the BIND
> > > > permission)      |
> > > >      @optname            |
> > > > Permission  |  @address              |
> > > > --------------------------|-------------|--------------------
> > > > -----|
> > > > SCTP_SOCKOPT_BINDX_ADD    |BINDX_ADDRS  |One or more ipv4/ipv6
> > > > adr|
> > 
> > This one can be useful, for that privilege-dropping case.
> > 
> > Paul note: I later changed BINDX_ADDRS to just BINDX
> > 
> > > > SCTP_PRIMARY_ADDR        |SET_PRI_ADDR |Single ipv4 or ipv6
> > > > adr  |
> > > > SCTP_SET_PEER_PRIMARY_ADDR|SET_PEER_ADDR|Single ipv4 or ipv6
> > > > adr  |
> > 
> > But these, can't use an use-case.
> > 
> > > ---------------------------------------------------------------
> > > -----
> > > ---------------------------------------------------------------
> > > -----
> > > >                sctp_socket CONNECT type permission
> > > > checks        |
> > > >            (The socket must also have the CONNECT
> > > > permission)    |
> > > >      @optname            |
> > > > Permission  |  @address              |
> > > > --------------------------|-------------|--------------------
> > > > -----|
> > > > SCTP_SOCKOPT_CONNECTX    |CONNECTX    |One or more ipv4/ipv6
> > > > adr|
> > > > SCTP_PARAM_ADD_IP        |BINDX_ADDRS  |One or more ipv4/ipv6
> > > > adr|
> > 
> > The 2 above, can be useful.
> > 
> > > > SCTP_PARAM_DEL_IP        |BINDX_ADDRS  |One or more ipv4/ipv6
> > > > adr|
> > > > SCTP_PARAM_SET_PRIMARY    |SET_PRI_ADDR |Single ipv4 or ipv6
> > > > adr  |
> > 
> > But not these two..
> > 
> > > ---------------------------------------------------------------
> > > -----
> > > 
> > > SCTP_SOCKOPT_BINDX_ADD - Allows additional bind addresses to be
> > > associated after (optionally) calling
> > > bind(3).
> > > sctp_bindx(3) adds a set of bind
> > > addresses on a socket.
> > > 
> > > SCTP_SOCKOPT_CONNECTX - Allows the allocation of multiple
> > > addresses for reaching a peer
> > > (multi-homed).
> > > sctp_connectx(3) initiates a connection
> > > on an SCTP socket using multiple
> > > destination addresses.
> > > 
> > > SCTP_PRIMARY_ADDR    - Set local primary address.
> > > 
> > > SCTP_SET_PEER_PRIMARY_ADDR - Request peer sets address as
> > > association primary.
> > 
> > No and no for the 2 above.
> 
> I still don't undetstand how, from a security perspective,
> sctp_socket:bindx is different from sctp_socket:bind.  I understand
> the distinction is important from a SCTP perspective, but from a
> SELinux perspective isn't sctp_socket:bindx simply sctp_socket:bind?
> How is bindx different from bind?
> 
> The same applies for connect/connectx.

Okay I'll remove the bindx and connectx permissions.

> 
> > > > > > +SCTP Peer Labeling
> > > > > > +===================
> > > > > > +An SCTP socket will only have one peer label assigned to
> > > > > > it.
> > > > > > This
> > > > > > will be
> > > > > > +assigned during the establishment of the first
> > > > > > association.
> > > > > > Once
> > > > > > the peer
> > > > > > +label has been assigned, any new associations will have
> > > > > > the
> > > > > > "association"
> > > > > > +permission validated by checking the socket peer sid
> > > > > > against
> > > > > > the
> > > > > > received
> > > > > > +packets peer sid to determine whether the association
> > > > > > should
> > > > > > be
> > > > > > allowed or
> > > > > > +denied.
> > > > > > +
> > > > > > +NOTES:
> > > > > > +   1) If peer labeling is not enabled, then the peer
> > > > > > context
> > > > > > will
> > > > > > always be
> > > > > > +      SECINITSID_UNLABELED (unlabeled_t in Reference
> > > > > > Policy).
> > > > > > +
> > > > > > +   2) As SCTP supports multiple endpoints with multi-
> > > > > > homing on
> > > > > > a
> > > > > > single socket
> > > > > > +      it is recommended that peer labels are consistent.
> > > > > 
> > > > > My apologies if I'm confused, but I thought it was multiple
> > > > > associations per-endpoint, with the associations providing
> > > > > the
> > > > > multi-homing functionality, no?
> > > > 
> > > > I've reworded to:
> > > > As SCTP can support more than one transport address per
> > > > endpoint
> > > > (multi-homing) on a single socket, it is possible to configure
> > > > policy
> > > > and NetLabel to provide different peer labels for each of
> > > > these. As
> > > > the
> > > > socket peer label is determined by the first associations
> > > > transport
> > > > address, it is recommended that all peer labels are consistent.
> > > 
> > > I'm still not sure this makes complete sense to me, but since I'm
> > > still not 100% confident in my understanding of SCTP I'm willing
> > > to
> > > punt on this for the moment.
> > 
> > I would like you to be happy with this so I've added what I think
> > is
> > some clarification.
> > 
> > The code that handles this is the selinux_sctp_assoc_request() in
> > hooks.c. If not the first association, then the other association
> > peer
> > SIDs are validated to enforce consistency among the peer SIDs.
> > However
> > what I recommend is that all peer labels should be the same. For
> > example:
> > 
> > Don't configure this (although valid):
> > netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
> >     label:system_u:object_r:netlabel_peer_lo_t:s0
> > netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
> >     label:system_u:object_r:netlabel_peer_wlan_t:s0
> > netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
> >     label:system_u:object_r:netlabel_peer_eth_t:s0
> > 
> > As this is recommended:
> > netlabelctl unlbl add interface:lo address:127.0.0.0/8 \
> >     label:system_u:object_r:netlabel_peer_t:s0
> > netlabelctl unlbl add interface:wlp12s0 address:192.168.1.0/24 \
> >     label:system_u:object_r:netlabel_peer_t:s0
> > netlabelctl unlbl add interface:enp9s0 address:193.168.1.0/24 \
> >     label:system_u:object_r:netlabel_peer_t:s0
> > 
> > Would you prefer me to delete this section ?
> 
> Okay, I think I'm getting a better idea of this ... let me walk
> through some of this, slowly.
> 
> On the send/egress side of things we don't really need to worry about
> the remote peer label as we only do access control checks using the
> sender's label (the local process/socket).  There is a slight
> exception with respect to forwarded traffic, but I think we handle
> that correctly for SCTP by substituting the traffic's label for the
> local process/socket label (current behavior).
> 
> On the receive/ingress side we could receive traffic from any of the
> associations, with the peer:recv permission checking the remote peer
> label (aka the sending association's label) against the local
> receiving socket.  This seems right to me.  As far as userspace is
> concerned, *_getpeersec_dgram() should work correctly,

I did get this working at one point as that's how I discovered IPv6 was
not working, hence https://github.com/SELinuxProject/selinux-kernel/iss
ues/24

>  but
> *_getpeersec_stream() will only return the peer label we stored
> during
> the initial association setup, yes?

Correct - Using this seemed reasonable as SCTP is 'connection
orientated'

>   This seems ... not the best.
> Although I'm not sure how to solve this.

I had thought of taking the initial associations peer label TE
component (as now) and then setting the MLS component to that of the
process as this reflects what the service can handle when using
MLS/CIPSO/CALIPSO. Using the SCTP selinux-testsuite for example:
Server process with CIPSO enabled using SOCK_SEQPACKET:
unconfined_u:unconfined_r:test_sctp_server_t:s0:c714,c769,c782,c788,c80
3,c842,c864

First client process MLS component: s0:c769,c788,c803 requests an
association that sets the servers peer context as:
system_u:object_r:netlabel_peer_t:s0:c769,c788,c803

However as packets are accepted by the server as per peer:recv
permission checking, then client 2 with an MLS component of
s0:c782,c714,c842,c864 would also be able to request an
association/send data. Therefore in these situations having the MLS
component as part of the peer contexts seems best. RFC 5570 (CALIPSO -
7.3.3. SCTP-Related Issues) has words of wisdom that I think I follow
regarding this type of scenario !!!

> 
> Is this why the requirement exists for each association to have an
> equivalent label?
> 

Yes I thought it was best to make this statement as it makes life
easier. I could also force this by accepting the first association (and
maybe set the MLS component as above), then do something like this in
selinux_sctp_assoc_request():

	} else if  (sksec->peer_sid != peer_sid) {
		/* All associations MUST have the same  peer label. */
		char *ctx1 = NULL, *ctx2 = NULL;
		u32 len;

		security_sid_to_context(peer_sid, &ctx1, &len);
		security_sid_to_context(sksec->peer_sid, &ctx2, &len);

		pr_err("%s: Invalid association peer context=%s
current=%s\n",
		       __func__, ctx1, ctx2);
		kfree(ctx1);
		kfree(ctx2);
		err = -EINVAL;
	}

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/security/SELinux-sctp.txt b/Documentation/security/SELinux-sctp.txt
new file mode 100644
index 0000000..32e0255
--- /dev/null
+++ b/Documentation/security/SELinux-sctp.txt
@@ -0,0 +1,108 @@ 
+                               SCTP SELinux Support
+                              ======================
+
+Security Hooks
+===============
+
+The Documentation/security/LSM-sctp.txt document describes how the following
+sctp security hooks are utilised:
+    security_sctp_assoc_request()
+    security_sctp_bind_connect()
+    security_sctp_sk_clone()
+
+    security_inet_conn_established()
+
+
+Policy Statements
+==================
+The following class and permissions to support SCTP are available within the
+kernel:
+    class sctp_socket inherits socket { node_bind }
+
+whenever the following policy capability is enabled:
+    policycap extended_socket_class;
+
+The SELinux SCTP support adds the additional permissions that are explained
+in the sections below:
+    association bindx connectx
+
+If userspace tools have been updated, SCTP will support the portcon
+statement as shown in the following example:
+    portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
+
+
+SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
+================================================================
+The hook security_sctp_bind_connect() is called by SCTP to check permissions
+required for ipv4/ipv6 addresses based on the @optname as follows:
+
+  ------------------------------------------------------------------
+  |                      BINDX Permission Check                    |
+  |       @optname             |         @address contains         |
+  |----------------------------|-----------------------------------|
+  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses |
+  ------------------------------------------------------------------
+
+  ------------------------------------------------------------------
+  |                  BIND Permission Checks                        |
+  |       @optname             |         @address contains         |
+  |----------------------------|-----------------------------------|
+  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6 address       |
+  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address       |
+  ------------------------------------------------------------------
+
+  ------------------------------------------------------------------
+  |                 CONNECTX Permission Check                      |
+  |       @optname             |         @address contains         |
+  |----------------------------|-----------------------------------|
+  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses |
+  ------------------------------------------------------------------
+
+  ------------------------------------------------------------------
+  |                 CONNECT Permission Checks                      |
+  |       @optname             |         @address contains         |
+  |----------------------------|-----------------------------------|
+  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6 address       |
+  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses |
+  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6 address       |
+  ------------------------------------------------------------------
+
+SCTP Peer Labeling
+===================
+An SCTP socket will only have one peer label assigned to it. This will be
+assigned during the establishment of the first association. Once the peer
+label has been assigned, any new associations will have the "association"
+permission validated by checking the socket peer sid against the received
+packets peer sid to determine whether the association should be allowed or
+denied.
+
+NOTES:
+   1) If peer labeling is not enabled, then the peer context will always be
+      SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
+
+   2) As SCTP supports multiple endpoints with multi-homing on a single socket
+      it is recommended that peer labels are consistent.
+
+   3) getpeercon(3) may be used by userspace to retrieve the sockets peer
+       context.
+
+   4) If using NetLabel be aware that if a label is assigned to a specific
+      interface, and that interface 'goes down', then the NetLabel service
+      will remove the entry. Therefore ensure that the network startup scripts
+      call netlabelctl(8) to set the required label (see netlabel-config(8)
+      helper script for details).
+
+   5) The NetLabel SCTP peer labeling rules apply as discussed in the following
+      set of posts tagged "netlabel" at: http://www.paul-moore.com/blog/t.
+
+   6) CIPSO is only supported for IPv4 addressing: socket(AF_INET, ...)
+      CALIPSO is only supported for IPv6 addressing: socket(AF_INET6, ...)
+
+      Note the following when testing CIPSO/CALIPSO:
+         a) CIPSO will send an ICMP packet if an SCTP packet cannot be
+            delivered because of an invalid label.
+         b) CALIPSO does not send an ICMP packet, just silently discards it.
+
+   7) IPSEC is not supported as rfc3554 - sctp/ipsec support has not been
+      implemented in userspace (racoon(8) or ipsec_pluto(8)), although the
+      kernel supports SCTP/IPSEC.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 33fd061..c3e9600 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -67,6 +67,8 @@ 
 #include <linux/tcp.h>
 #include <linux/udp.h>
 #include <linux/dccp.h>
+#include <linux/sctp.h>
+#include <net/sctp/structs.h>
 #include <linux/quota.h>
 #include <linux/un.h>		/* for Unix socket types */
 #include <net/af_unix.h>	/* for Unix socket types */
@@ -4119,6 +4121,23 @@  static int selinux_parse_skb_ipv4(struct sk_buff *skb,
 		break;
 	}
 
+#if IS_ENABLED(CONFIG_IP_SCTP)
+	case IPPROTO_SCTP: {
+		struct sctphdr _sctph, *sh;
+
+		if (ntohs(ih->frag_off) & IP_OFFSET)
+			break;
+
+		offset += ihlen;
+		sh = skb_header_pointer(skb, offset, sizeof(_sctph), &_sctph);
+		if (sh == NULL)
+			break;
+
+		ad->u.net->sport = sh->source;
+		ad->u.net->dport = sh->dest;
+		break;
+	}
+#endif
 	default:
 		break;
 	}
@@ -4192,6 +4211,19 @@  static int selinux_parse_skb_ipv6(struct sk_buff *skb,
 		break;
 	}
 
+#if IS_ENABLED(CONFIG_IP_SCTP)
+	case IPPROTO_SCTP: {
+		struct sctphdr _sctph, *sh;
+
+		sh = skb_header_pointer(skb, offset, sizeof(_sctph), &_sctph);
+		if (sh == NULL)
+			break;
+
+		ad->u.net->sport = sh->source;
+		ad->u.net->dport = sh->dest;
+		break;
+	}
+#endif
 	/* includes fragments */
 	default:
 		break;
@@ -4381,6 +4413,10 @@  static int selinux_socket_post_create(struct socket *sock, int family,
 		sksec = sock->sk->sk_security;
 		sksec->sclass = sclass;
 		sksec->sid = sid;
+		/* Allows detection of the first association on this socket */
+		if (sksec->sclass == SECCLASS_SCTP_SOCKET)
+			sksec->sctp_assoc_state = SCTP_ASSOC_UNSET;
+
 		err = selinux_netlbl_socket_post_create(sock->sk, family);
 	}
 
@@ -4401,11 +4437,7 @@  static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 	if (err)
 		goto out;
 
-	/*
-	 * If PF_INET or PF_INET6, check name_bind permission for the port.
-	 * Multiple address binding for SCTP is not supported yet: we just
-	 * check the first address now.
-	 */
+	/* If PF_INET or PF_INET6, check name_bind permission for the port. */
 	family = sk->sk_family;
 	if (family == PF_INET || family == PF_INET6) {
 		char *addrp;
@@ -4417,7 +4449,13 @@  static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 		unsigned short snum;
 		u32 sid, node_perm;
 
-		if (family == PF_INET) {
+		/*
+		 * sctp_bindx(3) calls via selinux_sctp_bind_connect()
+		 * that validates multiple binding addresses. Because of this
+		 * need to check address->sa_family as it is possible to have
+		 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
+		 */
+		if (family == PF_INET || address->sa_family == AF_INET) {
 			if (addrlen < sizeof(struct sockaddr_in)) {
 				err = -EINVAL;
 				goto out;
@@ -4471,6 +4509,10 @@  static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 			node_perm = DCCP_SOCKET__NODE_BIND;
 			break;
 
+		case SECCLASS_SCTP_SOCKET:
+			node_perm = SCTP_SOCKET__NODE_BIND;
+			break;
+
 		default:
 			node_perm = RAWIP_SOCKET__NODE_BIND;
 			break;
@@ -4485,7 +4527,7 @@  static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 		ad.u.net->sport = htons(snum);
 		ad.u.net->family = family;
 
-		if (family == PF_INET)
+		if (family == PF_INET || address->sa_family == AF_INET)
 			ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
 		else
 			ad.u.net->v6info.saddr = addr6->sin6_addr;
@@ -4510,10 +4552,12 @@  static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
 		return err;
 
 	/*
-	 * If a TCP or DCCP socket, check name_connect permission for the port.
+	 * If a TCP, DCCP or SCTP socket, check name_connect permission
+	 * for the port.
 	 */
 	if (sksec->sclass == SECCLASS_TCP_SOCKET ||
-	    sksec->sclass == SECCLASS_DCCP_SOCKET) {
+	    sksec->sclass == SECCLASS_DCCP_SOCKET ||
+	    sksec->sclass == SECCLASS_SCTP_SOCKET) {
 		struct common_audit_data ad;
 		struct lsm_network_audit net = {0,};
 		struct sockaddr_in *addr4 = NULL;
@@ -4521,7 +4565,14 @@  static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
 		unsigned short snum;
 		u32 sid, perm;
 
-		if (sk->sk_family == PF_INET) {
+		/* sctp_connectx(3) calls via
+		 *selinux_sctp_bind_connect() that validates multiple
+		 * connect addresses. Because of this need to check
+		 * address->sa_family as it is possible to have
+		 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
+		 */
+		if (sk->sk_family == PF_INET ||
+					address->sa_family == AF_INET) {
 			addr4 = (struct sockaddr_in *)address;
 			if (addrlen < sizeof(struct sockaddr_in))
 				return -EINVAL;
@@ -4534,11 +4585,21 @@  static int selinux_socket_connect(struct socket *sock, struct sockaddr *address,
 		}
 
 		err = sel_netport_sid(sk->sk_protocol, snum, &sid);
+
 		if (err)
 			goto out;
 
-		perm = (sksec->sclass == SECCLASS_TCP_SOCKET) ?
-		       TCP_SOCKET__NAME_CONNECT : DCCP_SOCKET__NAME_CONNECT;
+		switch (sksec->sclass) {
+		case SECCLASS_TCP_SOCKET:
+			perm = TCP_SOCKET__NAME_CONNECT;
+			break;
+		case SECCLASS_DCCP_SOCKET:
+			perm = DCCP_SOCKET__NAME_CONNECT;
+			break;
+		case SECCLASS_SCTP_SOCKET:
+			perm = SCTP_SOCKET__NAME_CONNECT;
+			break;
+		}
 
 		ad.type = LSM_AUDIT_DATA_NET;
 		ad.u.net = &net;
@@ -4815,7 +4876,8 @@  static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
 	u32 peer_sid = SECSID_NULL;
 
 	if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
-	    sksec->sclass == SECCLASS_TCP_SOCKET)
+	    sksec->sclass == SECCLASS_TCP_SOCKET ||
+	    sksec->sclass == SECCLASS_SCTP_SOCKET)
 		peer_sid = sksec->peer_sid;
 	if (peer_sid == SECSID_NULL)
 		return -ENOPROTOOPT;
@@ -4928,6 +4990,183 @@  static void selinux_sock_graft(struct sock *sk, struct socket *parent)
 	sksec->sclass = isec->sclass;
 }
 
+/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
+static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
+				      struct sk_buff *skb,
+				      int sctp_cid)
+{
+	struct sk_security_struct *sksec = ep->base.sk->sk_security;
+	struct common_audit_data ad;
+	struct lsm_network_audit net = {0,};
+	u8 peerlbl_active;
+	u32 peer_sid = SECINITSID_UNLABELED;
+	u32 conn_sid;
+	int err;
+
+	if (!selinux_policycap_extsockclass)
+		return 0;
+
+	peerlbl_active = selinux_peerlbl_enabled();
+
+	if (peerlbl_active) {
+		/* This will return peer_sid = SECSID_NULL if there are
+		 * no peer labels, see security_net_peersid_resolve().
+		 */
+		err = selinux_skb_peerlbl_sid(skb, ep->base.sk->sk_family,
+					      &peer_sid);
+
+		if (err)
+			return err;
+
+		if (peer_sid == SECSID_NULL)
+			peer_sid = SECINITSID_UNLABELED;
+	}
+
+	if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
+		sksec->sctp_assoc_state = SCTP_ASSOC_SET;
+
+		/* Here as first association on socket. As the peer SID
+		 * was allowed by peer recv (and the netif/node checks),
+		 * then it is approved by policy and used as the primary
+		 * peer SID for getpeercon(3).
+		 */
+		sksec->peer_sid = peer_sid;
+	} else if  (sksec->peer_sid != peer_sid) {
+		/* Other association peer SIDs are checked to enforce
+		 * consistency among the peer SIDs.
+		 */
+		ad.type = LSM_AUDIT_DATA_NET;
+		ad.u.net = &net;
+		ad.u.net->sk = ep->base.sk;
+		err = avc_has_perm(sksec->peer_sid, peer_sid, sksec->sclass,
+				   SCTP_SOCKET__ASSOCIATION, &ad);
+		if (err)
+			return err;
+	}
+
+	if (sctp_cid == SCTP_CID_INIT) {
+		/* Have INIT when incoming connect(2), sctp_connectx(3)
+		 * or sctp_sendmsg(3) (with no association already present),
+		 * so compute the MLS component for the connection and store
+		 * the information in ep. This will be used by SCTP TCP type
+		 * sockets and peeled off connections as they cause a new
+		 * socket to be generated. selinux_sctp_sk_clone() will then
+		 * plug this into the new socket.
+		 */
+		err = selinux_conn_sid(sksec->sid, peer_sid, &conn_sid);
+		if (err)
+			return err;
+
+		ep->secid = conn_sid;
+		ep->peer_secid = peer_sid;
+
+		/* Set any NetLabel labels including CIPSO/CALIPSO options. */
+		return selinux_netlbl_sctp_assoc_request(ep, skb);
+	}
+
+	return 0;
+}
+
+/*
+ * Check if sctp IPv4/IPv6 addresses are valid for binding or connecting
+ * based on their @optname.
+ */
+static int selinux_sctp_bind_connect(struct sock *sk, int optname,
+				     struct sockaddr *address,
+				     int addrlen)
+{
+	int len, err = 0, walk_size = 0;
+	void *addr_buf;
+	struct sockaddr *addr;
+	struct socket *sock;
+
+	if (!selinux_policycap_extsockclass)
+		return 0;
+
+	switch (optname) {
+	case SCTP_SOCKOPT_BINDX_ADD:
+		err = sock_has_perm(sk, SCTP_SOCKET__BINDX);
+		break;
+	case SCTP_SOCKOPT_CONNECTX:
+		err = sock_has_perm(sk, SCTP_SOCKET__CONNECTX);
+		break;
+	/* These need SOCKET__BIND or SOCKET__CONNECT permissions that will
+	 * be checked later.
+	 */
+	case SCTP_PRIMARY_ADDR:
+	case SCTP_SET_PEER_PRIMARY_ADDR:
+	case SCTP_PARAM_SET_PRIMARY:
+	case SCTP_PARAM_ADD_IP:
+	case SCTP_SENDMSG_CONNECT:
+		break;
+	default:
+		err = -EINVAL;
+	}
+	if (err)
+		return err;
+
+	/* Process one or more addresses that may be IPv4 or IPv6 */
+	sock = sk->sk_socket;
+	addr_buf = address;
+
+	while (walk_size < addrlen) {
+		addr = addr_buf;
+		switch (addr->sa_family) {
+		case AF_INET:
+			len = sizeof(struct sockaddr_in);
+			break;
+		case AF_INET6:
+			len = sizeof(struct sockaddr_in6);
+			break;
+		default:
+			return -EAFNOSUPPORT;
+		}
+
+		err = -EINVAL;
+		switch (optname) {
+		/* Bind checks */
+		case SCTP_PRIMARY_ADDR:
+		case SCTP_SET_PEER_PRIMARY_ADDR:
+		case SCTP_SOCKOPT_BINDX_ADD:
+			err = selinux_socket_bind(sock, addr, len);
+			break;
+		/* Connect checks */
+		case SCTP_SOCKOPT_CONNECTX:
+		case SCTP_PARAM_SET_PRIMARY:
+		case SCTP_PARAM_ADD_IP:
+		case SCTP_SENDMSG_CONNECT:
+			err = selinux_socket_connect(sock, addr, len);
+			break;
+		}
+
+		if (err)
+			return err;
+
+		addr_buf += len;
+		walk_size += len;
+	}
+	return 0;
+}
+
+/* Called whenever a new socket is created by accept(2) or sctp_peeloff(3). */
+static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
+				  struct sock *newsk)
+{
+	struct sk_security_struct *sksec = sk->sk_security;
+	struct sk_security_struct *newsksec = newsk->sk_security;
+
+	/* If policy does not support SECCLASS_SCTP_SOCKET then call
+	 * the non-sctp clone version.
+	 */
+	if (!selinux_policycap_extsockclass)
+		return selinux_sk_clone_security(sk, newsk);
+
+	newsksec->sid = ep->secid;
+	newsksec->peer_sid = ep->peer_secid;
+	newsksec->sclass = sksec->sclass;
+	newsksec->nlbl_state = sksec->nlbl_state;
+}
+
 static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
 				     struct request_sock *req)
 {
@@ -6416,6 +6655,9 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
 	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
 	LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
+	LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request),
+	LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
+	LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect),
 	LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request),
 	LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
 	LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index b9fe343..b4b10da 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -173,7 +173,8 @@  struct security_class_mapping secclass_map[] = {
 	  { COMMON_CAP2_PERMS, NULL } },
 	{ "sctp_socket",
 	  { COMMON_SOCK_PERMS,
-	    "node_bind", NULL } },
+	    "node_bind", "name_connect", "association", "bindx",
+	    "connectx", NULL } },
 	{ "icmp_socket",
 	  { COMMON_SOCK_PERMS,
 	    "node_bind", NULL } },
diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
index 75686d5..835a0d6 100644
--- a/security/selinux/include/netlabel.h
+++ b/security/selinux/include/netlabel.h
@@ -33,6 +33,7 @@ 
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <net/request_sock.h>
+#include <net/sctp/structs.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -53,7 +54,8 @@  int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
 int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 				 u16 family,
 				 u32 sid);
-
+int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+				     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);
@@ -114,6 +116,11 @@  static inline int selinux_netlbl_conn_setsid(struct sock *sk,
 	return 0;
 }
 
+static inline int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+						    struct sk_buff *skb)
+{
+	return 0;
+}
 static inline int selinux_netlbl_inet_conn_request(struct request_sock *req,
 						   u16 family)
 {
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 6ebc61e..660f270 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -130,6 +130,11 @@  struct sk_security_struct {
 	u32 sid;			/* SID of this object */
 	u32 peer_sid;			/* SID of peer */
 	u16 sclass;			/* sock security class */
+
+	enum {				/* SCTP association state */
+		SCTP_ASSOC_UNSET = 0,
+		SCTP_ASSOC_SET,
+	} sctp_assoc_state;
 };
 
 struct tun_security_struct {
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index aaba667..7d5aa15 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -250,6 +250,7 @@  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 	sk = skb_to_full_sk(skb);
 	if (sk != NULL) {
 		struct sk_security_struct *sksec = sk->sk_security;
+
 		if (sksec->nlbl_state != NLBL_REQSKB)
 			return 0;
 		secattr = selinux_netlbl_sock_getattr(sk, sid);
@@ -271,6 +272,41 @@  int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 }
 
 /**
+ * selinux_netlbl_sctp_assoc_request - Label an incoming sctp association.
+ * @ep: incoming association endpoint.
+ * @skb: the packet.
+ *
+ * Description:
+ * A new incoming connection is represented by @ep, ......
+ * Returns zero on success, negative values on failure.
+ *
+ */
+int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+				     struct sk_buff *skb)
+{
+	int rc;
+	struct netlbl_lsm_secattr secattr;
+	struct sk_security_struct *sksec = ep->base.sk->sk_security;
+
+	if (ep->base.sk->sk_family != PF_INET &&
+				ep->base.sk->sk_family != PF_INET6)
+		return 0;
+
+	netlbl_secattr_init(&secattr);
+	rc = security_netlbl_sid_to_secattr(ep->secid, &secattr);
+	if (rc != 0)
+		goto assoc_request_return;
+
+	rc = netlbl_sctp_setattr(ep->base.sk, skb, &secattr);
+	if (rc == 0)
+		sksec->nlbl_state = NLBL_LABELED;
+
+assoc_request_return:
+	netlbl_secattr_destroy(&secattr);
+	return rc;
+}
+
+/**
  * selinux_netlbl_inet_conn_request - Label an incoming stream connection
  * @req: incoming connection request socket
  *
@@ -481,7 +517,7 @@  int selinux_netlbl_socket_setsockopt(struct socket *sock,
  */
 int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
 {
-	int rc;
+	int rc, already_owned_by_user = 0;
 	struct sk_security_struct *sksec = sk->sk_security;
 	struct netlbl_lsm_secattr *secattr;
 
@@ -489,7 +525,16 @@  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
 	    sksec->nlbl_state != NLBL_CONNLABELED)
 		return 0;
 
-	lock_sock(sk);
+	/* Note: When called via connect(2) this happens before the socket
+	 * protocol layer connect operation and @sk is not locked, HOWEVER,
+	 * when called by the SCTP protocol layer via sctp_connectx(3),
+	 * sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore check if
+	 * @sk owned already.
+	 */
+	if (sock_owned_by_user(sk) && sksec->sclass == SECCLASS_SCTP_SOCKET)
+		already_owned_by_user = 1;
+	else
+		lock_sock(sk);
 
 	/* connected sockets are allowed to disconnect when the address family
 	 * is set to AF_UNSPEC, if that is what is happening we want to reset
@@ -510,6 +555,7 @@  int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
 		sksec->nlbl_state = NLBL_CONNLABELED;
 
 socket_connect_return:
-	release_sock(sk);
+	if (!already_owned_by_user)
+		release_sock(sk);
 	return rc;
 }