From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Subject: Re: [RFC PATCH 5/5] selinux: Add SCTP support Date: Mon, 13 Nov 2017 17:40:57 -0500 Message-ID: References: <20171017135953.4419-1-richard_c_haines@btinternet.com> <1510610721.3652.8.camel@btinternet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: selinux@tycho.nsa.gov, netdev@vger.kernel.org, linux-sctp@vger.kernel.org, linux-security-module@vger.kernel.org, Vlad Yasevich , nhorman@tuxdriver.com, Stephen Smalley , Eric Paris , marcelo.leitner@gmail.com To: Richard Haines Return-path: In-Reply-To: <1510610721.3652.8.camel@btinternet.com> Sender: owner-linux-security-module@vger.kernel.org List-Id: netdev.vger.kernel.org On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines wrote: > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote: >> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines >> wrote: >> > The SELinux SCTP implementation is explained in: >> > Documentation/security/SELinux-sctp.txt >> > >> > Signed-off-by: Richard Haines >> > --- >> > 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? >> > +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. >> > + 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. -- paul moore www.paul-moore.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Date: Mon, 13 Nov 2017 22:40:57 +0000 Subject: Re: [RFC PATCH 5/5] selinux: Add SCTP support Message-Id: List-Id: References: <20171017135953.4419-1-richard_c_haines@btinternet.com> <1510610721.3652.8.camel@btinternet.com> In-Reply-To: <1510610721.3652.8.camel@btinternet.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-security-module@vger.kernel.org On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines wrote: > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote: >> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines >> wrote: >> > The SELinux SCTP implementation is explained in: >> > Documentation/security/SELinux-sctp.txt >> > >> > Signed-off-by: Richard Haines >> > --- >> > 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? >> > +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. >> > + 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. -- paul moore www.paul-moore.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: paul@paul-moore.com (Paul Moore) Date: Mon, 13 Nov 2017 17:40:57 -0500 Subject: [RFC PATCH 5/5] selinux: Add SCTP support In-Reply-To: <1510610721.3652.8.camel@btinternet.com> References: <20171017135953.4419-1-richard_c_haines@btinternet.com> <1510610721.3652.8.camel@btinternet.com> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines wrote: > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote: >> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines >> wrote: >> > The SELinux SCTP implementation is explained in: >> > Documentation/security/SELinux-sctp.txt >> > >> > Signed-off-by: Richard Haines >> > --- >> > 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? >> > +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. >> > + 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. -- paul moore www.paul-moore.com -- 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