On Wed, Oct 27, 2021 at 4:30 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > >> > > > Different from selinux_inet_conn_established(), it also gives the > > > >> > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > >> > > > as one UDP-type socket may have more than one asocs. > > > >> > > > > > > >> > > > Note that peer_secid in asoc will save the peer secid for this > > > >> > > > asoc connection, and peer_sid in sksec will just keep the peer > > > >> > > > secid for the latest connection. So the right use should be do > > > >> > > > peeloff for UDP-type socket if there will be multiple asocs in > > > >> > > > one socket, so that the peeloff socket has the right label for > > > >> > > > its asoc. > > > >> > > > > > >> > > Hm... this sounds like something we should also try to fix (if > > > >> > > possible). In access control we can't trust userspace to do the right > > > >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > > > >> > > shouldn't cause checking against the wrong peer_sid. But that can be > > > >> > > addressed separately. (And maybe it's even already accounted for > > > >> > > somehow - I didn't yet look at the code closely.) > > > > > > There are a couple of things we need to worry about here: the > > > per-packet access controls (e.g. can this packet be received by this > > > socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and > > > IP_CMSG_PASSSEC). > > > > > > The per-packet access controls work by checking the individual > > > packet's security label against the corresponding sock label on the > > > system (sk->sk_security->sid). Because of this it is important that > > > the sock's label is correct. For unconnected sockets this is fairly > > > straightforward as it follows the usual inherit-from-parent[1] > > > behavior we see in other areas of SELinux. For connected stream > > > sockets this can be a bit more complicated. However, since we are > > > only discussing the client side things aren't too bad with the > > > behavior essentially the same, inherit-from-parent, with the only > > > interesting piece worth noting being the sksec->peer_sid > > > (sk->sk_security->peer_sid) that we record from the packet passed to > > > the LSM/SELinux hook (using selinux_skb_peerlbl_sid()). The > > > sksec->peer_sid is recorded primarily so that the kernel can correctly > > > respond to SO_GETPEERSEC requests from userspace; it shouldn't be used > > > in any access control decisions. > > > > Hi, Paul > > > > Understand now, the issue reported seems caused by when > > doing peel-off the peel-off socket gets the uninitialised sid > > from 'ep' on the client, though it should be "asoc". > > Hi Xin Long, > > Yes, that is my understanding. I got the impression from the thread > that there was some confusion about the different labels and what they > were used for in SELinux, I was trying to provide some background in > the text above. If you are already familiar with how things should > work you can disregard it :) > > > > In the case of SCTP, I would expect things to behave similarly: the > > > sksec->peer_sid should match the packet label of the traffic which > > > acknowledged/accepted the new connection, e.g. the other end of the > > > connected socket. You will have to forgive me some of the details, > > > it's been a while since I last looked at the SCTP bits, but I would > > > expect that if a client created a new connection and/or spun-off a new > > > socket the new socket's sksec->peer_sid would have the same property, > > > it would represent the security label of the other end of the > > > connection/association. > > > > In SCTP, a socket doesn't represent a peer connection, it's more an > > object binding some addresses and receiving incoming connecting > > request, then creates 'asoc' to represent the connection, so asoc-> > > peer_secid represents the security label of the other end of the > > connection/association. > > As mentioned previously the asoc->peer_secid *should* be the security > label of the remote end, so I think we are okay here. My concern > remains the asoc->secid label as I don't believe it is being set to > the correct value (more on that below). > > > After doing peel-off, it makes one asoc 'bind' to one new socket, > > and this socket is used for userspace to control this asoc (conection), > > so naturally we set sksec->peer_sid to asoc->secid for access control > > in socket. > > The sksec->peer_sid represents the security label of the remote end so > it should be set to the asoc->peer_secid and *not* the asoc->secid Right, sorry, it was a copy-paste error, it should've been "asoc->peer_secid". > value. Yes, they are presently the same value in your patches, but I > believe that is a mistake; I believe the asoc->secid value should be > set to that of the parent (see the prior inherit-from-parent > discussion) which in this case would likely be either the parent > association or the client process, I'm not entirely clear on which is Yes, I think that's what the current patch does in selinux_sctp_assoc_established(). > correct in the SCTP case. The initial SCTP client association would > need to take it's label from the parent process so perhaps that is the > right answer for all SCTP client associations[2]. > > [1] I would expect server side associations to follow the more > complicated selinux_conn_sid() labeling, just as we do for TCP/stream > connections today. Yes, selinux_conn_sid() is currently called in selinux_sctp_assoc_request() for the server side. > > [2] I'm guessing the client associations might also want to follow the > setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more > info. OK, I think we are on the same page now, I will post v2. Thanks! > > -- > paul moore > www.paul-moore.com
