| Message ID | 20220212175922.665442-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: fixups for the security hooks in sctp | expand |
On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > Do this by extracting the peer labeling per-association logic from > selinux_sctp_assoc_request() into a new helper > selinux_sctp_process_new_assoc() and use this helper in both > selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This > ensures that the peer labeling behavior as documented in > Documentation/security/SCTP.rst is applied both on the client and server > side: > """ > An SCTP socket will only have one peer label assigned to it. This will be > assigned during the establishment of the first association. Any further > associations on this socket will have their packet peer label compared to > the sockets peer label, and only if they are different will the > ``association`` permission be validated. This is validated by checking the > socket peer sid against the received packets peer sid to determine whether > the association should be allowed or denied. > """ > > At the same time, it also ensures that the peer label of the association > is set to the correct value, such that if it is peeled off into a new > socket, the socket's peer label will then be set to the association's > peer label, same as it already works on the server side. > > While selinux_inet_conn_established() (which we are replacing by > selinux_sctp_assoc_established() for SCTP) only deals with assigning a > peer label to the connection (socket), in case of SCTP we need to also > copy the (local) socket label to the association, so that > selinux_sctp_sk_clone() can then pick it up for the new socket in case > of SCTP peeloff. > > Careful readers will notice that the selinux_sctp_process_new_assoc() > helper also includes the "IPv4 packet received over an IPv6 socket" > check, even though it hadn't been in selinux_sctp_assoc_request() > before. While such check is not necessary in > selinux_inet_conn_request() (because struct request_sock's family field > is already set according to the skb's family), here it is needed, as we > don't have request_sock and we take the initial family from the socket. > In selinux_sctp_assoc_established() it is similarly needed as well (and > also selinux_inet_conn_established() already has it). > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Based-on-patch-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++----------- > 1 file changed, 66 insertions(+), 24 deletions(-) This patch, and patch 1/2, look good to me; I'm assuming this resolves all of the known SELinux/SCTP problems identified before the new year? If I can get an ACK from one of the SCTP and/or netdev folks I'll merge this into the selinux/next branch.
On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote: > If I can get an ACK from one of the SCTP and/or netdev folks I'll > merge this into the selinux/next branch. No objections here FWIW, I'd defer the official acking to the SCTP maintainers.
On Tue, Feb 15, 2022 at 11:58 AM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > > > Em seg., 14 de fev. de 2022 21:54, Jakub Kicinski <kuba@kernel.org> escreveu: >> >> On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote: >> > If I can get an ACK from one of the SCTP and/or netdev folks I'll >> > merge this into the selinux/next branch. >> >> No objections here FWIW, I'd defer the official acking to the SCTP >> maintainers. > > > None from my side either, but I really want to hear from Xin. He has worked on this since day 0. > Looks okay to me. The difference from the old one is that: with selinux_sctp_process_new_assoc() called in selinux_sctp_assoc_established(), the client sksec->peer_sid is using the first asoc's peer_secid, instead of the latest asoc's peer_secid. And not sure if it will cause any problems when doing the extra check sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns err*. But I don't know about selinux, I guess there must be a reason from selinux side. I will ACK on patch 0/2. Thanks Ondrej for working on this patiently.
On Mon, Feb 14, 2022 at 11:13 PM Xin Long <lucien.xin@gmail.com> wrote: > Looks okay to me. > > The difference from the old one is that: with > selinux_sctp_process_new_assoc() called in > selinux_sctp_assoc_established(), the client sksec->peer_sid is using > the first asoc's peer_secid, instead of the latest asoc's peer_secid. > And not sure if it will cause any problems when doing the extra check > sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns > err*. But I don't know about selinux, I guess there must be a reason > from selinux side. Generally speaking we don't want to change any SELinux socket labels once it has been created. While the peer_sid is a bit different, changing it after userspace has access to the socket could be problematic. In the case where the peer_sid differs between the two we have a permission check which allows policy to control this behavior which seems like the best option at this point. > I will ACK on patch 0/2. Thanks, I'm going to go ahead and merge these two patches into selinux/next right now.
On Mon, Feb 14, 2022 at 11:14 PM Paul Moore <paul@paul-moore.com> wrote: > On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > Do this by extracting the peer labeling per-association logic from > > selinux_sctp_assoc_request() into a new helper > > selinux_sctp_process_new_assoc() and use this helper in both > > selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This > > ensures that the peer labeling behavior as documented in > > Documentation/security/SCTP.rst is applied both on the client and server > > side: > > """ > > An SCTP socket will only have one peer label assigned to it. This will be > > assigned during the establishment of the first association. Any further > > associations on this socket will have their packet peer label compared to > > the sockets peer label, and only if they are different will the > > ``association`` permission be validated. This is validated by checking the > > socket peer sid against the received packets peer sid to determine whether > > the association should be allowed or denied. > > """ > > > > At the same time, it also ensures that the peer label of the association > > is set to the correct value, such that if it is peeled off into a new > > socket, the socket's peer label will then be set to the association's > > peer label, same as it already works on the server side. > > > > While selinux_inet_conn_established() (which we are replacing by > > selinux_sctp_assoc_established() for SCTP) only deals with assigning a > > peer label to the connection (socket), in case of SCTP we need to also > > copy the (local) socket label to the association, so that > > selinux_sctp_sk_clone() can then pick it up for the new socket in case > > of SCTP peeloff. > > > > Careful readers will notice that the selinux_sctp_process_new_assoc() > > helper also includes the "IPv4 packet received over an IPv6 socket" > > check, even though it hadn't been in selinux_sctp_assoc_request() > > before. While such check is not necessary in > > selinux_inet_conn_request() (because struct request_sock's family field > > is already set according to the skb's family), here it is needed, as we > > don't have request_sock and we take the initial family from the socket. > > In selinux_sctp_assoc_established() it is similarly needed as well (and > > also selinux_inet_conn_established() already has it). > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Based-on-patch-by: Xin Long <lucien.xin@gmail.com> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++----------- > > 1 file changed, 66 insertions(+), 24 deletions(-) > > This patch, and patch 1/2, look good to me; I'm assuming this resolves > all of the known SELinux/SCTP problems identified before the new year? No, not really. There is still the inconsistency that peeloff sockets go through the socket_[post_]create hooks and then the label computed is overwritten by the sctp_sk_clone hook. But it's a different issue unrelated to this one. I'm still in the process of cooking up the patches and figuring out the consequences (other LSMs would be affected by the change, too, so it is tricky...). -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Tue, Feb 15, 2022 at 9:03 PM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Feb 14, 2022 at 11:13 PM Xin Long <lucien.xin@gmail.com> wrote: > > Looks okay to me. > > > > The difference from the old one is that: with > > selinux_sctp_process_new_assoc() called in > > selinux_sctp_assoc_established(), the client sksec->peer_sid is using > > the first asoc's peer_secid, instead of the latest asoc's peer_secid. > > And not sure if it will cause any problems when doing the extra check > > sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns > > err*. But I don't know about selinux, I guess there must be a reason > > from selinux side. > > Generally speaking we don't want to change any SELinux socket labels > once it has been created. While the peer_sid is a bit different, > changing it after userspace has access to the socket could be > problematic. In the case where the peer_sid differs between the two > we have a permission check which allows policy to control this > behavior which seems like the best option at this point. I think that maybe Xin was referring to the fact that on error return from the hook the return code information is lost and the assoc is just silently dropped (but I may have misunderstood). In case of a denial (avc_has_perm() returning -EACCESS) this isn't much of a problem, because the denial is logged in the audit log, so there is a way to figure out why opening the association failed. In case of other errors we could indeed do better and either log an SELINUX_ERR audit event or at least pr_err() into the console, but there are likely several other existing cases like this, so it would be best to do this cleanup independently in another patch (if anyone feels up to the task...). -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ab32303e6618..dafabb4dcc64 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5238,37 +5238,38 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent) sksec->sclass = isec->sclass; } -/* Called whenever SCTP receives an INIT chunk. This happens when an incoming - * connect(2), sctp_connectx(3) or sctp_sendmsg(3) (with no association - * already present). +/* + * Determines peer_secid for the asoc and updates socket's peer label + * if it's the first association on the socket. */ -static int selinux_sctp_assoc_request(struct sctp_association *asoc, - struct sk_buff *skb) +static int selinux_sctp_process_new_assoc(struct sctp_association *asoc, + struct sk_buff *skb) { - struct sk_security_struct *sksec = asoc->base.sk->sk_security; + struct sock *sk = asoc->base.sk; + u16 family = sk->sk_family; + struct sk_security_struct *sksec = sk->sk_security; struct common_audit_data ad; struct lsm_network_audit net = {0,}; - u8 peerlbl_active; - u32 peer_sid = SECINITSID_UNLABELED; - u32 conn_sid; - int err = 0; + int err; - if (!selinux_policycap_extsockclass()) - return 0; + /* handle mapped IPv4 packets arriving via IPv6 sockets */ + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) + family = PF_INET; - peerlbl_active = selinux_peerlbl_enabled(); + if (selinux_peerlbl_enabled()) { + asoc->peer_secid = SECSID_NULL; - if (peerlbl_active) { /* This will return peer_sid = SECSID_NULL if there are * no peer labels, see security_net_peersid_resolve(). */ - err = selinux_skb_peerlbl_sid(skb, asoc->base.sk->sk_family, - &peer_sid); + err = selinux_skb_peerlbl_sid(skb, family, &asoc->peer_secid); if (err) return err; - if (peer_sid == SECSID_NULL) - peer_sid = SECINITSID_UNLABELED; + if (asoc->peer_secid == SECSID_NULL) + asoc->peer_secid = SECINITSID_UNLABELED; + } else { + asoc->peer_secid = SECINITSID_UNLABELED; } if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) { @@ -5279,8 +5280,8 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc, * then it is approved by policy and used as the primary * peer SID for getpeercon(3). */ - sksec->peer_sid = peer_sid; - } else if (sksec->peer_sid != peer_sid) { + sksec->peer_sid = asoc->peer_secid; + } else if (sksec->peer_sid != asoc->peer_secid) { /* Other association peer SIDs are checked to enforce * consistency among the peer SIDs. */ @@ -5288,11 +5289,32 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc, ad.u.net = &net; ad.u.net->sk = asoc->base.sk; err = avc_has_perm(&selinux_state, - sksec->peer_sid, peer_sid, sksec->sclass, - SCTP_SOCKET__ASSOCIATION, &ad); + sksec->peer_sid, asoc->peer_secid, + sksec->sclass, SCTP_SOCKET__ASSOCIATION, + &ad); if (err) return err; } + return 0; +} + +/* Called whenever SCTP receives an INIT or COOKIE ECHO chunk. This + * happens on an incoming connect(2), sctp_connectx(3) or + * sctp_sendmsg(3) (with no association already present). + */ +static int selinux_sctp_assoc_request(struct sctp_association *asoc, + struct sk_buff *skb) +{ + struct sk_security_struct *sksec = asoc->base.sk->sk_security; + u32 conn_sid; + int err; + + if (!selinux_policycap_extsockclass()) + return 0; + + err = selinux_sctp_process_new_assoc(asoc, skb); + if (err) + return err; /* Compute the MLS component for the connection and store * the information in asoc. This will be used by SCTP TCP type @@ -5300,17 +5322,36 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc, * socket to be generated. selinux_sctp_sk_clone() will then * plug this into the new socket. */ - err = selinux_conn_sid(sksec->sid, peer_sid, &conn_sid); + err = selinux_conn_sid(sksec->sid, asoc->peer_secid, &conn_sid); if (err) return err; asoc->secid = conn_sid; - asoc->peer_secid = peer_sid; /* Set any NetLabel labels including CIPSO/CALIPSO options. */ return selinux_netlbl_sctp_assoc_request(asoc, skb); } +/* Called when SCTP receives a COOKIE ACK chunk as the final + * response to an association request (initited by us). + */ +static int selinux_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ + struct sk_security_struct *sksec = asoc->base.sk->sk_security; + + if (!selinux_policycap_extsockclass()) + return 0; + + /* Inherit secid from the parent socket - this will be picked up + * by selinux_sctp_sk_clone() if the association gets peeled off + * into a new socket. + */ + asoc->secid = sksec->sid; + + return selinux_sctp_process_new_assoc(asoc, skb); +} + /* Check if sctp IPv4/IPv6 addresses are valid for binding or connecting * based on their @optname. */ @@ -7131,6 +7172,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),
Do this by extracting the peer labeling per-association logic from selinux_sctp_assoc_request() into a new helper selinux_sctp_process_new_assoc() and use this helper in both selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This ensures that the peer labeling behavior as documented in Documentation/security/SCTP.rst is applied both on the client and server side: """ An SCTP socket will only have one peer label assigned to it. This will be assigned during the establishment of the first association. Any further associations on this socket will have their packet peer label compared to the sockets peer label, and only if they are different will the ``association`` permission be validated. This is validated by checking the socket peer sid against the received packets peer sid to determine whether the association should be allowed or denied. """ At the same time, it also ensures that the peer label of the association is set to the correct value, such that if it is peeled off into a new socket, the socket's peer label will then be set to the association's peer label, same as it already works on the server side. While selinux_inet_conn_established() (which we are replacing by selinux_sctp_assoc_established() for SCTP) only deals with assigning a peer label to the connection (socket), in case of SCTP we need to also copy the (local) socket label to the association, so that selinux_sctp_sk_clone() can then pick it up for the new socket in case of SCTP peeloff. Careful readers will notice that the selinux_sctp_process_new_assoc() helper also includes the "IPv4 packet received over an IPv6 socket" check, even though it hadn't been in selinux_sctp_assoc_request() before. While such check is not necessary in selinux_inet_conn_request() (because struct request_sock's family field is already set according to the skb's family), here it is needed, as we don't have request_sock and we take the initial family from the socket. In selinux_sctp_assoc_established() it is similarly needed as well (and also selinux_inet_conn_established() already has it). Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Based-on-patch-by: Xin Long <lucien.xin@gmail.com> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 24 deletions(-)