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. > 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. * 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). > 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. -- paul-moore.com
