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

[richard_c_haines@btinternet.com (Richard Haines)]

On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
> <richard_c_haines@btinternet.com> wrote:
> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> > > <richard_c_haines@btinternet.com> 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.



> 
> > > > +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 majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html