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

[Xin Long <lucien.xin@gmail.com>]

On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > On Tue, Nov 2, 2021 at 1:03 PM 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.
> > > > >
> > > > > 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.

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.

>
> In my mind, selinux_sctp_sk_clone() should end up looking like this.
>
>   void selinux_sctp_sk_clone(asoc, sk, newsk)
>   {
>     struct sk_security_struct sksec = sk->sk_security;
>     struct sk_security_struct newsksec = newsk->sk_security;
>
>     if (!selinux_policycap_extsockclass())
>         return selinux_sk_clone_security(sk, newsk);
>
>     newsksec->sid = sksec->secid;
>     newsksec->peer_sid = asoc->peer_secid;
>     newsksec->sclass = sksec->sclass;
>     selinux_netlbl_sctp_sk_clone(sk, newsk);
>   }
Let's say, this socket has 3 associations now, how can we ensure
the new socket's sid is set to the right sid? I don't think we can use
"sksec->secid" in this place, this is not TCP.

>
> Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be
> removed from selinux_sctp_assoc_established().  If we are treating
> SCTP associations similarly to TCP connections, the association's
> label/secid should be set once and not changed during the life of the
> association.
The association's label/secid will never change once set in this patchset.
it's just a temporary record, and later it will be used to set socket's
label/secid. I think that's the idea at the beginning.

>
> > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.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.

Thanks.

>
> --
> paul moore
> www.paul-moore.com