Message-ID: <1511285851.7228.6.camel@btinternet.com>
Date:   Tue, 21 Nov 2017 17:37:31 +0000
From:   Richard Haines <richard_c_haines@...nternet.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     selinux@...ho.nsa.gov, netdev@...r.kernel.org,
        linux-sctp@...r.kernel.org, linux-security-module@...r.kernel.org,
        Vlad Yasevich <vyasevich@...il.com>, nhorman@...driver.com,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Eric Paris <eparis@...isplace.org>, marcelo.leitner@...il.com
Subject: Re: [RFC PATCH 5/5] selinux: Add SCTP support

On Mon, 2017-11-20 at 16:55 -0500, Paul Moore wrote:
> On Tue, Nov 14, 2017 at 4:52 PM, Richard Haines
> <richard_c_haines@...nternet.com> wrote:
> > 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@...nternet.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@...nternet.com> wrote:
> > > > > > The SELinux SCTP implementation is explained in:
> > > > > > Documentation/security/SELinux-sctp.txt
> > > > > > 
> > > > > > Signed-off-by: Richard Haines <richard_c_haines@...nternet.
> > > > > > 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;
	}

Powered by blists - more mailing lists