| Message ID | 53026dedd66beeaf18a4570437c4e6c9e760bb90.1634884487.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: fixups for the security hooks in sctp | expand |
On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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.) > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > security/selinux/hooks.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index f025fc00421b..793fdcbc68bd 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > selinux_netlbl_sctp_sk_clone(sk, newsk); > } > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > + struct sk_buff *skb) > +{ > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > + u16 family = asoc->base.sk->sk_family; > + > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > + family = PF_INET; > + > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); You could replace the above with `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code duplication. > + asoc->secid = sksec->sid; > + asoc->peer_secid = sksec->peer_sid; > +} > + > static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, > struct request_sock *req) > { > @@ -7290,6 +7305,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), > LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), > LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), > + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), > LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), > LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), > LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), > -- > 2.27.0 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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.) > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > security/selinux/hooks.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index f025fc00421b..793fdcbc68bd 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > } > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > + struct sk_buff *skb) > > +{ > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > + u16 family = asoc->base.sk->sk_family; > > + > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > > + family = PF_INET; > > + > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > You could replace the above with > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > duplication. Hi Ondrej, will do, thanks! > > > + asoc->secid = sksec->sid; > > + asoc->peer_secid = sksec->peer_sid; > > +} > > + Now I'm thinking: 'peer_sid' should be correct here. BUT 'sid' is copied from its parent socket. Later when doing peel-off, asoc->secid will be set back to the peel-off socket's sksec->sid. Do you think this is okay? or should the peel-off socket have its own sksec->sid, which might be different from the parent socket's? (Note the socket's sid initially was set in selinux_socket_post_create()) > > static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, > > struct request_sock *req) > > { > > @@ -7290,6 +7305,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), > > LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), > > LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), > > + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), > > LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), > > LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), > > LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), > > -- > > 2.27.0 > > > > -- > Ondrej Mosnacek > Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc. >
On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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.) > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > security/selinux/hooks.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index f025fc00421b..793fdcbc68bd 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > > } > > > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > > + struct sk_buff *skb) > > > +{ > > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > > + u16 family = asoc->base.sk->sk_family; > > > + > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > > > + family = PF_INET; > > > + > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > > > You could replace the above with > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > > duplication. > Hi Ondrej, > > will do, thanks! > > > > > > + asoc->secid = sksec->sid; > > > + asoc->peer_secid = sksec->peer_sid; > > > +} > > > + > Now I'm thinking: 'peer_sid' should be correct here. > > BUT 'sid' is copied from its parent socket. Later when doing peel-off, > asoc->secid will be set back to the peel-off socket's sksec->sid. Hi, I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() should be called, which sets the peel-off socket's sksec->sid to asoc->secid, not the other way around. (Are we hitting the language barrier here? :) > Do you think this is okay? or should the peel-off socket have its own > sksec->sid, which might be different from the parent socket's? > (Note the socket's sid initially was set in selinux_socket_post_create()) I *think* in case of a client socket it is expected for the peeloff-style child socket to just inherit the same sksec->sid. But frankly I haven't been with SELinux and kernel development long enough to understand the intricacies of SELinux's network connection handling very well... Hopefully Paul/Richard/Stephen can give a more confident answer/review here. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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. 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. [1] Yes, there is setsockcreatecon(), but that isn't important for this discussion. >> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") >> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> >> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> >> > > > --- >> > > > security/selinux/hooks.c | 16 ++++++++++++++++ >> > > > 1 file changed, 16 insertions(+) >> > > > >> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > > > index f025fc00421b..793fdcbc68bd 100644 >> > > > --- a/security/selinux/hooks.c >> > > > +++ b/security/selinux/hooks.c >> > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk >> > > > selinux_netlbl_sctp_sk_clone(sk, newsk); >> > > > } >> > > > >> > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, >> > > > + struct sk_buff *skb) >> > > > +{ >> > > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; >> > > > + u16 family = asoc->base.sk->sk_family; >> > > > + >> > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ >> > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) >> > > > + family = PF_INET; >> > > > + >> > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); >> > > >> > > You could replace the above with >> > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code >> > > duplication. >> > Hi Ondrej, >> > >> > will do, thanks! >> > >> > > >> > > > + asoc->secid = sksec->sid; >> > > > + asoc->peer_secid = sksec->peer_sid; >> > > > +} >> > > > + >> > Now I'm thinking: 'peer_sid' should be correct here. >> > >> > BUT 'sid' is copied from its parent socket. Later when doing peel-off, >> > asoc->secid will be set back to the peel-off socket's sksec->sid. >> >> Hi, >> >> I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() >> should be called, which sets the peel-off socket's sksec->sid to >> asoc->secid, not the other way around. (Are we hitting the language >> barrier here? :) > > Right, sorry. > > Set the peel-off socket's sksec->sid to asoc->secid, I meant :D For the sake of clarity, let's scribble down some pseudo code to discuss :) Taking into account the feedback above, I arrived at the code below (corrections are welcome if I misunderstood what you wanted to convey) with my comments after: static void selinux_sctp_assoc_established(asoc, skb) { struct sock *sk = asoc->base.sk; struct sk_security_struct *sksec = sk->sk_security; selinux_inet_conn_established(sk, skb); asoc->secid = sksec->peer_sid; asoc->peer_secid = sksec->peer_sid; } My only concern with the above code is the 'asoc->secid = sksec->peer_sid' assignment. As this particular association is a client side association I would expect it to follow the normal inherit-from-parent behavior as described above and not take the label of remote peer, however I could be misunderstanding some of the SCTP specifics here. My initial reaction is that we need to adjust the LSM/SELinux hook as well as the call site in sctp_sf_do_5_1D_ce() to pass both 'new_asoc' as well 'asoc' and set 'new_asoc->secid' to 'asoc->secid' to better mirror the existing stream/TCP behavior on the client side. Does that make sense? If not, what am I missing :)
On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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". Thanks! > > 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. 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. As for the 'parent' sk/ep, we can't use it to record peer_scid, as when there are multiple connections, the later one will overwrite the older one. > > [1] Yes, there is setsockcreatecon(), but that isn't important for > this discussion. > > >> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > >> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > >> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > >> > > > --- > >> > > > security/selinux/hooks.c | 16 ++++++++++++++++ > >> > > > 1 file changed, 16 insertions(+) > >> > > > > >> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> > > > index f025fc00421b..793fdcbc68bd 100644 > >> > > > --- a/security/selinux/hooks.c > >> > > > +++ b/security/selinux/hooks.c > >> > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > >> > > > selinux_netlbl_sctp_sk_clone(sk, newsk); > >> > > > } > >> > > > > >> > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > >> > > > + struct sk_buff *skb) > >> > > > +{ > >> > > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > >> > > > + u16 family = asoc->base.sk->sk_family; > >> > > > + > >> > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > >> > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > >> > > > + family = PF_INET; > >> > > > + > >> > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > >> > > > >> > > You could replace the above with > >> > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > >> > > duplication. > >> > Hi Ondrej, > >> > > >> > will do, thanks! > >> > > >> > > > >> > > > + asoc->secid = sksec->sid; > >> > > > + asoc->peer_secid = sksec->peer_sid; > >> > > > +} > >> > > > + > >> > Now I'm thinking: 'peer_sid' should be correct here. > >> > > >> > BUT 'sid' is copied from its parent socket. Later when doing peel-off, > >> > asoc->secid will be set back to the peel-off socket's sksec->sid. > >> > >> Hi, > >> > >> I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() > >> should be called, which sets the peel-off socket's sksec->sid to > >> asoc->secid, not the other way around. (Are we hitting the language > >> barrier here? :) > > > > Right, sorry. > > > > Set the peel-off socket's sksec->sid to asoc->secid, I meant :D > > For the sake of clarity, let's scribble down some pseudo code to > discuss :) Taking into account the feedback above, I arrived at the > code below (corrections are welcome if I misunderstood what you wanted > to convey) with my comments after: > > static void selinux_sctp_assoc_established(asoc, skb) > { > struct sock *sk = asoc->base.sk; > struct sk_security_struct *sksec = sk->sk_security; > > selinux_inet_conn_established(sk, skb); > asoc->secid = sksec->peer_sid; > asoc->peer_secid = sksec->peer_sid; > } > > My only concern with the above code is the 'asoc->secid = > sksec->peer_sid' assignment. As this particular association is a > client side association I would expect it to follow the normal > inherit-from-parent behavior as described above and not take the label > of remote peer, however I could be misunderstanding some of the SCTP selinux_sctp_assoc_established() is the first place to set sksec->peer_sid for this connection on client, so sksec->peer_sid is the 'parent' socket's peer_sid to this asoc. > specifics here. My initial reaction is that we need to adjust the > LSM/SELinux hook as well as the call site in sctp_sf_do_5_1D_ce() to > pass both 'new_asoc' as well 'asoc' and set 'new_asoc->secid' to > 'asoc->secid' to better mirror the existing stream/TCP behavior on the > client side. Sorry, there's something about SCTP that I should've made clear: sctp_sf_do_5_1D_ce() is normally called when no existing asoc (in most cases) or the existing asoc's state is CLOSE. #define TYPE_SCTP_COOKIE_ECHO { \ /* SCTP_STATE_CLOSED */ \ <---- (also include asoc is NULL) TYPE_SCTP_FUNC(sctp_sf_do_5_1D_ce), \ and for the existing asoc case, SCTP doesn't use anything from it for the new asoc, but just ignores it. IF these are conflicts when inserting transports for the new asoc with the existing asoc, sctp_process_init() will handle it and return errors. Just note that the existing asoc is NOT the one created when processing INIT chunk, and that INIT one is a temporary asoc and is deleted right after finishing processing INIT. But if you mean the hooks in sctp_sf_do_5_2_4_dupcook() where it's processing a duplicate COOKIE_ECHO and copying things from the existing asoc, maybe it should do such things, we can make another thread for this corner case. > > Does that make sense? If not, what am I missing :) > > -- > paul moore > www.paul-moore.com
On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin@gmail.com> wrote: > On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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 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 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. [2] I'm guessing the client associations might also want to follow the setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more info.
On Wed, Oct 27, 2021 at 4:30 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> 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
On Wed, Oct 27, 2021 at 12:00 AM Xin Long <lucien.xin@gmail.com> wrote:
> OK, I think we are on the same page now, I will post v2.
I'm not quite as confident we are on the same page just yet, but I
agree that having a new revision is a good idea; if nothing else it
will help reset the discussion to focus on updated patches - thanks!
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f025fc00421b..793fdcbc68bd 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk selinux_netlbl_sctp_sk_clone(sk, newsk); } +static void selinux_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ + struct sk_security_struct *sksec = asoc->base.sk->sk_security; + u16 family = asoc->base.sk->sk_family; + + /* handle mapped IPv4 packets arriving via IPv6 sockets */ + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) + family = PF_INET; + + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); + asoc->secid = sksec->sid; + asoc->peer_secid = sksec->peer_sid; +} + static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, struct request_sock *req) { @@ -7290,6 +7305,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established),
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. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- security/selinux/hooks.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)