On Thu, Jul 21, 2022 at 4:14 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> 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 believe the latter is closer to what I was thinking: keep the
existing tests, add additional ones to test what you want. The SCTP
test already haa a bunch of individual tests (over a hundred I
believe), a few more shouldn't be a big problem. If we need to add
functionality to the SCTP test server and client programs we can do
that too.
> > > 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.
This is one of the awkward parts of SCTP and mapping its behavior to
the existing socket approach. I believe we discussed some of the
difficulties around locking an association to the first
peer/connection when the patches went in and we couldn't think of a
better way at the time. If you, or anyone else, has a different
approach that you believe would work better, let's discuss it; just
keep in mind we can't affect/relabel existing things, that's a big
no-no :)
> To me
> it feels like a sort of desperate attempt to provide at least some
> peer context, even if it might not be meaningful.
I would argue that it is still meaningful. I would also mention that
the association labeling isn't just the peer label, but the
association's label itself.
> 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.
Unfortunately we can't force a 1-to-1 socket/association as SCTP
supports both 1-to-1 and 1-to-many. I agree it would be nice, but
that decision has already been made for us.
The getpeercon() API returns the label of the remote
peer/node/socket/association based on the initial network traffic.
This should remain unchanged. If an SELinux aware SCTP application
that leverages the sequential/datagram behavior of SCTP instead of the
stream behavior, wants to know the label on an individual datagram it
should use IP_PASSSEC/SCM_SECURITY just as a SELinux aware UDP
application would.
> 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.)
The difference being that with SCTP an association exists whereas
there is no similar state for UDP.
--
paul-moore.com
