On Thu, Jul 21, 2022 at 12:17 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Wed, Jul 20, 2022 at 7:14 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > On Tue, Jul 19, 2022 at 4:28 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Jul 19, 2022 at 7:58 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > > On Tue, Jul 19, 2022 at 12:31 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > > Rework the SCTP tests slightly to remove two assumptions which are
> > > > > not always guaranteed to be true (below). This should have not any
> > > > > affect on the current test suite or released kernels, but it will
> > > > > help ensure that the test suite continues to work with upcoming
> > > > > kernel releases.
> > > > >
> > > > > * Do not rely on IP options attached to a socket. Depending on the
> > > > > kernel configuration, the on-the-wire packet labels may be
> > > > > generated on a per-packet basis as opposed to a per-socket basis.
> > > >
> > > > Could you expand a bit on why this would be a problem? It's not clear
> > > > to me how switching to per-packet would break the tests. (Maybe I'm
> > > > just not thinking about it hard enough, but ideally the commit message
> > > > would explain the problem to me so I don't have to :)
> > >
> > > NetLabel can either attach on-the-wire packet labels (IP options)
> > > directly to the packet or to the socket, in the latter case the
> > > network stack handles writing the on-the-wire labels to the packet
> > > when it is generated. Deciding on when to attach IP options
> > > (on-the-wire labels) to the socket versus the packet is an
> > > implementation detail and depends on the specific configuration of the
> > > system and the protocols involved. It is my opinion that going into
> > > the level of detail necessary to explain the differences would involve
> > > a discussion about how the Linux network stacks works, the design of
> > > the NetLabel subsystem, and how the different network protocols work.
> > > The important takeaway is that one can not safely rely on IP options
> > > attached to a socket as a means of determining the labeling behavior
> > > of a socket/connection/association/etc., this is why we have APIs such
> > > as getpeercon() and the LSM specific socket options.
> >
> > Oh wait... I looked closer at what the test is actually doing with the
> > -i options and I get it now.
>
> The labeled networking stuff can be a little confusing at times, I'm
> glad you were able to wrap your head around it.
>
> > However, I'm pondering whether in the seq server case we shouldn't
> > change the approach a bit... Currently (after your patch) we are
> > basically testing the "unified" socket-level peercon and that forces
> > us to restart the server.
>
> /me nods
>
> > But we could also ignore the socket-level
> > peercon in case of SOCK_SEQPACKET and instead extract the per-packet
> > peercon via IP_PASSSEC and SCM_SECURITY control messages, like we do
> > in tests/inet_socket/server.c in the SOCK_DGRAM case.
>
> Yes, we could do that. I do think there is some value in checking the
> peer label when sending SEQPACKETs and the two labels differ, but we
> can do both, the tests are not mutually exclusive :)
True, but then we need to preserve the server restarting to work
around the "pinning" of the socket-level peercon, which is a bit
awkward... Or we could separate the expected values for the two
peercons and test that the socket-level one is always the one from the
first connection, while the packet-level ones correspond to the actual
peer.
> > I think this
> > should be the main way for users to get peercon when using
> > SOCK_SEQPACKET SCTP sockets with multiple peers and we should test it
> > with higher priority than the socket-level peercon.
>
> I'm not entirely sure what you are suggesting, but just to be clear,
> the peer label should always be the label of the remote ("peer"). If
> the remote end of the connection is running with s0:c0.c10, it doesn't
> matter if the local end initiates communication at s0:c5, the peer
> label from the local's perspective should be s0:c0.c10 as that is
> label of the remote/server.
>
> It gets a little confusing when you start thinking about how
> setsockcreatecon() affects this, and SCTP adds its own twists with the
> different associations, but the core idea should remain the same.
The problem with the socket-level peercon on a SOCK_SEQPACKET socket
is that due to the multi-peer nature of it we resort to setting it
based on the first peer and then just revalidate any new differing
peer contexts through the SCTP_SOCKET__ASSOCIATION permission. To me
it feels like a sort of desperate attempt to provide at least some
peer context, even if it might not be meaningful. It can be convenient
if you know you are going have just one peer context per socket (and
the policy enforces it), but other than that it's better to ignore the
socket peercon and just look at the packet peercons (which are always
accurate) or peel off each new association into a 1-to-1 socket, where
the socket's peer context is accurate.
I'm of the opinion that it would be better to not return any peercon
at all for sockets that might end up receiving from multiple peers,
like we do with SOCK_DGRAM/UDP sockets. (Now it's probably too late to
change it, but that's what I would propose if the SCTP support was
being introduced now.)
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
