Message-ID: <50a04eaf-6130-74d0-43e4-b09bd4de2180@schaufler-ca.com>
Date:   Fri, 22 Dec 2017 09:20:45 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        selinux@...ho.nsa.gov, netdev@...r.kernel.org,
        linux-sctp@...r.kernel.org, linux-security-module@...r.kernel.org
Cc:     paul@...l-moore.com, vyasevich@...il.com, nhorman@...driver.com,
        sds@...ho.nsa.gov, eparis@...isplace.org,
        richard_c_haines@...nternet.com
Subject: Re: [PATCH v3 1/4] security: Add support for SCTP security hooks

On 12/22/2017 5:05 AM, Marcelo Ricardo Leitner wrote:
> From: Richard Haines <richard_c_haines@...nternet.com>
>
> The SCTP security hooks are explained in:
> Documentation/security/LSM-sctp.rst
>
> Signed-off-by: Richard Haines <richard_c_haines@...nternet.com>
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> ---
>  Documentation/security/LSM-sctp.rst | 194 ++++++++++++++++++++++++++++++++++++
>  include/linux/lsm_hooks.h           |  35 +++++++
>  include/linux/security.h            |  25 +++++
>  security/security.c                 |  22 ++++
>  4 files changed, 276 insertions(+)
>  create mode 100644 Documentation/security/LSM-sctp.rst
>
> diff --git a/Documentation/security/LSM-sctp.rst b/Documentation/security/LSM-sctp.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..61373672ce9f63bbd52d953500f44cdf3427c3f0
> --- /dev/null
> +++ b/Documentation/security/LSM-sctp.rst
> @@ -0,0 +1,194 @@
> +SCTP LSM Support
> +================
> +
> +For security module support, three sctp specific hooks have been implemented::
> +
> +    security_sctp_assoc_request()
> +    security_sctp_bind_connect()
> +    security_sctp_sk_clone()
> +
> +Also the following security hook has been utilised::
> +
> +    security_inet_conn_established()
> +
> +The usage of these hooks are described below with the SELinux implementation
> +described in ``Documentation/security/SELinux-sctp.rst``
> +
> +
> +security_sctp_assoc_request()
> +-----------------------------
> +This new hook passes the ``@...` and ``@...nk->skb`` (the association INIT
> +packet) to the security module. Returns 0 on success, error on failure.
> +::
> +
> +    @ep - pointer to sctp endpoint structure.
> +    @skb - pointer to skbuff of association packet.
> +
> +The security module performs the following operations:
> +     IF this is the first association on ``@ep->base.sk``, then set the peer
> +     sid to that in ``@...``. This will ensure there is only one peer sid
> +     assigned to ``@ep->base.sk`` that may support multiple associations.
> +
> +     ELSE validate the ``@ep->base.sk peer_sid`` against the ``@skb peer sid``
> +     to determine whether the association should be allowed or denied.
> +
> +     Set the sctp ``@ep sid`` to socket's sid (from ``ep->base.sk``) with
> +     MLS portion taken from ``@skb peer sid``. This will be used by SCTP
> +     TCP style sockets and peeled off connections as they cause a new socket
> +     to be generated.
> +
> +     If IP security options are configured (CIPSO/CALIPSO), then the ip
> +     options are set on the socket.

Please! Basing the documentation for the infrastructure behavior
on a specific security module implementation makes it *really rough*
to adopt it to a different module. It makes it doubly difficult to
define how it will work with multiple modules. Take the SELinux specifics
out of the documentation for the hooks. Describe the general intention,
not how SELinux uses it.

> +
> +
> +security_sctp_bind_connect()
> +-----------------------------
> +This new hook passes one or more ipv4/ipv6 addresses to the security module
> +for validation based on the ``@...name`` that will result in either a bind or
> +connect service as shown in the permission check tables below.
> +Returns 0 on success, error on failure.
> +::
> +
> +    @sk      - Pointer to sock structure.
> +    @optname - Name of the option to validate.
> +    @address - One or more ipv4 / ipv6 addresses.
> +    @addrlen - The total length of address(s). This is calculated on each
> +               ipv4 or ipv6 address using sizeof(struct sockaddr_in) or
> +               sizeof(struct sockaddr_in6).
> +
> +  ------------------------------------------------------------------
> +  |                     BIND Type Checks                           |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_BINDX_ADD     | One or more ipv4 / ipv6 addresses |
> +  | SCTP_PRIMARY_ADDR          | Single ipv4 or ipv6 address       |
> +  | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +  ------------------------------------------------------------------
> +  |                   CONNECT Type Checks                          |
> +  |       @optname             |         @address contains         |
> +  |----------------------------|-----------------------------------|
> +  | SCTP_SOCKOPT_CONNECTX      | One or more ipv4 / ipv6 addresses |
> +  | SCTP_PARAM_ADD_IP          | One or more ipv4 / ipv6 addresses |
> +  | SCTP_SENDMSG_CONNECT       | Single ipv4 or ipv6 address       |
> +  | SCTP_PARAM_SET_PRIMARY     | Single ipv4 or ipv6 address       |
> +  ------------------------------------------------------------------
> +
> +A summary of the ``@...name`` entries is as follows::
> +
> +    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_SENDMSG_CONNECT  - Initiate a connection that is generated by a
> +                            sendmsg(2) or sctp_sendmsg(3) on a new asociation.
> +
> +    SCTP_PRIMARY_ADDR     - Set local primary address.
> +
> +    SCTP_SET_PEER_PRIMARY_ADDR - Request peer sets address as
> +                                 association primary.
> +
> +    SCTP_PARAM_ADD_IP          - These are used when Dynamic Address
> +    SCTP_PARAM_SET_PRIMARY     - Reconfiguration is enabled as explained below.
> +
> +
> +To support Dynamic Address Reconfiguration the following parameters must be
> +enabled on both endpoints (or use the appropriate **setsockopt**\(2))::
> +
> +    /proc/sys/net/sctp/addip_enable
> +    /proc/sys/net/sctp/addip_noauth_enable
> +
> +then the following *_PARAM_*'s are sent to the peer in an
> +ASCONF chunk when the corresponding ``@...name``'s are present::
> +
> +          @optname                      ASCONF Parameter
> +         ----------                    ------------------
> +    SCTP_SOCKOPT_BINDX_ADD     ->   SCTP_PARAM_ADD_IP
> +    SCTP_SET_PEER_PRIMARY_ADDR ->   SCTP_PARAM_SET_PRIMARY
> +
> +
> +security_sctp_sk_clone()
> +-------------------------
> +This new hook is called whenever a new socket is created by **accept**\(2)
> +(i.e. a TCP style socket) or when a socket is 'peeled off' e.g userspace
> +calls **sctp_peeloff**\(3). ``security_sctp_sk_clone()`` will set the new
> +sockets sid and peer sid to that contained in the ``@ep sid`` and
> +``@ep peer sid`` respectively.
> +::
> +
> +    @ep - pointer to old sctp endpoint structure.
> +    @sk - pointer to old sock structure.
> +    @sk - pointer to new sock structure.

Again, SELinux may set the sids in this way, but another
module may treat the hook differently. Don't put SELinux
specifics into the infrastructure documentation.

> +
> +
> +security_inet_conn_established()
> +---------------------------------
> +This hook has been added to the receive COOKIE ACK processing where it sets
> +the connection's peer sid to that in ``@...``::
> +
> +    @sk  - pointer to sock structure.
> +    @skb - pointer to skbuff of the COOKIE ACK packet.
> +
> +
> +Security Hooks used for Association Establishment
> +=================================================
> +The following diagram shows the use of ``security_sctp_connect_bind()``,
> +``security_sctp_assoc_request()``, ``security_inet_conn_established()`` when
> +establishing an association.
> +::
> +
> +      SCTP endpoint "A"                                SCTP endpoint "Z"
> +      =================                                =================
> +    sctp_sf_do_prm_asoc()
> + Association setup can be initiated
> + by a connect(2), sctp_connectx(3),
> + sendmsg(2) or sctp_sendmsg(3).
> + These will result in a call to
> + security_sctp_bind_connect() to
> + initiate an association to
> + SCTP peer endpoint "Z".
> +         INIT --------------------------------------------->
> +                                                   sctp_sf_do_5_1B_init()
> +                                                 Respond to an INIT chunk.
> +                                             SCTP peer endpoint "A" is
> +                                             asking for an association. Call
> +                                             security_sctp_assoc_request()
> +                                             to set the peer label if first
> +                                             association.
> +                                             If not first association, check
> +                                             whether allowed, IF so send:
> +          <----------------------------------------------- INIT ACK
> +          |                                  ELSE audit event and silently
> +          |                                       discard the packet.
> +          |
> +    COOKIE ECHO ------------------------------------------>
> +                                                          |
> +                                                          |
> +                                                          |
> +          <------------------------------------------- COOKIE ACK
> +          |                                               |
> +    sctp_sf_do_5_1E_ca                                    |
> + Call security_inet_conn_established()                    |
> + to set the correct peer sid.                             |
> +          |                                               |
> +          |                               If SCTP_SOCKET_TCP or peeled off
> +          |                               socket security_sctp_sk_clone() is
> +          |                               called to clone the new socket.
> +          |                                               |
> +      ESTABLISHED                                    ESTABLISHED
> +          |                                               |
> +    ------------------------------------------------------------------
> +    |                     Association Established                    |
> +    ------------------------------------------------------------------
> +
> +
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e41757187cdb8b2f83c5901966345902..92ee9c6c604212ce38590bd2e5fcba55617b9c04 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -906,6 +906,32 @@
>   *	associated with the TUN device's security structure.
>   *	@security pointer to the TUN devices's security structure.
>   *
> + * Security hooks for SCTP
> + *
> + * @sctp_assoc_request:
> + *	If first association, then set the peer sid to that in @skb. If
> + *	@sctp_cid is from an INIT chunk, then set the sctp endpoint sid to
> + *	socket's sid (ep->base.sk) with MLS portion taken from peer sid.

Don't direct the module hook to do something. If the SCTP
code knows what every module should do, why not have the
SCTP code do it? Do not describe the hook in terms of the
SELinux behavior.

> + *	@ep pointer to sctp endpoint structure.
> + *	@skb pointer to skbuff of association packet.
> + *	Return 0 on success, error on failure.
> + * @sctp_bind_connect:
> + *	Validiate permissions required for each address associated with sock
> + *	@sk. Depending on @optname, the addresses will be treated as either
> + *	for a connect or bind service. The @addrlen is calculated on each
> + *	ipv4 and ipv6 address using sizeof(struct sockaddr_in) or
> + *	sizeof(struct sockaddr_in6).
> + *	@sk pointer to sock structure.
> + *	@optname name of the option to validate.
> + *	@address list containing one or more ipv4/ipv6 addresses.
> + *	@addrlen total length of address(s).
> + *	Return 0 on success, error on failure.
> + * @sctp_sk_clone:
> + *	Sets the new child socket's sid to the old endpoint sid.

... and again.

> + *	@ep pointer to old sctp endpoint structure.
> + *	@sk pointer to old sock structure.
> + *	@sk pointer to new sock structure.
> + *
>   * Security hooks for Infiniband
>   *
>   * @ib_pkey_access:
> @@ -1631,6 +1657,12 @@ union security_list_options {
>  	int (*tun_dev_attach_queue)(void *security);
>  	int (*tun_dev_attach)(struct sock *sk, void *security);
>  	int (*tun_dev_open)(void *security);
> +	int (*sctp_assoc_request)(struct sctp_endpoint *ep,
> +				  struct sk_buff *skb);
> +	int (*sctp_bind_connect)(struct sock *sk, int optname,
> +				 struct sockaddr *address, int addrlen);
> +	void (*sctp_sk_clone)(struct sctp_endpoint *ep, struct sock *sk,
> +			      struct sock *newsk);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_INFINIBAND
> @@ -1869,6 +1901,9 @@ struct security_hook_heads {
>  	struct list_head tun_dev_attach_queue;
>  	struct list_head tun_dev_attach;
>  	struct list_head tun_dev_open;
> +	struct list_head sctp_assoc_request;
> +	struct list_head sctp_bind_connect;
> +	struct list_head sctp_sk_clone;
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  #ifdef CONFIG_SECURITY_INFINIBAND
>  	struct list_head ib_pkey_access;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c430a90e1ad3c3749d0a438ecaca9..51f6cc2417f278674dfbd434587af805cb0c03d3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -115,6 +115,7 @@ struct xfrm_policy;
>  struct xfrm_state;
>  struct xfrm_user_sec_ctx;
>  struct seq_file;
> +struct sctp_endpoint;
>  
>  #ifdef CONFIG_MMU
>  extern unsigned long mmap_min_addr;
> @@ -1229,6 +1230,11 @@ int security_tun_dev_create(void);
>  int security_tun_dev_attach_queue(void *security);
>  int security_tun_dev_attach(struct sock *sk, void *security);
>  int security_tun_dev_open(void *security);
> +int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb);
> +int security_sctp_bind_connect(struct sock *sk, int optname,
> +			       struct sockaddr *address, int addrlen);
> +void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> +			    struct sock *newsk);
>  
>  #else	/* CONFIG_SECURITY_NETWORK */
>  static inline int security_unix_stream_connect(struct sock *sock,
> @@ -1421,6 +1427,25 @@ static inline int security_tun_dev_open(void *security)
>  {
>  	return 0;
>  }
> +
> +static inline int security_sctp_assoc_request(struct sctp_endpoint *ep,
> +					      struct sk_buff *skb)
> +{
> +	return 0;
> +}
> +
> +static inline int security_sctp_bind_connect(struct sock *sk, int optname,
> +					     struct sockaddr *address,
> +					     int addrlen)
> +{
> +	return 0;
> +}
> +
> +static inline void security_sctp_sk_clone(struct sctp_endpoint *ep,
> +					  struct sock *sk,
> +					  struct sock *newsk)
> +{
> +}
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_INFINIBAND
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef94df1d3c44b7fed6b7b651c1924f..1400678f6b72b36123f2fa2b909f35d257a62cd4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1472,6 +1472,7 @@ void security_inet_conn_established(struct sock *sk,
>  {
>  	call_void_hook(inet_conn_established, sk, skb);
>  }
> +EXPORT_SYMBOL(security_inet_conn_established);
>  
>  int security_secmark_relabel_packet(u32 secid)
>  {
> @@ -1527,6 +1528,27 @@ int security_tun_dev_open(void *security)
>  }
>  EXPORT_SYMBOL(security_tun_dev_open);
>  
> +int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb)
> +{
> +	return call_int_hook(sctp_assoc_request, 0, ep, skb);
> +}
> +EXPORT_SYMBOL(security_sctp_assoc_request);
> +
> +int security_sctp_bind_connect(struct sock *sk, int optname,
> +			       struct sockaddr *address, int addrlen)
> +{
> +	return call_int_hook(sctp_bind_connect, 0, sk, optname,
> +			     address, addrlen);
> +}
> +EXPORT_SYMBOL(security_sctp_bind_connect);
> +
> +void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> +			    struct sock *newsk)
> +{
> +	call_void_hook(sctp_sk_clone, ep, sk, newsk);
> +}
> +EXPORT_SYMBOL(security_sctp_sk_clone);
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_INFINIBAND

Powered by blists - more mailing lists