Re: [PATCH V4 4/4] selinux: Add SCTP support

[Richard Haines]

On Wed, 2018-01-10 at 11:37 -0500, Paul Moore wrote:
> On Sat, Dec 30, 2017 at 12:20 PM, Richard Haines
>  wrote:
> > The SELinux SCTP implementation is explained in:
> > Documentation/security/SELinux-sctp.rst
> > 
> > Signed-off-by: Richard Haines 
> > ---
> >  Documentation/security/SELinux-sctp.rst | 157 ++
> >  security/selinux/hooks.c| 280
> > +---
> >  security/selinux/include/classmap.h |   2 +-
> >  security/selinux/include/netlabel.h |  21 ++-
> >  security/selinux/include/objsec.h   |   4 +
> >  security/selinux/netlabel.c | 138 ++--
> >  6 files changed, 570 insertions(+), 32 deletions(-)
> >  create mode 100644 Documentation/security/SELinux-sctp.rst
> 
> ...
> 
> > +/**
> > + * selinux_netlbl_socket_connect - Label a client-side socket on
> > connect
> > + * @sk: the socket to label
> > + * @addr: the destination address
> > + *
> > + * Description:
> > + * Attempt to label a connected socket with NetLabel using the
> > given address.
> > + * Returns zero values on success, negative values on failure.
> > + *
> > + */
> > +int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr
> > *addr)
> > +{
> > +   int rc;
> > +   struct sk_security_struct *sksec = sk->sk_security;
> > +
> > +   if (sksec->nlbl_state != NLBL_REQSKB &&
> > +   sksec->nlbl_state != NLBL_CONNLABELED)
> > +   return 0;
> > +
> > +   lock_sock(sk);
> > +   rc = selinux_netlbl_socket_connect_helper(sk, addr);
> > release_sock(sk);
> > +
> > return rc;
> >  }
> > +
> > +/**
> > + * selinux_netlbl_socket_connect_locked - Label a client-side
> > socket on
> > + * connect
> > + * @sk: the socket to label
> > + * @addr: the destination address
> > + *
> > + * Description:
> > + * Attempt to label a connected socket that already has the socket
> > locked
> > + * with NetLabel using the given address.
> > + * Returns zero values on success, negative values on failure.
> > + *
> > + */
> > +int selinux_netlbl_socket_connect_locked(struct sock *sk,
> > +struct sockaddr *addr)
> > +{
> > +   struct sk_security_struct *sksec = sk->sk_security;
> > +
> > +   if (sksec->nlbl_state != NLBL_REQSKB &&
> > +   sksec->nlbl_state != NLBL_CONNLABELED)
> > +   return 0;
> > +
> > +   return selinux_netlbl_socket_connect_helper(sk, addr);
> > +}
> 
> [Sorry for the review delay, the holidays and some associated travel
> made it hard to find some quiet time to look at the latest patches.]
> 
> I probably should have been a bit more clear in my last comment, but
> what I had in mind was something like the following:
> 
> int selinux_netlbl_socket_connect_locked(...)
> {
> if (sksec->nlbl_state ...)
> return 0;
> 
> return selinux_netlbl_socket_connect_helper();
> }
> 
> int selinux_netlbl_socket_connect(...)
> {
> int rc;
> 
> lock_sock();
> rc = selinux_netlbl_socket_connect_locked();
> release_sock();
> 
> return rc;
> }
> 
> Yes, you do end up checking nlbl_state while the socket lock is held,
> but I believe the benefit of consolidating the code outweighs any
> additional overhead (I believe it would be "noise" anyway).

Okay I'll send an updated [PATCH V5 4/4] tomorrow.

> 
> Otherwise, this all looks good to me.
> 

Re: [PATCH V4 4/4] selinux: Add SCTP support

[Paul Moore]

On Sat, Dec 30, 2017 at 12:20 PM, Richard Haines
 wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.rst
>
> Signed-off-by: Richard Haines 
> ---
>  Documentation/security/SELinux-sctp.rst | 157 ++
>  security/selinux/hooks.c| 280 
> +---
>  security/selinux/include/classmap.h |   2 +-
>  security/selinux/include/netlabel.h |  21 ++-
>  security/selinux/include/objsec.h   |   4 +
>  security/selinux/netlabel.c | 138 ++--
>  6 files changed, 570 insertions(+), 32 deletions(-)
>  create mode 100644 Documentation/security/SELinux-sctp.rst

...

> +/**
> + * selinux_netlbl_socket_connect - Label a client-side socket on connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> +{
> +   int rc;
> +   struct sk_security_struct *sksec = sk->sk_security;
> +
> +   if (sksec->nlbl_state != NLBL_REQSKB &&
> +   sksec->nlbl_state != NLBL_CONNLABELED)
> +   return 0;
> +
> +   lock_sock(sk);
> +   rc = selinux_netlbl_socket_connect_helper(sk, addr);
> release_sock(sk);
> +
> return rc;
>  }
> +
> +/**
> + * selinux_netlbl_socket_connect_locked - Label a client-side socket on
> + * connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket that already has the socket locked
> + * with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect_locked(struct sock *sk,
> +struct sockaddr *addr)
> +{
> +   struct sk_security_struct *sksec = sk->sk_security;
> +
> +   if (sksec->nlbl_state != NLBL_REQSKB &&
> +   sksec->nlbl_state != NLBL_CONNLABELED)
> +   return 0;
> +
> +   return selinux_netlbl_socket_connect_helper(sk, addr);
> +}

[Sorry for the review delay, the holidays and some associated travel
made it hard to find some quiet time to look at the latest patches.]

I probably should have been a bit more clear in my last comment, but
what I had in mind was something like the following:

int selinux_netlbl_socket_connect_locked(...)
{
if (sksec->nlbl_state ...)
return 0;

return selinux_netlbl_socket_connect_helper();
}

int selinux_netlbl_socket_connect(...)
{
int rc;

lock_sock();
rc = selinux_netlbl_socket_connect_locked();
release_sock();

return rc;
}

Yes, you do end up checking nlbl_state while the socket lock is held,
but I believe the benefit of consolidating the code outweighs any
additional overhead (I believe it would be "noise" anyway).

Otherwise, this all looks good to me.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4 4/4] selinux: Add SCTP support

[Marcelo Ricardo Leitner]

On Sat, Dec 30, 2017 at 05:20:35PM +, Richard Haines wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.rst
> 
> Signed-off-by: Richard Haines 

Reviewed-by: Marcelo Ricardo Leitner 

Thanks Richard.

> ---
>  Documentation/security/SELinux-sctp.rst | 157 ++
>  security/selinux/hooks.c| 280 
> +---
>  security/selinux/include/classmap.h |   2 +-
>  security/selinux/include/netlabel.h |  21 ++-
>  security/selinux/include/objsec.h   |   4 +
>  security/selinux/netlabel.c | 138 ++--
>  6 files changed, 570 insertions(+), 32 deletions(-)
>  create mode 100644 Documentation/security/SELinux-sctp.rst
> 
> diff --git a/Documentation/security/SELinux-sctp.rst 
> b/Documentation/security/SELinux-sctp.rst
> new file mode 100644
> index 000..2f66bf3
> --- /dev/null
> +++ b/Documentation/security/SELinux-sctp.rst
> @@ -0,0 +1,157 @@
> +SCTP SELinux Support
> +=
> +
> +Security Hooks
> +===
> +
> +``Documentation/security/LSM-sctp.rst`` describes the following SCTP security
> +hooks with the SELinux specifics expanded below::
> +
> +security_sctp_assoc_request()
> +security_sctp_bind_connect()
> +security_sctp_sk_clone()
> +security_inet_conn_established()
> +
> +
> +security_sctp_assoc_request()
> +-
> +Passes the ``@ep`` and ``@chunk->skb`` of 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 ``@skb``. 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.
> +
> +
> +security_sctp_bind_connect()
> +-
> +Checks permissions required for ipv4/ipv6 addresses based on the ``@optname``
> +as follows::
> +
> +  --
> +  |   BIND Permission 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 Permission 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   |
> +  --
> +
> +
> +``Documentation/security/LSM-sctp.rst`` gives a summary of the ``@optname``
> +entries and also describes ASCONF chunk processing when Dynamic Address
> +Reconfiguration is enabled.
> +
> +
> +security_sctp_sk_clone()
> +-
> +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 current sctp endpoint structure.
> +@sk - pointer to current sock structure.
> +@sk - pointer to new sock structure.
> +
> +
> +security_inet_conn_established()
> +-
> +Called when a COOKIE ACK is received where it sets the connection's peer sid
> +to that in ``@skb``::
> +
> +@sk  - pointer to sock structure.
> +@skb - pointer to skbuff of the 

[PATCH V4 4/4] selinux: Add SCTP support

[Richard Haines]

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

Signed-off-by: Richard Haines 
---
 Documentation/security/SELinux-sctp.rst | 157 ++
 security/selinux/hooks.c| 280 +---
 security/selinux/include/classmap.h |   2 +-
 security/selinux/include/netlabel.h |  21 ++-
 security/selinux/include/objsec.h   |   4 +
 security/selinux/netlabel.c | 138 ++--
 6 files changed, 570 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/security/SELinux-sctp.rst

diff --git a/Documentation/security/SELinux-sctp.rst 
b/Documentation/security/SELinux-sctp.rst
new file mode 100644
index 000..2f66bf3
--- /dev/null
+++ b/Documentation/security/SELinux-sctp.rst
@@ -0,0 +1,157 @@
+SCTP SELinux Support
+=
+
+Security Hooks
+===
+
+``Documentation/security/LSM-sctp.rst`` describes the following SCTP security
+hooks with the SELinux specifics expanded below::
+
+security_sctp_assoc_request()
+security_sctp_bind_connect()
+security_sctp_sk_clone()
+security_inet_conn_established()
+
+
+security_sctp_assoc_request()
+-
+Passes the ``@ep`` and ``@chunk->skb`` of 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 ``@skb``. 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.
+
+
+security_sctp_bind_connect()
+-
+Checks permissions required for ipv4/ipv6 addresses based on the ``@optname``
+as follows::
+
+  --
+  |   BIND Permission 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 Permission 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   |
+  --
+
+
+``Documentation/security/LSM-sctp.rst`` gives a summary of the ``@optname``
+entries and also describes ASCONF chunk processing when Dynamic Address
+Reconfiguration is enabled.
+
+
+security_sctp_sk_clone()
+-
+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 current sctp endpoint structure.
+@sk - pointer to current sock structure.
+@sk - pointer to new sock structure.
+
+
+security_inet_conn_established()
+-
+Called when a COOKIE ACK is received where it sets the connection's peer sid
+to that in ``@skb``::
+
+@sk  - pointer to sock structure.
+@skb - pointer to skbuff of the COOKIE ACK packet.
+
+
+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;
+
+SELinux SCTP support adds the ``name_connect`` permission for connecting
+to a