Message-ID: <CAHC9VhSjPVotYVb8-ABescHmnNnDL=9B3M0J=txiDOuyJNoYuw@mail.gmail.com>
Date:   Wed, 3 Nov 2021 23:17:00 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     Ondrej Mosnacek <omosnace@...hat.com>,
        network dev <netdev@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        "linux-sctp @ vger . kernel . org" <linux-sctp@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        James Morris <jmorris@...ei.org>,
        Richard Haines <richard_c_haines@...nternet.com>
Subject: Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook
 in selinux

On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@...il.com> wrote:
> On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@...il.com> wrote:
> > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@...il.com> wrote:
> > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@...il.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.
> > > > > >
> > > > > > v1->v2:
> > > > > >   - call selinux_inet_conn_established() to reduce some code
> > > > > >     duplication in selinux_sctp_assoc_established(), as Ondrej
> > > > > >     suggested.
> > > > > >   - when doing peeloff, it calls sock_create() where it actually
> > > > > >     gets secid for socket from socket_sockcreate_sid(). So reuse
> > > > > >     SECSID_WILD to ensure the peeloff socket keeps using that
> > > > > >     secid after calling selinux_sctp_sk_clone() for client side.
> > > > >
> > > > > Interesting... I find strange that SCTP creates the peeloff socket
> > > > > using sock_create() rather than allocating it directly via
> > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls
> > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the
> > > > > sock_create() call and just rely on the security_sctp_sk_clone()
> > > > > semantic to set up the labels? Would anything break if
> > > > > sctp_do_peeloff() switched to plain sock_alloc()?
> > > > >
> > > > > I'd rather we avoid this SECSID_WILD hack to support the weird
> > > > > created-but-also-cloned socket hybrid and just make the peeloff socket
> > > > > behave the same as an accept()-ed socket (i.e. no
> > > > > security_socket_[post_]create() hook calls, just
> > > > > security_sctp_sk_clone()).
> >
> > I believe the important part is that sctp_do_peeloff() eventually
> > calls security_sctp_sk_clone() via way of sctp_copy_sock().  Assuming
> > we have security_sctp_sk_clone() working properly I would expect that
> > the new socket would be setup properly when sctp_do_peeloff() returns
> > on success.
> >
> > ... and yes, that SECSID_WILD approach is *not* something we want to do.
>
> SECSID_WILD is used to avoid client's new socket's sid overwritten by
> old socket's.

In the case of security_sctp_sk_clone() the new client socket (the
cloned socket) should inherit the label/sid from the original socket
(the "parent" in the inherit-from-parent label inheritance behavior
discussed earlier).  The selinux_sctp_assoc_established() function
should not change the socket's label/sid at all, only the peer label.

> If I understand correctly, new socket's should keep using its original
> sid, namely,
> the one set from security_socket_[post_]create() on client side. I
> AGREE with that.
> Now I want to *confirm* this with you, as it's different from the last version's
> 'inherit from parent socket' that Richard and Ondrej reviewed.

Unfortunately I think we are struggling to communicate because you are
not familiar with SELinux concepts and I'm not as well versed in SCTP
as you are.  As things currently stand, I am getting a disconnect
between your explanations and the code you have submitted; they simply
aren't consistent from my perspective.

In an effort to help provide something that is hopefully a bit more
clear, here are the selinux_sctp_sk_clone() and
selinux_sctp_assoc_established() functions which I believe we need.
If you feel these are incorrect, please explain and/or provide edits:

  static void selinux_sctp_sk_clone(struct sctp_association *asoc,
                                    struct sock *sk, struct sock *newsk)
  {
    struct sk_security_struct *sksec = sk->sk_security;
    struct sk_security_struct *newsksec = newsk->sk_security;

    /* If policy does not support SECCLASS_SCTP_SOCKET then call
     * the non-sctp clone version.
     */
    if (!selinux_policycap_extsockclass())
      return selinux_sk_clone_security(sk, newsk);

    newsksec->secid = sksec->secid;
    newsksec->peer_sid = asoc->peer_secid;
    newsksec->sclass = sksec->sclass;
    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;

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

> > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > > > Reported-by: Prashanth Prahlad <pprahlad@...hat.com>
> > > > > > Reviewed-by: Richard Haines <richard_c_haines@...nternet.com>
> > > > > > Tested-by: Richard Haines <richard_c_haines@...nternet.com>
> > > > >
> > > > > You made non-trivial changes since the last revision in this patch, so
> > > > > you should have also dropped the Reviewed-by and Tested-by here. Now
> > > > > David has merged the patches probably under the impression that they
> > > > > have been reviewed/approved from the SELinux side, which isn't
> > > > > completely true.
> > > >
> > > > Oh, that's a mistake, I thought I didn't add it.
> > > > Will he be able to test this new patchset?
> >
> > While I tend to try to avoid reverts as much as possible, I think the
> > right thing to do is to get these patches reverted out of DaveM's tree
> > while we continue to sort this out and do all of the necessary testing
> > and verification.
> >
> > Xin Long, please work with the netdev folks to get your patchset
> > reverted and then respin this patchset using the feedback provided.
>
> Hi, Paul,
>
> The original issue this patchset fixes is a crucial one (it could cause
> peeloff sockets on client side to not work) which I think
> can already be fixed now. If you think SECSID_WILD is tricky but
> no better way yet, my suggestion is to leave it for now until we have
> a better solution to follow up. As I couldn't find a better way to work
> it out. Also, we may want to hear Richard's opinion on how it should
> work and how this should be fixed.

While I understand you did not intend to mislead DaveM and the netdev
folks with the v2 patchset, your failure to properly manage the
patchset's metadata *did* mislead them and as a result a patchset with
serious concerns from the SELinux side was merged.  You need to revert
this patchset while we continue to discuss, develop, and verify a
proper fix that we can all agree on.  If you decide not to revert this
patchset I will work with DaveM to do it for you, and that is not
something any of us wants.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists