On Tue, Feb 15, 2022 at 9:03 PM Paul Moore [off-list ref] wrote:

On Mon, Feb 14, 2022 at 11:13 PM Xin Long [off-list ref] wrote:

quoted

Looks okay to me.

The difference from the old one is that: with
selinux_sctp_process_new_assoc() called in
selinux_sctp_assoc_established(), the client sksec->peer_sid is using
the first asoc's peer_secid, instead of the latest asoc's peer_secid.
And not sure if it will cause any problems when doing the extra check
sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
err*. But I don't know about selinux, I guess there must be a reason
from selinux side.

Generally speaking we don't want to change any SELinux socket labels
once it has been created.  While the peer_sid is a bit different,
changing it after userspace has access to the socket could be
problematic.  In the case where the peer_sid differs between the two
we have a permission check which allows policy to control this
behavior which seems like the best option at this point.

I think that maybe Xin was referring to the fact that on error return
from the hook the return code information is lost and the assoc is
just silently dropped (but I may have misunderstood). In case of a
denial (avc_has_perm() returning -EACCESS) this isn't much of a
problem, because the denial is logged in the audit log, so there is a
way to figure out why opening the association failed. In case of other
errors we could indeed do better and either log an SELINUX_ERR audit
event or at least pr_err() into the console, but there are likely
several other existing cases like this, so it would be best to do this
cleanup independently in another patch (if anyone feels up to the
task...).

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.