On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> 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?
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.
--
paul-moore.com
