Message-ID: <CADvbK_cmo5Nbrgdt_qLZVnee4M_e4vrw+ocHG5BZpPH_=SS=bQ@mail.gmail.com>
Date:   Thu, 4 Nov 2021 06:56:17 -0400
From:   Xin Long <lucien.xin@...il.com>
To:     Paul Moore <paul@...l-moore.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 11:17 PM Paul Moore <paul@...l-moore.com> wrote:
>
> 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 initial SCTP client association would
need to take it's label from the parent process so perhaps that is the
right answer for all SCTP client associations[2].

[2] I'm guessing the client associations might also want to follow the
setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more
info.
"""
What I got is to take it's label from the parent process, which means
we get it from socket_sockcreate_sid(), not directly copy from parent
socket. It seems I misunderstood that, Sorry, maybe we should just
use the v1 patchset.


> (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.
Right, that's what it currently does in this patchset, no *socket* sid
is changed, and only *socket*'s peer label.

{
        struct sk_security_struct *sksec = asoc->base.sk->sk_security;

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

>
> > 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);
>   }
here, SCTP is one-to-many socket, and it means one socket can have
multiple associations or connections, so for sksec->sid in one socket
it can only save the latest cid, if we peel off an old one, it will get the
wrong cid on server side.

>
>   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