On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> ---
> 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.
