Re: [RFC PATCH 1/1] kernel: Add SELinux SCTP protocol support

[Richard Haines <richard_c_haines@btinternet.com>]

On Wed, 2016-12-21 at 14:09 -0200, Marcelo Ricardo Leitner wrote:
> On Wed, Dec 14, 2016 at 01:39:59PM +0000, Richard Haines wrote:
> > +SCTP Socket Option Permissions
> > +===============> > +The permissions consist of: "bindx_add" "bindx_rem" "connectx"
> > "set_addr" and
> > +"set_params" that are validated on setsockopt(2) calls, and
> > "peeloff" that is
> > +validated on getsockopt(2) calls.
> > +
> > +SCTP_SOCKOPT_BINDX_ADD - Allows additional bind addresses to be
> > +                         associated after (optionally) calling
> > bind(2)
> > +                         if given the "bind_add" permission.
> > +
> > +SCTP_SOCKOPT_CONNECTX  - Allows the allocation of multiple
> > +                         addresses for reaching a multi-homed peer
> > +                         if given the "connectx" permission.
> > +
> > +  Together they are used to form SCTP associations with
> > information being
> > +  passed over the link to inform the peer of any changes. As these
> > two options
> > +  can support multiple addresses, each address is checked via
> > +  selinux_socket_bind() or selinux_socket_connect() to determine
> > whether they
> > +  have the correct permissions:
> > +    bindx_add: bind, name_bind, node_bind + node SID + port SID
> > via the
> > +               (portcon sctp port ctx) policy statement.
> > +    connectx:  connect, name_connect + port SID via the
> > +               (portcon sctp port ctx) policy statement.
> > +
> > +SCTP_SOCKOPT_BINDX_REM - Allows additional bind addresses to be
> > removed
> > +                         if given the "bind_rem" permission.
> > +
> > +SCTP_PEER_ADDR_PARAMS - Alter heartbeats and address max
> > retransmissions.
> > +SCTP_PEER_ADDR_THLDS  - Alter the thresholds.
> > +SCTP_ASSOCINFO        - Alter association and endpoint parameters.
> > + These require the "set_params" permission.
> > +
> > +SCTP_PRIMARY_ADDR          - Set local primary address.
> > +SCTP_SET_PEER_PRIMARY_ADDR - Request peer sets address as
> > association primary.
> > + These require the "set_addr" permission.
> > +
> > +SCTP_SOCKOPT_PEELOFF - Branch off an association into a new socket
> > that
> > +will be a one-to-one style socket. As SELinux already handles the
> > creation
> > +of new sockets, only the "peeloff" permission is checked.
> 
> ...
> 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 7b0e059..ff4f1a8 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1009,6 +1009,12 @@ static int sctp_setsockopt_bindx(struct sock
> > *sk,
> >  	/* Do the work. */
> >  	switch (op) {
> >  	case SCTP_BINDX_ADD_ADDR:
> > +		/* Allow security module to validate bindx
> > addresses. */
> > +		err = security_sk_setsockopt(sk, SOL_SCTP,
> > +					     SCTP_SOCKOPT_BINDX_AD
> > D,
> > +					     (char *)kaddrs,
> > addrs_size);
> 
> Here, kaddrs is about the addresses that we are going to bind to.
> 
> > +		if (err)
> > +			goto out;
> >  		err = sctp_bindx_add(sk, kaddrs, addrcnt);
> >  		if (err)
> >  			goto out;
> > @@ -1329,9 +1335,17 @@ static int __sctp_setsockopt_connectx(struct
> > sock *sk,
> >  	if (__copy_from_user(kaddrs, addrs, addrs_size)) {
> >  		err = -EFAULT;
> >  	} else {
> > +		/* Allow security module to validate connectx
> > addresses. */
> > +		err = security_sk_setsockopt(sk, SOL_SCTP,
> > +					     SCTP_SOCKOPT_CONNECTX
> > ,
> > +					    (char *)kaddrs,
> > addrs_size);
> 

Sorry for the delay but I now think I've resolved all but one of the
SCTP issues with tests to check them. The only area I'm having trouble
with is labeling TCP-style child sockets but hope to resolve.

> Here, kaddrs is about the remote addresses that we are connecting to.
> Not sure how feasible this is for SELinux, to maintain a list of
> allowed
> peers. 
SELinux does not maintain lists, however it can check whether the
addresses/ports are allowed or not (which is what I do for binds,
connects etc.).

> But this being right, I think we are missing the hooks at ASCONF
> handling side.
> One SCTP peer can start/stop binding to another IP in runtime using
> ASCONF chunks. So considering that peer A here validated that it can
> associate to be peer B, if B is using ASCONF to inform A that it's
> now
> also binding on address X, A should validate so before ACKing it.
> 
> This validation would be around sctp_process_asconf_param. Not sure
> you
> can hook it on selinux_sctp_setsockopt too as it would be similar to
> the
> validation done for CONNECT.

I now have this working after hooking into sctp_process_asconf_param
and checking permissions on address/port as required. Also have tests
as part of the selinux-testsuite.
> 
> Richard, the other point we talked offline, was for validating that
> peer
> A can actually request to add address X, that would be ok, yes.
> 

This was regarding handling ASCONF requests on receiver the side. Yes
this does seem to be resolved.

I hope to send out an updated patch in a few weeks so hopefully these
can be verified.

> Thanks,
>   Marcelo
> 
> > +		if (err)
> > +			goto out_free;
> > +
> >  		err = __sctp_connect(sk, kaddrs, addrs_size,
> > assoc_id);
> >  	}
> >  
> > +out_free:
> >  	kfree(kaddrs);
> >  
> >  	return err;
> 
> 
> 
> > +int selinux_sctp_setsockopt(struct sock *sk, int level, int
> > optname,
> > +					    char *optval, int
> > optlen)
> > +{
> > +	int err, addrlen;
> > +	void *addr_buf;
> > +	struct sockaddr *address;
> > +	struct socket *sock;
> > +	int walk_size = 0;
> > +
> > +	if (level != SOL_SCTP || level != IPPROTO_SCTP)
> > +		return 0;
> > +
> > +	switch (optname) {
> > +	case SCTP_SOCKOPT_BINDX_ADD:
> > +	case SCTP_SOCKOPT_CONNECTX:
> > +		/* Note that for SCTP_SOCKOPT_BINDX_ADD and
> > +		 * SCTP_SOCKOPT_CONNECTX the sctp kernel code has
> > already
> > +		 * copied the optval to kernel space. See
> > net/sctp/socket.c
> > +		 * security_sk_setsockopt() calls.
> > +		 */
> > +		err = sock_has_perm(current, sk,
> > +			    (optname = SCTP_SOCKOPT_BINDX_ADD ?
> > +			     SCTP_SOCKET__BINDX_ADD :
> > +			     SCTP_SOCKET__CONNECTX));
> > +		if (err)
> > +			return err;
> > +
> > +		sock = sk->sk_socket;
> > +		addr_buf = optval;
> > +		/* Process list - may contain IPv4 or IPv6 addr's
> > */
> > +		while (walk_size < optlen) {
> > +			address = addr_buf;
> > +
> > +			switch (address->sa_family) {
> > +			case PF_INET:
> > +				addrlen = sizeof(struct
> > sockaddr_in);
> > +				break;
> > +			case PF_INET6:
> > +				addrlen = sizeof(struct
> > sockaddr_in6);
> > +				break;
> > +			default:
> > +				return -EINVAL;
> > +			}
> > +
> > +			err = -EINVAL;
> > +			if (optname = SCTP_SOCKOPT_BINDX_ADD) {
> > +				err = selinux_socket_bind(sock,
> > +					    address, addrlen);
> > +			} else if (optname =
> > SCTP_SOCKOPT_CONNECTX) {
> > +				err = selinux_socket_connect(sock,
> > +					    address, addrlen);
> > +			}
> > +			if (err)
> > +				return err;
> > +
> > +			addr_buf += addrlen;
> > +			walk_size += addrlen;
> > +		}
> > +		break;
> > +
> > +	case SCTP_SOCKOPT_BINDX_REM:
> > +		/* The addresses have been checked as they were
> > +		 * added, so just see if allowed to be removed.
> > +		 */
> > +		err = sock_has_perm(current, sk,
> > SCTP_SOCKET__BINDX_REM);
> > +		if (err)
> > +			return err;
> > +		break;
> > +
> > +	/* Set heartbeats and address max retransmissions. */
> > +	case SCTP_PEER_ADDR_PARAMS:
> > +	/* Set thresholds. */
> > +	case SCTP_PEER_ADDR_THLDS:
> > +	/* Set association and endpoint parameters */
> > +	case SCTP_ASSOCINFO:
> > +		err = sock_has_perm(current, sk,
> > SCTP_SOCKET__SET_PARAMS);
> > +		if (err)
> > +			return err;
> > +		break;
> > +
> > +	/* Set local primary address. */
> > +	case SCTP_PRIMARY_ADDR:
> > +	/* Request peer sets address as association primary. */
> > +	case SCTP_SET_PEER_PRIMARY_ADDR:
> > +		err = sock_has_perm(current, sk,
> > SCTP_SOCKET__SET_ADDR);
> > +		if (err)
> > +			return err;
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
>