Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux

[Paul Moore <paul@paul-moore.com>]

On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote:
> On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote:
>> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> wrote:
>> > > > Different from selinux_inet_conn_established(), it also gives the
>> > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(),
>> > > > as one UDP-type socket may have more than one asocs.
>> > > >
>> > > > Note that peer_secid in asoc will save the peer secid for this
>> > > > asoc connection, and peer_sid in sksec will just keep the peer
>> > > > secid for the latest connection. So the right use should be do
>> > > > peeloff for UDP-type socket if there will be multiple asocs in
>> > > > one socket, so that the peeloff socket has the right label for
>> > > > its asoc.
>> > >
>> > > Hm... this sounds like something we should also try to fix (if
>> > > possible). In access control we can't trust userspace to do the right
>> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket
>> > > shouldn't cause checking against the wrong peer_sid. But that can be
>> > > addressed separately. (And maybe it's even already accounted for
>> > > somehow - I didn't yet look at the code closely.)

There are a couple of things we need to worry about here: the
per-packet access controls (e.g. can this packet be received by this
socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and
IP_CMSG_PASSSEC).

The per-packet access controls work by checking the individual
packet's security label against the corresponding sock label on the
system (sk->sk_security->sid).  Because of this it is important that
the sock's label is correct.  For unconnected sockets this is fairly
straightforward as it follows the usual inherit-from-parent[1]
behavior we see in other areas of SELinux.  For connected stream
sockets this can be a bit more complicated.  However, since we are
only discussing the client side things aren't too bad with the
behavior essentially the same, inherit-from-parent, with the only
interesting piece worth noting being the sksec->peer_sid
(sk->sk_security->peer_sid) that we record from the packet passed to
the LSM/SELinux hook (using selinux_skb_peerlbl_sid()).  The
sksec->peer_sid is recorded primarily so that the kernel can correctly
respond to SO_GETPEERSEC requests from userspace; it shouldn't be used
in any access control decisions.

In the case of SCTP, I would expect things to behave similarly: the
sksec->peer_sid should match the packet label of the traffic which
acknowledged/accepted the new connection, e.g. the other end of the
connected socket.  You will have to forgive me some of the details,
it's been a while since I last looked at the SCTP bits, but I would
expect that if a client created a new connection and/or spun-off a new
socket the new socket's sksec->peer_sid would have the same property,
it would represent the security label of the other end of the
connection/association.

[1] Yes, there is setsockcreatecon(), but that isn't important for
this discussion.

>> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
>> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
>> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> > > > ---
>> > > >  security/selinux/hooks.c | 16 ++++++++++++++++
>> > > >  1 file changed, 16 insertions(+)
>> > > >
>> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > > > index f025fc00421b..793fdcbc68bd 100644
>> > > > --- a/security/selinux/hooks.c
>> > > > +++ b/security/selinux/hooks.c
>> > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
>> > > >         selinux_netlbl_sctp_sk_clone(sk, newsk);
>> > > >  }
>> > > >
>> > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc,
>> > > > +                                          struct sk_buff *skb)
>> > > > +{
>> > > > +       struct sk_security_struct *sksec = asoc->base.sk->sk_security;
>> > > > +       u16 family = asoc->base.sk->sk_family;
>> > > > +
>> > > > +       /* handle mapped IPv4 packets arriving via IPv6 sockets */
>> > > > +       if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
>> > > > +               family = PF_INET;
>> > > > +
>> > > > +       selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
>> > >
>> > > You could replace the above with
>> > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code
>> > > duplication.
>> > Hi Ondrej,
>> >
>> > will do, thanks!
>> >
>> > >
>> > > > +       asoc->secid = sksec->sid;
>> > > > +       asoc->peer_secid = sksec->peer_sid;
>> > > > +}
>> > > > +
>> > Now I'm thinking: 'peer_sid' should be correct here.
>> >
>> > BUT 'sid' is copied from its parent socket. Later when doing peel-off,
>> > asoc->secid will be set back to the peel-off socket's sksec->sid.
>>
>> Hi,
>>
>> I'm not sure I follow... When doing peel-off, security_sctp_sk_clone()
>> should be called, which sets the peel-off socket's sksec->sid to
>> asoc->secid, not the other way around. (Are we hitting the language
>> barrier here? :)
>
> Right, sorry.
>
> Set the peel-off socket's sksec->sid to asoc->secid, I meant :D

For the sake of clarity, let's scribble down some pseudo code to
discuss :)  Taking into account the feedback above, I arrived at the
code below (corrections are welcome if I misunderstood what you wanted
to convey) with my comments after:

  static void selinux_sctp_assoc_established(asoc, skb)
  {
    struct sock *sk = asoc->base.sk;
    struct sk_security_struct *sksec = sk->sk_security;

    selinux_inet_conn_established(sk, skb);
    asoc->secid = sksec->peer_sid;
    asoc->peer_secid = sksec->peer_sid;
  }

My only concern with the above code is the 'asoc->secid =
sksec->peer_sid' assignment.  As this particular association is a
client side association I would expect it to follow the normal
inherit-from-parent behavior as described above and not take the label
of remote peer, however I could be misunderstanding some of the SCTP
specifics here.  My initial reaction is that we need to adjust the
LSM/SELinux hook as well as the call site in sctp_sf_do_5_1D_ce() to
pass both 'new_asoc' as well 'asoc' and set 'new_asoc->secid' to
'asoc->secid' to better mirror the existing stream/TCP behavior on the
client side.

Does that make sense?  If not, what am I missing :)

-- 
paul moore
www.paul-moore.com