| Message ID | 166008317384.447519.7250489605112175227.stgit@olly (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: SCTP fixes, including ASCONF | expand |
On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote: > This patch makes two changes to how SELinux processes SCTP traffic: Why one patch for two independent changes? > > * Considering the multi-homed nature of SCTP, all SCTP traffic is > marked as NLBL_REQSKB from a NetLabel perspective so that > traffic is labeled on a per-packet basis using the destination IP, > and not the on-the-wire label cached at the socket layer. > > * New permissions have been added to the "sctp_socket" object class: > sctp_socket/{asconf_addip, asconf_connect}. These new permissions > are gated by the "sctp_asconf" SELinux policy capability, and > control the ability of ASCONF to add a new IP address to an > association and set the primary IP of the association. The ASCONF > access control points now work like the examples below; <socket> is > the local socket's label, <port> is the label of the network port, > and <peer> is the network peer label (dependent on the labeled > networking configuration). > > - legacy policy (no sctp_asconf) > > allow <socket> <port>:sctp_socket { name_connect }; > > - updated policy without labeled networking (enabled sctp_asconf) > > allow <socket> <port>:sctp_socket { asconf_connect }; > > - updated policy with labeled networking (enabled sctp_asconf) > > allow <peer> <socket>:sctp_socket { asconf_addip }; > allow <socket> <port>:sctp_socket { asconf_connect }; > > Cc: stable@vger.kernel.org > Fixes: d452930fd3b9 ("selinux: Add SCTP support") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 86 +++++++++++++++++++--------- > security/selinux/include/classmap.h | 3 + > security/selinux/include/netlabel.h | 1 > security/selinux/include/policycap.h | 1 > security/selinux/include/policycap_names.h | 3 + > security/selinux/include/security.h | 7 ++ > security/selinux/netlabel.c | 30 ++++++++-- > 7 files changed, 95 insertions(+), 36 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 1bbd53321d13..02751a66c5d8 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c <snip> > @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, > */ > err = selinux_netlbl_socket_connect_locked(sk, addr); > break; > + /* ASCONF checks (IETF RFC 5061) */ > + case SCTP_PARAM_ADD_IP: > + case SCTP_PARAM_SET_PRIMARY: > + if (pcap_asconf) { > + if (peerlbl) { > + err = sock_has_perm_subj(sksec->peer_sid, > + sk, SCTP_SOCKET__ASCONF_ADDIP); How about splitting this into SCTP_SOCKET__ADD_IP_FROM and SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the scontext is a peer label as with NODE__RECVFROM; and split so that one can allow SET_PRIMARY and not ADD_IP, as the former is much less of a security concern than the latter. For example, the client could first do connectx() with multiple addresses controlled by itself and then it might be desirable to allow the remote peer to switch between them via SET_PRIMARY, yet keep ADD_IP (which can actually modify the address list) disallowed. Also, using the "ADDIP" name for both is just misleading as SET_PRIMARY doesn't add any address, only switches between the already added ones (and note that these might be added not only via a previous ADD_IP, but also by the client itself via CONNECTX). > + if (err) > + return err; > + } > + perm = SCTP_SOCKET__ASCONF_CONNECT; Again, if it was up to me, I'd split the permission up into ADD_IP and SET_PRIMARY. And I'd like to reiterate once more that validating the port seems unnecessary here, since only the address can be changed via ASCONF, the port is the same for the whole association (just look at the definition of struct sctp_association for an indirect proof). If you insist on the name_connect semantics regardless, let's at least name the permissions "add_ip_name_connect" and "set_primary_name_connect" so that it's clear that the tcontext is a port context; otherwise they can be simply: allow <socket> self:sctp_socket { add_ip set_primary }; (Possibly all with the "asconf_" prefix for more clarity, but IMHO it's not necessary.) > + } else > + perm = SCTP_SOCKET__NAME_CONNECT; > + err = selinux_socket_connect_helper(sock, perm, > + addr, len); > + break; > } > > if (err) <snip> > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > index 1321f15799e2..28d0ead32416 100644 > --- a/security/selinux/netlabel.c > +++ b/security/selinux/netlabel.c I believe you forgot to modify also selinux_netlbl_sctp_assoc_request() in a similar fashion as the other functions below? > @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family) > */ > void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk) > { > - struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > - newsksec->nlbl_state = sksec->nlbl_state; > + /* SCTP is multi-homed so we must label each packet based on dest IP */ > + newsksec->nlbl_state = NLBL_REQSKB; > } > > /** > @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) > secattr = selinux_netlbl_sock_genattr(sk); > if (secattr == NULL) > return -ENOMEM; > + > + /* SCTP has the ability to communicate with multiple endpoints for a > + * given association so we need to force NLBL_REQSKB so that we always > + * label traffic based on the destination endpoint and not the > + * association's connection > + */ > + if (sk->sk_protocol == IPPROTO_SCTP) { > + sksec->nlbl_state = NLBL_REQSKB; > + return 0; > + } > + > rc = netlbl_sock_setattr(sk, family, secattr); > switch (rc) { > case 0: > @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk, > struct sk_security_struct *sksec = sk->sk_security; > struct netlbl_lsm_secattr *secattr; > > - /* connected sockets are allowed to disconnect when the address family > - * is set to AF_UNSPEC, if that is what is happening we want to reset > - * the socket */ > - if (addr->sa_family == AF_UNSPEC) { > + /* special handling for AF_UNSPEC and IPPROTO_SCTP: > + * - sockets are allowed to disconnect when the address family > + * is set to AF_UNSPEC, if that is what is happening we want to reset > + * the socket > + * - SCTP has the ability to communicate with multiple endpoints for > + * a given association so we need to force NLBL_REQSKB so that we > + * always label traffic based on the destination endpoint and not > + * the association's connection, see similar comment in > + * selinux_netlbl_socket_post_create() > + */ > + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) { > netlbl_sock_delattr(sk); > sksec->nlbl_state = NLBL_REQSKB; > rc = 0; Instead of overloading the AF_UNSPEC condition, how about just adding: if (sk->sk_protocol == IPPROTO_SCTP) return 0; ...before this one with a separate comment? Overloading the disconnect condition just makes the code harder to follow. Just bailing out in the connect helper should be enough once you also patch up selinux_netlbl_sctp_assoc_request(), since those are the only places that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or set any netlabel options on it (well, except when a user explicitly sets some via setsockopt(2), but in that case we probably shouldn't delete them even on a disconnect... it's corner-case bugs all the way down...). -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote: > > This patch makes two changes to how SELinux processes SCTP traffic: > > Why one patch for two independent changes? Taken separately the REQSKB change would be somewhat difficult to detect as a regular user, putting the two changes into a single patch means that one can use the presence of the "sctp_asconf" policy capability as an indicator that the REQSKB fix is present in the running kernel. > > @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, > > */ > > err = selinux_netlbl_socket_connect_locked(sk, addr); > > break; > > + /* ASCONF checks (IETF RFC 5061) */ > > + case SCTP_PARAM_ADD_IP: > > + case SCTP_PARAM_SET_PRIMARY: > > + if (pcap_asconf) { > > + if (peerlbl) { > > + err = sock_has_perm_subj(sksec->peer_sid, > > + sk, SCTP_SOCKET__ASCONF_ADDIP); > > How about splitting this into SCTP_SOCKET__ADD_IP_FROM and > SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the > scontext is a peer label as with NODE__RECVFROM ... I prefer the permission names as they are (without the "_FROM" suffix) as they better align with the SCTP RFCs. > ... and split so that one > can allow SET_PRIMARY and not ADD_IP, as the former is much less of a > security concern than the latter. I agree that ADD_IP is a much larger concern, arguably really the only concern, since you can't set the primary address without first adding it to the association. It may sound odd, but since SET_PRIMARY MUST (let's use some of that RFC language here <g>) come after a matching ADD_IP, I made them equivalent in this patch so that both would be covered by the same set of allow rules. In other words, the policy author really only needs to worry about the ADD_IP operation since that is the critical parameter/op. I did debate dropping the SET_PRIMARY entirely, but keeping it made the legacy case easier and in the peerlbl/TRUE case it still offers some value in the case where the SET_PRIMARY is done at a different time from the ADD_IP (potentially different network peer labels for each parameter/op). > Also, using the "ADDIP" name for both is just misleading as > SET_PRIMARY doesn't add any address, only switches between the already > added ones (and note that these might be added not only via a previous > ADD_IP, but also by the client itself via CONNECTX). Yes, I read the RFC too, but see my comments above. > > + if (err) > > + return err; > > + } > > + perm = SCTP_SOCKET__ASCONF_CONNECT; > > Again, if it was up to me, I'd split the permission up into ADD_IP and > SET_PRIMARY. And I'd like to reiterate once more that validating the > port seems unnecessary here ... To touch on the port issue quickly - while part of the motivation was to blend better with the existing legacy case, I think there may also be value in being able to control different ASCONF policies on a port-by-port basis. For example, maybe you want to allow pretty much anything from an ASCONF perspective for the http ports but you want to be much more restrictive about ssh. > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > > index 1321f15799e2..28d0ead32416 100644 > > --- a/security/selinux/netlabel.c > > +++ b/security/selinux/netlabel.c > > I believe you forgot to modify also > selinux_netlbl_sctp_assoc_request() in a similar fashion as the other > functions below? Good point. I suspect selinux_netlbl_sctp_assoc_request() will be greatly simplified too, that will be nice. > > @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family) > > */ > > void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk) > > { > > - struct sk_security_struct *sksec = sk->sk_security; > > struct sk_security_struct *newsksec = newsk->sk_security; > > > > - newsksec->nlbl_state = sksec->nlbl_state; > > + /* SCTP is multi-homed so we must label each packet based on dest IP */ > > + newsksec->nlbl_state = NLBL_REQSKB; > > } > > > > /** > > @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) > > secattr = selinux_netlbl_sock_genattr(sk); > > if (secattr == NULL) > > return -ENOMEM; > > + > > + /* SCTP has the ability to communicate with multiple endpoints for a > > + * given association so we need to force NLBL_REQSKB so that we always > > + * label traffic based on the destination endpoint and not the > > + * association's connection > > + */ > > + if (sk->sk_protocol == IPPROTO_SCTP) { > > + sksec->nlbl_state = NLBL_REQSKB; > > + return 0; > > + } > > + > > rc = netlbl_sock_setattr(sk, family, secattr); > > switch (rc) { > > case 0: > > @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk, > > struct sk_security_struct *sksec = sk->sk_security; > > struct netlbl_lsm_secattr *secattr; > > > > - /* connected sockets are allowed to disconnect when the address family > > - * is set to AF_UNSPEC, if that is what is happening we want to reset > > - * the socket */ > > - if (addr->sa_family == AF_UNSPEC) { > > + /* special handling for AF_UNSPEC and IPPROTO_SCTP: > > + * - sockets are allowed to disconnect when the address family > > + * is set to AF_UNSPEC, if that is what is happening we want to reset > > + * the socket > > + * - SCTP has the ability to communicate with multiple endpoints for > > + * a given association so we need to force NLBL_REQSKB so that we > > + * always label traffic based on the destination endpoint and not > > + * the association's connection, see similar comment in > > + * selinux_netlbl_socket_post_create() > > + */ > > + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) { > > netlbl_sock_delattr(sk); > > sksec->nlbl_state = NLBL_REQSKB; > > rc = 0; > > Instead of overloading the AF_UNSPEC condition, how about just adding: > > if (sk->sk_protocol == IPPROTO_SCTP) > return 0; > > ...before this one with a separate comment? Overloading the disconnect > condition just makes the code harder to follow. Heh. Okay, I see your point, but if that bit of code is too hard to follow for someone they are going to have a very rough time with the rest of the kernel code ;) I'm going to leave this as-is. > Just bailing out in > the connect helper should be enough once you also patch up > selinux_netlbl_sctp_assoc_request(), since those are the only places > that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or > set any netlabel options on it (well, except when a user explicitly > sets some via setsockopt(2), but in that case we probably shouldn't > delete them even on a disconnect... Just as a FYI, the socket_setsockopt hook prevents a user from overwriting an existing NetLabel socket option, and in the REQSKB case we either overwrite any socket inherited NetLabel socket options in the case of CIPSO or add another hop-by-hop option in the case of CALIPSO (it's quicker that way). We don't explicitly block a user form setting a on-the-wire packet label using setsockopt() in the case where no labeling is configured in case the application wants to manage things on their own, but we do provide a socket-level permission check (socket/setopt) so that policy developers can block this if they want.
On Thu, Aug 11, 2022 at 11:04 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote: ... > > I believe you forgot to modify also > > selinux_netlbl_sctp_assoc_request() in a similar fashion as the other > > functions below? > > Good point. I suspect selinux_netlbl_sctp_assoc_request() will be > greatly simplified too, that will be nice. I'm beginning to think the SELinux/SCTP code is cursed :/ That change brought up some other issues (more of the multiple-associations-on-a-single-socket variety) which I need to think about some more, so I'm putting this on the back-burner for a little bit while I sort out the io_uring and other things which are more straightforward and need to get into linux-next ASAP.
(Sorry for the late reply, I was on vacation last week.) On Thu, Aug 11, 2022 at 5:04 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote: > > > This patch makes two changes to how SELinux processes SCTP traffic: > > > > Why one patch for two independent changes? > > Taken separately the REQSKB change would be somewhat difficult to > detect as a regular user, putting the two changes into a single patch > means that one can use the presence of the "sctp_asconf" policy > capability as an indicator that the REQSKB fix is present in the > running kernel. Why would one need to detect it, though? For selinux-testsuite? For that it will be sufficient if the REQSKB fix is applied before the ASCONF fix. Anyway, this is bikeshedding and I don't insist on splitting the patches, just that I don't see an obvious reason so I'd like to understand why it's being done this way. > > > @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, > > > */ > > > err = selinux_netlbl_socket_connect_locked(sk, addr); > > > break; > > > + /* ASCONF checks (IETF RFC 5061) */ > > > + case SCTP_PARAM_ADD_IP: > > > + case SCTP_PARAM_SET_PRIMARY: > > > + if (pcap_asconf) { > > > + if (peerlbl) { > > > + err = sock_has_perm_subj(sksec->peer_sid, > > > + sk, SCTP_SOCKET__ASCONF_ADDIP); > > > > How about splitting this into SCTP_SOCKET__ADD_IP_FROM and > > SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the > > scontext is a peer label as with NODE__RECVFROM ... > > I prefer the permission names as they are (without the "_FROM" suffix) > as they better align with the SCTP RFCs. I don't see how the suffix is relevant to alignment to RFCs, but anyway I'm beginning to doubt if the _FROM suffix makes that much sense... Let's leave it at ASCONF_SOMETHING then. (But see below why I still think it should be split into two permissions or apply only to ADD_IP.) > > ... and split so that one > > can allow SET_PRIMARY and not ADD_IP, as the former is much less of a > > security concern than the latter. > > I agree that ADD_IP is a much larger concern, arguably really the only > concern, since you can't set the primary address without first adding > it to the association. It may sound odd, but since SET_PRIMARY MUST > (let's use some of that RFC language here <g>) come after a matching > ADD_IP, I don't think that's necessarily true. 5.4 says "A sender MUST only send a set primary request to an address that is already considered part of the association." But the addresses in the association don't all have to come from ASCONF, they can be also populated by the client itself via CONNECTX. So unless I'm mistaken, it is possible to open a multi-address association via CONNECTX and then let the remote peer indicate its preferred primary address via SET_PRIMARY. If (at SELinux level) I wanted to allow this scenario, but prevent the remote peer from adding any extra addresses to the association, I wouldn't be able to do so with this patch. > I made them equivalent in this patch so that both would be > covered by the same set of allow rules. In other words, the policy > author really only needs to worry about the ADD_IP operation since > that is the critical parameter/op. I did debate dropping the > SET_PRIMARY entirely, but keeping it made the legacy case easier and > in the peerlbl/TRUE case it still offers some value in the case where > the SET_PRIMARY is done at a different time from the ADD_IP > (potentially different network peer labels for each parameter/op). > > > Also, using the "ADDIP" name for both is just misleading as > > SET_PRIMARY doesn't add any address, only switches between the already > > added ones (and note that these might be added not only via a previous > > ADD_IP, but also by the client itself via CONNECTX). > > Yes, I read the RFC too, but see my comments above. > > > > + if (err) > > > + return err; > > > + } > > > + perm = SCTP_SOCKET__ASCONF_CONNECT; > > > > Again, if it was up to me, I'd split the permission up into ADD_IP and > > SET_PRIMARY. And I'd like to reiterate once more that validating the > > port seems unnecessary here ... > > To touch on the port issue quickly - while part of the motivation was > to blend better with the existing legacy case, I think there may also > be value in being able to control different ASCONF policies on a > port-by-port basis. For example, maybe you want to allow pretty much > anything from an ASCONF perspective for the http ports but you want to > be much more restrictive about ssh. Hmm, I guess that might be a valid use case indeed... Though I wonder if we shouldn't keep the "name" part in the permission name. At least I get the impression that the "name" in "name_connect" and "name_bind" means we are checking against a port's label. Sorry if that's a misunderstanding. > > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > > > index 1321f15799e2..28d0ead32416 100644 > > > --- a/security/selinux/netlabel.c > > > +++ b/security/selinux/netlabel.c > > > > I believe you forgot to modify also > > selinux_netlbl_sctp_assoc_request() in a similar fashion as the other > > functions below? > > Good point. I suspect selinux_netlbl_sctp_assoc_request() will be > greatly simplified too, that will be nice. > > > > @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family) > > > */ > > > void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk) > > > { > > > - struct sk_security_struct *sksec = sk->sk_security; > > > struct sk_security_struct *newsksec = newsk->sk_security; > > > > > > - newsksec->nlbl_state = sksec->nlbl_state; > > > + /* SCTP is multi-homed so we must label each packet based on dest IP */ > > > + newsksec->nlbl_state = NLBL_REQSKB; > > > } > > > > > > /** > > > @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) > > > secattr = selinux_netlbl_sock_genattr(sk); > > > if (secattr == NULL) > > > return -ENOMEM; > > > + > > > + /* SCTP has the ability to communicate with multiple endpoints for a > > > + * given association so we need to force NLBL_REQSKB so that we always > > > + * label traffic based on the destination endpoint and not the > > > + * association's connection > > > + */ > > > + if (sk->sk_protocol == IPPROTO_SCTP) { > > > + sksec->nlbl_state = NLBL_REQSKB; > > > + return 0; > > > + } > > > + > > > rc = netlbl_sock_setattr(sk, family, secattr); > > > switch (rc) { > > > case 0: > > > @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk, > > > struct sk_security_struct *sksec = sk->sk_security; > > > struct netlbl_lsm_secattr *secattr; > > > > > > - /* connected sockets are allowed to disconnect when the address family > > > - * is set to AF_UNSPEC, if that is what is happening we want to reset > > > - * the socket */ > > > - if (addr->sa_family == AF_UNSPEC) { > > > + /* special handling for AF_UNSPEC and IPPROTO_SCTP: > > > + * - sockets are allowed to disconnect when the address family > > > + * is set to AF_UNSPEC, if that is what is happening we want to reset > > > + * the socket > > > + * - SCTP has the ability to communicate with multiple endpoints for > > > + * a given association so we need to force NLBL_REQSKB so that we > > > + * always label traffic based on the destination endpoint and not > > > + * the association's connection, see similar comment in > > > + * selinux_netlbl_socket_post_create() > > > + */ > > > + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) { > > > netlbl_sock_delattr(sk); > > > sksec->nlbl_state = NLBL_REQSKB; > > > rc = 0; > > > > Instead of overloading the AF_UNSPEC condition, how about just adding: > > > > if (sk->sk_protocol == IPPROTO_SCTP) > > return 0; > > > > ...before this one with a separate comment? Overloading the disconnect > > condition just makes the code harder to follow. > > Heh. Okay, I see your point, but if that bit of code is too hard to > follow for someone they are going to have a very rough time with the > rest of the kernel code ;) > > I'm going to leave this as-is. Well, yes, the kernel contains a lot of ugly code, but that's no excuse to add more... Yet, this is another bikeshedding that is maybe worth raising but not worth arguing about, so I'm not going to scratch it further. > > Just bailing out in > > the connect helper should be enough once you also patch up > > selinux_netlbl_sctp_assoc_request(), since those are the only places > > that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or > > set any netlabel options on it (well, except when a user explicitly > > sets some via setsockopt(2), but in that case we probably shouldn't > > delete them even on a disconnect... > > Just as a FYI, the socket_setsockopt hook prevents a user from > overwriting an existing NetLabel socket option, and in the REQSKB case > we either overwrite any socket inherited NetLabel socket options in > the case of CIPSO or add another hop-by-hop option in the case of > CALIPSO (it's quicker that way). We don't explicitly block a user > form setting a on-the-wire packet label using setsockopt() in the case > where no labeling is configured in case the application wants to > manage things on their own, but we do provide a socket-level > permission check (socket/setopt) so that policy developers can block > this if they want. Well, yes, but none of that explains why we call netlbl_sock_delattr(sk) unconditionally on disconnect. Shouldn't the whole AF_UNSPEC block be executed only if sksec->nlbl_state == NLBL_CONNLABELED? Because of the guard in selinux_netlbl_socket_connect_locked() the state can be only either NLBL_CONNLABELED or NLBL_REQSKB here. In the case of NLBL_CONNLABELED, deleting the NetLabel options is logical, but in the case of NLBL_REQSKB we would be deleting options set by the user, which doesn't seem right. It is a pre-existing bug, but you are trying to exacerbate it even more by reusing the conditional for SCTP, so perhaps worth addressing beforehand. One more side thought: It's weird that currently we will allow setting the socket options in case of NLBL_REQSKB, but not in the case where the socket has a NETLBL_NLTYPE_UNLABELED entry. In this case there are no options set on the socket by NetLabel and it is marked as NLBL_LABELED, which means that selinux_netlbl_socket_setsockopt() will reject setting NetLabel options on it. Oh, and while I'm at it - does anybody know what is the intent behind the mysterious if-else in selinux_netlbl_inet_csk_clone()? -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1bbd53321d13..02751a66c5d8 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4530,7 +4530,7 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec, secclass, NULL, socksid); } -static int sock_has_perm(struct sock *sk, u32 perms) +static int sock_has_perm_subj(u32 subj, struct sock *sk, u32 perms) { struct sk_security_struct *sksec = sk->sk_security; struct common_audit_data ad; @@ -4544,8 +4544,12 @@ static int sock_has_perm(struct sock *sk, u32 perms) ad.u.net->sk = sk; return avc_has_perm(&selinux_state, - current_sid(), sksec->sid, sksec->sclass, perms, - &ad); + subj, sksec->sid, sksec->sclass, perms, &ad); +} + +static int sock_has_perm(struct sock *sk, u32 perms) +{ + return sock_has_perm_subj(current_sid(), sk, perms); } static int selinux_socket_create(int family, int type, @@ -4752,16 +4756,13 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in /* This supports connect(2) and SCTP connect services such as sctp_connectx(3) * and sctp_sendmsg(3) as described in Documentation/security/SCTP.rst */ -static int selinux_socket_connect_helper(struct socket *sock, +static int selinux_socket_connect_helper(struct socket *sock, u32 perm, struct sockaddr *address, int addrlen) { struct sock *sk = sock->sk; struct sk_security_struct *sksec = sk->sk_security; int err; - err = sock_has_perm(sk, SOCKET__CONNECT); - if (err) - return err; if (addrlen < offsetofend(struct sockaddr, sa_family)) return -EINVAL; @@ -4783,7 +4784,7 @@ static int selinux_socket_connect_helper(struct socket *sock, struct sockaddr_in *addr4 = NULL; struct sockaddr_in6 *addr6 = NULL; unsigned short snum; - u32 sid, perm; + u32 sid; /* sctp_connectx(3) calls via selinux_sctp_bind_connect() * that validates multiple connect addresses. Because of this @@ -4817,18 +4818,6 @@ static int selinux_socket_connect_helper(struct socket *sock, if (err) return err; - switch (sksec->sclass) { - case SECCLASS_TCP_SOCKET: - perm = TCP_SOCKET__NAME_CONNECT; - break; - case SECCLASS_DCCP_SOCKET: - perm = DCCP_SOCKET__NAME_CONNECT; - break; - case SECCLASS_SCTP_SOCKET: - perm = SCTP_SOCKET__NAME_CONNECT; - break; - } - ad.type = LSM_AUDIT_DATA_NET; ad.u.net = &net; ad.u.net->dport = htons(snum); @@ -4847,9 +4836,26 @@ static int selinux_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen) { int err; + u32 perm; struct sock *sk = sock->sk; + struct sk_security_struct *sksec; - err = selinux_socket_connect_helper(sock, address, addrlen); + err = sock_has_perm(sk, SOCKET__CONNECT); + if (err) + return err; + sksec = sk->sk_security; + switch (sksec->sclass) { + case SECCLASS_TCP_SOCKET: + perm = TCP_SOCKET__NAME_CONNECT; + break; + case SECCLASS_DCCP_SOCKET: + perm = DCCP_SOCKET__NAME_CONNECT; + break; + case SECCLASS_SCTP_SOCKET: + perm = SCTP_SOCKET__NAME_CONNECT; + break; + } + err = selinux_socket_connect_helper(sock, perm, address, addrlen); if (err) return err; @@ -5360,17 +5366,24 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, int addrlen) { int len, err = 0, walk_size = 0; + int pcap_asconf; + int peerlbl; + u32 perm; void *addr_buf; struct sockaddr *addr; struct socket *sock; + struct sk_security_struct *sksec; if (!selinux_policycap_extsockclass()) return 0; - /* Process one or more addresses that may be IPv4 or IPv6 */ + pcap_asconf = selinux_policycap_sctp_asconf(); + peerlbl = selinux_peerlbl_enabled(); + sksec = sk->sk_security; sock = sk->sk_socket; addr_buf = address; + /* Process one or more addresses that may be IPv4 or IPv6 */ while (walk_size < addrlen) { if (walk_size + sizeof(sa_family_t) > addrlen) return -EINVAL; @@ -5393,18 +5406,21 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, err = -EINVAL; switch (optname) { - /* Bind checks */ + /* bind() style checks */ case SCTP_PRIMARY_ADDR: case SCTP_SET_PEER_PRIMARY_ADDR: case SCTP_SOCKOPT_BINDX_ADD: err = selinux_socket_bind(sock, addr, len); break; - /* Connect checks */ + /* connect() style checks */ case SCTP_SOCKOPT_CONNECTX: - case SCTP_PARAM_SET_PRIMARY: - case SCTP_PARAM_ADD_IP: case SCTP_SENDMSG_CONNECT: - err = selinux_socket_connect_helper(sock, addr, len); + err = sock_has_perm(sk, SOCKET__CONNECT); + if (err) + return err; + err = selinux_socket_connect_helper(sock, + SCTP_SOCKET__NAME_CONNECT, + addr, len); if (err) return err; @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, */ err = selinux_netlbl_socket_connect_locked(sk, addr); break; + /* ASCONF checks (IETF RFC 5061) */ + case SCTP_PARAM_ADD_IP: + case SCTP_PARAM_SET_PRIMARY: + if (pcap_asconf) { + if (peerlbl) { + err = sock_has_perm_subj(sksec->peer_sid, + sk, SCTP_SOCKET__ASCONF_ADDIP); + if (err) + return err; + } + perm = SCTP_SOCKET__ASCONF_CONNECT; + } else + perm = SCTP_SOCKET__NAME_CONNECT; + err = selinux_socket_connect_helper(sock, perm, + addr, len); + break; } if (err) diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index ff757ae5f253..8d4e3b4d160e 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -179,7 +179,8 @@ const struct security_class_mapping secclass_map[] = { { COMMON_CAP2_PERMS, NULL } }, { "sctp_socket", { COMMON_SOCK_PERMS, - "node_bind", "name_connect", "association", NULL } }, + "node_bind", "name_connect", "association", "asconf_addip", + "asconf_connect", NULL } }, { "icmp_socket", { COMMON_SOCK_PERMS, "node_bind", NULL } }, diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h index 4d0456d3d459..32aa8d4724fa 100644 --- a/security/selinux/include/netlabel.h +++ b/security/selinux/include/netlabel.h @@ -55,7 +55,6 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock, int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr); int selinux_netlbl_socket_connect_locked(struct sock *sk, struct sockaddr *addr); - #else static inline void selinux_netlbl_cache_invalidate(void) { diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index f35d3458e71d..6aaee9e12812 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -12,6 +12,7 @@ enum { POLICYDB_CAP_NNP_NOSUID_TRANSITION, POLICYDB_CAP_GENFS_SECLABEL_SYMLINKS, POLICYDB_CAP_IOCTL_SKIP_CLOEXEC, + POLICYDB_CAP_SCTP_ASCONF, __POLICYDB_CAP_MAX }; #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1) diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index 2a87fc3702b8..e6d8fbddbbdc 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -13,7 +13,8 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = { "cgroup_seclabel", "nnp_nosuid_transition", "genfs_seclabel_symlinks", - "ioctl_skip_cloexec" + "ioctl_skip_cloexec", + "sctp_asconf", }; #endif /* _SELINUX_POLICYCAP_NAMES_H_ */ diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 393aff41d3ef..98d3e6c0a146 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -230,6 +230,13 @@ static inline bool selinux_policycap_ioctl_skip_cloexec(void) return READ_ONCE(state->policycap[POLICYDB_CAP_IOCTL_SKIP_CLOEXEC]); } +static inline bool selinux_policycap_sctp_asconf(void) +{ + struct selinux_state *state = &selinux_state; + + return READ_ONCE(state->policycap[POLICYDB_CAP_SCTP_ASCONF]); +} + struct selinux_policy_convert_data; struct selinux_load_state { diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 1321f15799e2..28d0ead32416 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family) */ void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk) { - struct sk_security_struct *sksec = sk->sk_security; struct sk_security_struct *newsksec = newsk->sk_security; - newsksec->nlbl_state = sksec->nlbl_state; + /* SCTP is multi-homed so we must label each packet based on dest IP */ + newsksec->nlbl_state = NLBL_REQSKB; } /** @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) secattr = selinux_netlbl_sock_genattr(sk); if (secattr == NULL) return -ENOMEM; + + /* SCTP has the ability to communicate with multiple endpoints for a + * given association so we need to force NLBL_REQSKB so that we always + * label traffic based on the destination endpoint and not the + * association's connection + */ + if (sk->sk_protocol == IPPROTO_SCTP) { + sksec->nlbl_state = NLBL_REQSKB; + return 0; + } + rc = netlbl_sock_setattr(sk, family, secattr); switch (rc) { case 0: @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk, struct sk_security_struct *sksec = sk->sk_security; struct netlbl_lsm_secattr *secattr; - /* connected sockets are allowed to disconnect when the address family - * is set to AF_UNSPEC, if that is what is happening we want to reset - * the socket */ - if (addr->sa_family == AF_UNSPEC) { + /* special handling for AF_UNSPEC and IPPROTO_SCTP: + * - sockets are allowed to disconnect when the address family + * is set to AF_UNSPEC, if that is what is happening we want to reset + * the socket + * - SCTP has the ability to communicate with multiple endpoints for + * a given association so we need to force NLBL_REQSKB so that we + * always label traffic based on the destination endpoint and not + * the association's connection, see similar comment in + * selinux_netlbl_socket_post_create() + */ + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) { netlbl_sock_delattr(sk); sksec->nlbl_state = NLBL_REQSKB; rc = 0;
This patch makes two changes to how SELinux processes SCTP traffic: * Considering the multi-homed nature of SCTP, all SCTP traffic is marked as NLBL_REQSKB from a NetLabel perspective so that traffic is labeled on a per-packet basis using the destination IP, and not the on-the-wire label cached at the socket layer. * New permissions have been added to the "sctp_socket" object class: sctp_socket/{asconf_addip, asconf_connect}. These new permissions are gated by the "sctp_asconf" SELinux policy capability, and control the ability of ASCONF to add a new IP address to an association and set the primary IP of the association. The ASCONF access control points now work like the examples below; <socket> is the local socket's label, <port> is the label of the network port, and <peer> is the network peer label (dependent on the labeled networking configuration). - legacy policy (no sctp_asconf) allow <socket> <port>:sctp_socket { name_connect }; - updated policy without labeled networking (enabled sctp_asconf) allow <socket> <port>:sctp_socket { asconf_connect }; - updated policy with labeled networking (enabled sctp_asconf) allow <peer> <socket>:sctp_socket { asconf_addip }; allow <socket> <port>:sctp_socket { asconf_connect }; Cc: stable@vger.kernel.org Fixes: d452930fd3b9 ("selinux: Add SCTP support") Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/hooks.c | 86 +++++++++++++++++++--------- security/selinux/include/classmap.h | 3 + security/selinux/include/netlabel.h | 1 security/selinux/include/policycap.h | 1 security/selinux/include/policycap_names.h | 3 + security/selinux/include/security.h | 7 ++ security/selinux/netlabel.c | 30 ++++++++-- 7 files changed, 95 insertions(+), 36 deletions(-)