On Fri, Jun 3, 2022 at 12:45 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > On Thu, Jun 2, 2022 at 10:49 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > On Wed, Jun 1, 2022 at 3:32 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > On Tue, May 31, 2022 at 7:53 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > On Tue, May 31, 2022 at 1:05 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > > Hello everyone, > > > > > > > > > > Investigating the yet still spuriously failing SCTP ASCONF test [1] > > > > > > > > FWIW, I haven't seen failures with the SCTP tests when doing my > > > > testing, but perhaps I've just been lucky with the timing windows. > > > > > > > > > has led me to realize that the SCTP_PARAM_* chunk handling is in fact > > > > > severely flawed. The SCTP_PARAM_* code paths reuse the > > > > > security_sctp_bind_connect() hook, but that hook uses the current > > > > > task's sid when checking the socket::connect permission, which is not > > > > > correct, since there is no guarantee on the task context in which the > > > > > incoming ASCONF packet will be processed. > > > > > > > > > > The relevant selinux-testsuite test [1] expects the subject sid to be > > > > > the one of the server, which has been true only by accident, as SCTP > > > > > often processes the incoming ASCONF chunk via softirq right after it > > > > > is sent. > > > > > > > > > > This seems tricky to fix, as we don't have any appropriate subject > > > > > context at hand at the time of receiving the ASCONF chunk... Any > > > > > ideas? > > > > > > From a quick skim of the SCTP RFCs, the ASCONF chunk is sent from a > > > remote endpoint to update the SCTP parameters so in this case I > > > believe the subject should be the remote peer (the association/sock's > > > peer_secid) and the object should be the local association/socket. > > > It's important to note that any access control checks using the remote > > > peer label should be gated by the selinux_peerlbl_enabled() function, > > > see selinux_socket_sock_rcv_skb() for an example. > > > > I don't like the idea of using peer_secid as the subject for > > socket::connect, which normally has a task sid as the subject. > > While I have concerns about reusing the socket:connect permission for > the ASCONF updates, it isn't because of the peer_secid. The > peer_secid represents the security label of the remote peer and it is > the subject label for ASCONF operations. It does make sense to use it as a subject label, but not for the connect permission, where the convention is that the subject context is a process context. While we don't have any hard rule against mixing different "kinds" of contexts in the subject/target of a given permission, it makes both figuring out AVC denials and reasoning about policy harder and I want to avoid it wherever possible. > > If we go this way, then I'd suggest extracting this case into a new > > permission. Maybe sctp_socket::asconf_from? Or even split it into > > ::add_ip_from and ::set_primary_from, as the latter is less security > > sensitive[2]? > > Two thoughts in random order: > > * Policy capabilities would be needed as we currently apply the > socket:connect permission to cover the ASCONF updates. Yes. > * We should list out all of the current ASCONF parameter changes and > have a discussion about which ones we want to protect with access > controls, and of those remaining controls which should be grouped (if > any). I agree that we should think this through before making changes. I likely won't have enough time to implement the fixes myself, but at least I will try to provide some food for thought for whoever will attempt to tackle this :) > > Also we might want to have another check over just the socket context > > to allow/disallow ADD_IP/SET_PRIMARY regardless of the peer so that > > there is some level of control also when peer labeling is disabled. > > Are you talking about something like this: "allow socket_t > self:sctp_socket some_asconf_perm"? I'd need to hear a good > explanation for this and why we would need this control, because it > doesn't make a lot of sense to me. If you are allowing an application > to create a SCTP socket, you are granting it permission to make use of > SCTP, including ASCONF. My idea is to provide policy writers/users (particularly those who don't want to enable peer labeling) a simple way to control the usage of ASCONF on the machine (either at global or socket level). ASCONF is quite a niche feature of SCTP and probably won't be used by most applications (and the SCTP RFC mandates that it be used only in conjunction with auth chunks, which is itself another niche feature). Given that ASCONF has security implications, I see it as a natural requirement to be able to allow a domain to use SCTP itself, but not to process ASCONF commands from any peer. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
