(Sorry for the late reply, I was on vacation last week.)
On Thu, Aug 11, 2022 at 5:04 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 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.
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.
