On Thu, Jul 21, 2022 at 3:51 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > 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. Ok, I'm going to merge this patch with some minor edits (see [1]) if you're okay with them and then I'll look into further improvements. [1] https://github.com/WOnder93/selinux-testsuite/commit/0f7bb1696a15972a555d997377348b8e4ae56b38 > > > > 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 :) I don't think it makes sense to change it at this point, since the user can always simply choose to ignore the socket peercon when using the SOCK_SEQPACKET SCTP socket. My point really was that we obviously can't test everything and if we have to choose between testing a less meaningful interface and testing a more meaningful interface, we should choose the latter. But it seems that in this case it won't be too hard/costly to test both, so that point is no longer relevant. > > > 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. I disagree, but it doesn't matter as we can't remove it at this point anyway. > > 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. No, we can't force that, but we could have refused to provide a socket-level peer context on (potentially) 1-to-many sockets. I'm also saying that we (the kernel) made it hard to enforce via policy that the peer label returned corresponds to the actual peer. On the type level it's easy as you can just not define any SCTP_SOCKET__ASSOCIATION rules, but on MLS level you would have to (AFAICT) add some non-trivial constraints to prevent peers with non-matching levels to connect to the same socket (and thus basically pretend to have different level if the userspace relies on the socket's peer context). > 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. That's irrelevant as the association doesn't correspond 1-to-1 to the socket. If we really really wanted to provide a way to get the association's peer context without peeling it off, we should do it via some SCTP-specific getsockopt that would take assoc ID as an argument (same as SCTP provides access to assoc-level info/properties). But IMHO it wouldn't be worth the effort as peeling off the assoc or using IP_PASSSEC should be a viable alternative in most if not all cases. I still stand behind my opinion that providing no socket-or-association-level peercon interface for SOCK_SEQPACKET would have been better than providing a bad one. (But anyway, this discussion is merely academic as it can't be undone now due to backwards compatibility.) -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
