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. 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]?
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.
[2] "A sender MUST only send a set primary request to an address that
is already considered part of the association."
https://datatracker.ietf.org/doc/html/rfc5061#section-5.4
>
> I haven't looked too closely, but my initial gut feeling would be to
> move the sock_has_perm() call out of selinux_socket_connect_helper()
> and either modify the sock_has_perm() function to take the subject as
> a parameter, or open code it inside selinux_sctp_bind_connect() using
> the peer label. I suspect the additional parameter will be cleaner
> due to the common_audit_data requirement of avc_has_perm().
>
> --
> paul-moore.com
>
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
