Re: [PATCH net] selinux: fix SCTP client peeloff socket labeling

[Paul Moore]

On Thu, Nov 4, 2021 at 3:59 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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 <lucien.xin@xxxxxxxxx>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
>
> 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