On Thu, Nov 4, 2021 at 3:59 PM Ondrej Mosnacek [off-list ref] wrote:

The commit referenced in the "Fixes" tag mistakenly attempted to
preserve the label of the peeloff socket that had been set in
selinux_socket_post_create() in the case of a client socket. However,
the peeloff socket should in fact just inherit the label from the parent
socket. In practice these labels are usually the same, but they may
differ when setsockcreatecon(3) or socket type transition rules are
involved.

The fact that selinux_socket_[post_]create() are called on the peeloff
socket is actually not what should be happening (it is a side effect of
sctp_do_peeloff() using socket_create() to create the socket, which
calls the aforementioned LSM hooks). A patch to fix this is being worked
on.

In the meanwhile, at least fix sctp_assoc_established() to set
asoc->secid to the socket's sid and selinux_sctp_sk_clone() to
unconditionally get the peeloff socket's sid from asoc->secid, which
will ensure that the peeloff socket gets the right label in case of both
client and server SCTP socket. The label set by
selinux_socket_post_create() will be simply overwritten in both cases,
as was the case before the commit this patch is fixing.

Passed both the selinux-testsuite (with client peeloff tests added) and
the SCTP functional test suite from lksctp-tools.

Fixes: e7310c94024c ("security: implement sctp_assoc_established hook in selinux")
Based-on-patch-by: Xin Long [off-list ref]
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

As agreed with Xin Long, I'm posting this fix up instead of him. I am
now fairly convinced that this is the right way to deal with the
immediate problem of client peeloff socket labeling. I'll work on
addressing the side problem regarding selinux_socket_post_create()
being called on the peeloff sockets separately.

Please don't merge this patch without an ack from Paul, as it seems
we haven't reached an overall consensus yet.

 security/selinux/hooks.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

When we change things as significantly as we are doing here, i.e.
shifting some of the labeling away from the endpoint to the
association, I much rather we do it as a chunk/patchset so that we can
review it in a consistent manner.  Some of that has gone out the door
here because of what I view as recklessness on the part of the netdev
folks, but that doesn't mean we need to abandon all order.  Let's get
all the fixes and repairs queued up in a single patchset so that we
can fully see what the end result of these changes are going to look
like.  Further, I think it would be good if at least one of the
patches has a very clear explanation in the commit description (not
the cover letter, I want to see this in the git log) of what happens
with respect to labeling on the server side, the client side, during
socket peeloffs on both ends, and how multiple associations are
handled.  My hope is that this should give us all a more consistent
view, which will be very important moving forward if netdev is going
to act independent of the other subsystems.

-- 
paul moore
www.paul-moore.com