On Wed, 2017-02-22 at 17:03 +0000, Richard Haines wrote: > Add SELinux support for the SCTP protocol. The SELinux-sctp.txt > document > describes how the patch has been implemented. > > Patches to assist the testing of this kernel patch are: > 1) Support new SCTP portcon statement used by SCTP tests in the > selinux-testsuite [1]. > 2) Add SCTP tests to the selinux-testsuite [2]. > > Built and tested on Fedora 25 with linux-4.9.9 kernel. Need to re-base and test on a suitable upstream tree (maybe security next or selinux next). Since the extended socket class policy capability has been merged, you can leverage it and drop the duplicated portions. > > [1] http://arctic.selinuxproject.org/~rhaines/selinux-sctp/selinux-Ad > d-support-for-the-SCTP-portcon-keyword.patch > [2] http://arctic.selinuxproject.org/~rhaines/selinux-sctp/selinux- > testsuite-Add-SCTP-test-support.patch I wouldn't include URLs for these userspace patches in the patch description or in-tree documentation; you can note them in your cover letter posting as an aid to testing but they shouldn't be part of the permanent history since they will presumably be upstreamed too. > > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx> > --- > Documentation/security/SELinux-sctp.txt | 178 > ++++++++++++++++++++++++++ > include/net/sctp/structs.h | 7 ++ > net/sctp/sm_make_chunk.c | 12 ++ > net/sctp/sm_statefuns.c | 20 +++ > net/sctp/socket.c | 42 ++++++- I'd either move the net/sctp changes into the first patch that defines the LSM hooks or move them into their own separate patch between these two patches. > security/selinux/hooks.c | 213 > ++++++++++++++++++++++++++++++-- > security/selinux/include/classmap.h | 3 + > 7 files changed, 466 insertions(+), 9 deletions(-) > create mode 100644 Documentation/security/SELinux-sctp.txt > > diff --git a/Documentation/security/SELinux-sctp.txt > b/Documentation/security/SELinux-sctp.txt > new file mode 100644 > index 0000000..ada666f > --- /dev/null > +++ b/Documentation/security/SELinux-sctp.txt > @@ -0,0 +1,178 @@ > + SCTP SELinux Support > + ====================== > + > +Testing - selinux-testsuite > +============================ > +There is a patch available that adds SCTP/SELinux tests to the > +selinux-testsuite. This is available from: > + > +http://arctic.selinuxproject.org/~rhaines/selinux-sctp/selinux-tests > uite-Add-SCTP-test-support.patch > + > +These tests require libsepol to support the new sctp portcon > statement. > +A patch is available from: > + > +http://arctic.selinuxproject.org/~rhaines/selinux-sctp/selinux-Add- > support-for-the-SCTP-portcon-keyword.patch Ditto here; I wouldn't include these patch URLs in the in-tree documentation since the patches should get upstreamed. > + > +Before running these tests, read the selinux-testsuite/README.sctp > as it is > +also possible to run the lksctp-tools/src/func_tests that are > available from: > + > +https://github.com/sctp/lksctp-tools > + > + > +Security Hooks > +=============== > + > +The Documentation/security/LSM-sctp.txt document describes how the > following > +sctp security hooks are utilised: > + security_sctp_assoc_request() > + security_sctp_accept_conn() > + security_sctp_sk_clone() > + security_sctp_addr_list() > + > + > +Policy Statements > +================== > +A new object class "sctp_socket" has been introduced with the > following SCTP > +specific permissions: association bindx_add connectx > + > +The permissions are explained in the sections below. > + > +Kernel policy language > +----------------------- > +class sctp_socket > +class sctp_socket inherits socket { node_bind name_connect > association > + bindx_add connectx } > + > +CIL policy language > +-------------------- > +(classcommon sctp_socket socket) > +(class sctp_socket (node_bind name_connect association bindx_add > connectx)) > +(classorder (unordered sctp_socket)) > + > +If the SELinux userspace tools have been updated, then the portcon > statement > +may be used as shown in the following example: > + (portcon sctp (1024 1035) (system_u object_r sctp_port_t ((s0) > (s0)))) Not a strong objection, but not sure if CIL documentation belongs in the kernel tree. <snip> > + > +SCTP Peer Labeling and Permission Checks > +========================================= > +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, the "association" permission will be > checked as > +follows: > + > + allow socket_t peer_t : sctp_socket { association }; > + > +This allows policy to decide whether to allow or deny associations > from peers, > +as there can be multiple associations on a single socket. These > associations > +could come from any of the policy allowed peers, however it could be > that > +these are required by other services but sctp associations are not > allowed > +from all of them. I'm still confused by this check. We should already be performing a peer recv check between the socket label and the peer label, so we don't need to duplicate that check. What would make sense would be some kind of permission check between the peer label from the first association, which is the one you save and return to userspace for SO_PEERSEC, and the peer label of any subsequent associations established on the socket. That would allow policy to prohibit or restrict mixing of associations with different peer labels on the same socket (since effectively that permits impersonation of another peer label to userspace components). But instead you are always checking between the socket label and the saved peer label from the first association. So you'll just keep repeating the same permission check for every association. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7f4387f..c0be892 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c <snip> > @@ -4827,6 +4879,149 @@ static void selinux_sock_graft(struct sock > *sk, struct socket *parent) > sksec->sclass = isec->sclass; > } > > +static int selinux_sctp_assoc_request(struct sock *sk, struct > sk_buff *skb) > +{ > + struct sk_security_struct *sksec = sk->sk_security; > + struct common_audit_data ad; > + struct lsm_network_audit net = {0,}; > + u8 peerlbl_active; > + int err; > + > + peerlbl_active = selinux_peerlbl_enabled(); > + > + if (sksec->peer_sid == SECINITSID_UNLABELED && > peerlbl_active) { > + /* Here because this is the first association on > this > + * socket that is always unlabeled, therefore set > + * sksec->peer_sid to new peer ctx. For further info > see: > + * Documentation/security/SELinux-sctp.txt > + */ > + err = selinux_skb_peerlbl_sid(skb, sk->sk_family, > + &sksec->peer_sid); > + if (err) > + return err; > + } > + > + ad.type = LSM_AUDIT_DATA_NET; > + ad.u.net = &net; > + ad.u.net->sk = sk; > + > + err = avc_has_perm(sksec->sid, sksec->peer_sid, sksec- > >sclass, > + SCTP_SOCKET__ASSOCIATION, &ad); As above, you'll end up performing the same permission check repeatedly here for every association, even when the association itself would have a different peer label. And this permission check seems to be no different than the peer recv check (same SID arguments). What would make sense is something like: u32 peer_sid = SECINITSID_UNLABELED; if (peerlbl_active) { err = selinux_skb_peerlbl_sid(skb, sk->sk_family, &peer_sid); if (err) return err; } if (sksec->peer_sid == SECINITSID_UNLABELED) sksec->peer_sid = peer_sid; else if (sksec->peer_sid != peer_sid) { err = avc_has_perm(sksec->peer_sid, peer_sid, sksec->sclass, SCTP_SOCKET_ASSOCIATION, &ad); if (err) return err; } return 0; This would allow preventing multiple associations with different peer labels, or controlling their inter-relationships. You don't need a socket-peer check here; that is already covered by the peer recv check. > + return err; > +} > + > +static int selinux_sctp_accept_conn(struct sctp_endpoint *ep, > + struct sk_buff *skb) > +{ > + struct sk_security_struct *sksec = ep->base.sk->sk_security; > + int err; > + u32 connsid; > + u32 peersid; > + > + /* Have COOKIE ECHO so compute the MLS component for the > connection > + * and store the information in ep. This will only be used > by > + * TCP/peeloff connections as they cause a new socket to be > generated. Not sure why you say TCP above. And won't this be true of accept()'d sockets too in addition to peeloff ones? > + * selinux_sctp_sk_clone() will then plug this into the new > socket > + * as described in Documentation/security/LSM-sctp.txt > + */ > + err = selinux_skb_peerlbl_sid(skb, ep->base.sk->sk_family, > &peersid); > + if (err) > + return err; > + > + err = selinux_conn_sid(sksec->sid, peersid, &connsid); > + if (err) > + return err; > + > + ep->secid = connsid; > + ep->peer_secid = peersid; > + > + return 0; > +} > + -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
