| Message ID | cdca8eaca8a0ec5fe4aa58412a6096bb08c3c9bc.1635854268.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: fixups for the security hooks in sctp | expand |
Hi Xin, On Tue, Nov 2, 2021 at 1:03 PM 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. > > v1->v2: > - call selinux_inet_conn_established() to reduce some code > duplication in selinux_sctp_assoc_established(), as Ondrej > suggested. > - when doing peeloff, it calls sock_create() where it actually > gets secid for socket from socket_sockcreate_sid(). So reuse > SECSID_WILD to ensure the peeloff socket keeps using that > secid after calling selinux_sctp_sk_clone() for client side. Interesting... I find strange that SCTP creates the peeloff socket using sock_create() rather than allocating it directly via sock_alloc() like the other callers of sctp_copy_sock() (which calls security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the sock_create() call and just rely on the security_sctp_sk_clone() semantic to set up the labels? Would anything break if sctp_do_peeloff() switched to plain sock_alloc()? I'd rather we avoid this SECSID_WILD hack to support the weird created-but-also-cloned socket hybrid and just make the peeloff socket behave the same as an accept()-ed socket (i.e. no security_socket_[post_]create() hook calls, just security_sctp_sk_clone()). > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > Tested-by: Richard Haines <richard_c_haines@btinternet.com> You made non-trivial changes since the last revision in this patch, so you should have also dropped the Reviewed-by and Tested-by here. Now David has merged the patches probably under the impression that they have been reviewed/approved from the SELinux side, which isn't completely true. > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > security/selinux/hooks.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a9977a2ae8ac..341cd5dccbf5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > - newsksec->sid = asoc->secid; > + if (asoc->secid != SECSID_WILD) > + newsksec->sid = asoc->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > } > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > + struct sk_buff *skb) > +{ > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > + > + selinux_inet_conn_established(asoc->base.sk, skb); > + asoc->peer_secid = sksec->peer_sid; > + asoc->secid = SECSID_WILD; > +} > + > static int selinux_secmark_relabel_packet(u32 sid) > { > const struct task_security_struct *__tsec; > @@ -7290,6 +7301,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 Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > Hi Xin, > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > v1->v2: > > - call selinux_inet_conn_established() to reduce some code > > duplication in selinux_sctp_assoc_established(), as Ondrej > > suggested. > > - when doing peeloff, it calls sock_create() where it actually > > gets secid for socket from socket_sockcreate_sid(). So reuse > > SECSID_WILD to ensure the peeloff socket keeps using that > > secid after calling selinux_sctp_sk_clone() for client side. > > Interesting... I find strange that SCTP creates the peeloff socket > using sock_create() rather than allocating it directly via > sock_alloc() like the other callers of sctp_copy_sock() (which calls > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > sock_create() call and just rely on the security_sctp_sk_clone() > semantic to set up the labels? Would anything break if > sctp_do_peeloff() switched to plain sock_alloc()? > > I'd rather we avoid this SECSID_WILD hack to support the weird > created-but-also-cloned socket hybrid and just make the peeloff socket > behave the same as an accept()-ed socket (i.e. no > security_socket_[post_]create() hook calls, just > security_sctp_sk_clone()). please check Paul's comment: """ 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. """ That's what I got from it: For client side, secid should be copied from its parent socket directly, but get it from socket_sockcreate_sid(). and you? > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > You made non-trivial changes since the last revision in this patch, so > you should have also dropped the Reviewed-by and Tested-by here. Now > David has merged the patches probably under the impression that they > have been reviewed/approved from the SELinux side, which isn't > completely true. Oh, that's a mistake, I thought I didn't add it. Will he be able to test this new patchset? Thanks. > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > security/selinux/hooks.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index a9977a2ae8ac..341cd5dccbf5 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > if (!selinux_policycap_extsockclass()) > > return selinux_sk_clone_security(sk, newsk); > > > > - newsksec->sid = asoc->secid; > > + if (asoc->secid != SECSID_WILD) > > + newsksec->sid = asoc->secid; > > newsksec->peer_sid = asoc->peer_secid; > > newsksec->sclass = sksec->sclass; > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > } > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > + struct sk_buff *skb) > > +{ > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > + > > + selinux_inet_conn_established(asoc->base.sk, skb); > > + asoc->peer_secid = sksec->peer_sid; > > + asoc->secid = SECSID_WILD; > > +} > > + > > static int selinux_secmark_relabel_packet(u32 sid) > > { > > const struct task_security_struct *__tsec; > > @@ -7290,6 +7301,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 Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > Hi Xin, > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > v1->v2: > > > - call selinux_inet_conn_established() to reduce some code > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > suggested. > > > - when doing peeloff, it calls sock_create() where it actually > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > Interesting... I find strange that SCTP creates the peeloff socket > > using sock_create() rather than allocating it directly via > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > sock_create() call and just rely on the security_sctp_sk_clone() > > semantic to set up the labels? Would anything break if > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > created-but-also-cloned socket hybrid and just make the peeloff socket > > behave the same as an accept()-ed socket (i.e. no > > security_socket_[post_]create() hook calls, just > > security_sctp_sk_clone()). > > please check Paul's comment: > > """ > 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. > """ > > That's what I got from it: > For client side, secid should be copied from its parent socket directly, but > get it from socket_sockcreate_sid(). For client side, secid should NOT be copied from its parent socket directly, but gets it from socket_sockcreate_sid(). > > and you? > > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > You made non-trivial changes since the last revision in this patch, so > > you should have also dropped the Reviewed-by and Tested-by here. Now > > David has merged the patches probably under the impression that they > > have been reviewed/approved from the SELinux side, which isn't > > completely true. > Oh, that's a mistake, I thought I didn't add it. > Will he be able to test this new patchset? > > Thanks. > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > security/selinux/hooks.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index a9977a2ae8ac..341cd5dccbf5 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > > if (!selinux_policycap_extsockclass()) > > > return selinux_sk_clone_security(sk, newsk); > > > > > > - newsksec->sid = asoc->secid; > > > + if (asoc->secid != SECSID_WILD) > > > + newsksec->sid = asoc->secid; > > > newsksec->peer_sid = asoc->peer_secid; > > > newsksec->sclass = sksec->sclass; > > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > > > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > > } > > > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > > + struct sk_buff *skb) > > > +{ > > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > > + > > > + selinux_inet_conn_established(asoc->base.sk, skb); > > > + asoc->peer_secid = sksec->peer_sid; > > > + asoc->secid = SECSID_WILD; > > > +} > > > + > > > static int selinux_secmark_relabel_packet(u32 sid) > > > { > > > const struct task_security_struct *__tsec; > > > @@ -7290,6 +7301,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 Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > v1->v2: > > > > - call selinux_inet_conn_established() to reduce some code > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > suggested. > > > > - when doing peeloff, it calls sock_create() where it actually > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > using sock_create() rather than allocating it directly via > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > semantic to set up the labels? Would anything break if > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > behave the same as an accept()-ed socket (i.e. no > > > security_socket_[post_]create() hook calls, just > > > security_sctp_sk_clone()). I believe the important part is that sctp_do_peeloff() eventually calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming we have security_sctp_sk_clone() working properly I would expect that the new socket would be setup properly when sctp_do_peeloff() returns on success. ... and yes, that SECSID_WILD approach is *not* something we want to do. In my mind, selinux_sctp_sk_clone() should end up looking like this. void selinux_sctp_sk_clone(asoc, sk, newsk) { struct sk_security_struct sksec = sk->sk_security; struct sk_security_struct newsksec = newsk->sk_security; if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); newsksec->sid = sksec->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); } Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be removed from selinux_sctp_assoc_established(). If we are treating SCTP associations similarly to TCP connections, the association's label/secid should be set once and not changed during the life of the association. > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > You made non-trivial changes since the last revision in this patch, so > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > David has merged the patches probably under the impression that they > > > have been reviewed/approved from the SELinux side, which isn't > > > completely true. > > > > Oh, that's a mistake, I thought I didn't add it. > > Will he be able to test this new patchset? While I tend to try to avoid reverts as much as possible, I think the right thing to do is to get these patches reverted out of DaveM's tree while we continue to sort this out and do all of the necessary testing and verification. Xin Long, please work with the netdev folks to get your patchset reverted and then respin this patchset using the feedback provided.
On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > v1->v2: > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > suggested. > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > using sock_create() rather than allocating it directly via > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > semantic to set up the labels? Would anything break if > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > behave the same as an accept()-ed socket (i.e. no > > > > security_socket_[post_]create() hook calls, just > > > > security_sctp_sk_clone()). > > I believe the important part is that sctp_do_peeloff() eventually > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > we have security_sctp_sk_clone() working properly I would expect that > the new socket would be setup properly when sctp_do_peeloff() returns > on success. > > ... and yes, that SECSID_WILD approach is *not* something we want to do. SECSID_WILD is used to avoid client's new socket's sid overwritten by old socket's. If I understand correctly, new socket's should keep using its original sid, namely, the one set from security_socket_[post_]create() on client side. I AGREE with that. Now I want to *confirm* this with you, as it's different from the last version's 'inherit from parent socket' that Richard and Ondrej reviewed. > > In my mind, selinux_sctp_sk_clone() should end up looking like this. > > void selinux_sctp_sk_clone(asoc, sk, newsk) > { > struct sk_security_struct sksec = sk->sk_security; > struct sk_security_struct newsksec = newsk->sk_security; > > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->sid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } Let's say, this socket has 3 associations now, how can we ensure the new socket's sid is set to the right sid? I don't think we can use "sksec->secid" in this place, this is not TCP. > > Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be > removed from selinux_sctp_assoc_established(). If we are treating > SCTP associations similarly to TCP connections, the association's > label/secid should be set once and not changed during the life of the > association. The association's label/secid will never change once set in this patchset. it's just a temporary record, and later it will be used to set socket's label/secid. I think that's the idea at the beginning. > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > David has merged the patches probably under the impression that they > > > > have been reviewed/approved from the SELinux side, which isn't > > > > completely true. > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > Will he be able to test this new patchset? > > While I tend to try to avoid reverts as much as possible, I think the > right thing to do is to get these patches reverted out of DaveM's tree > while we continue to sort this out and do all of the necessary testing > and verification. > > Xin Long, please work with the netdev folks to get your patchset > reverted and then respin this patchset using the feedback provided. Hi, Paul, The original issue this patchset fixes is a crucial one (it could cause peeloff sockets on client side to not work) which I think can already be fixed now. If you think SECSID_WILD is tricky but no better way yet, my suggestion is to leave it for now until we have a better solution to follow up. As I couldn't find a better way to work it out. Also, we may want to hear Richard's opinion on how it should work and how this should be fixed. Thanks. > > -- > paul moore > www.paul-moore.com
On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > > > v1->v2: > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > suggested. > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > using sock_create() rather than allocating it directly via > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > semantic to set up the labels? Would anything break if > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > security_socket_[post_]create() hook calls, just > > > > > security_sctp_sk_clone()). > > > > I believe the important part is that sctp_do_peeloff() eventually > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > we have security_sctp_sk_clone() working properly I would expect that > > the new socket would be setup properly when sctp_do_peeloff() returns > > on success. > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > old socket's. In the case of security_sctp_sk_clone() the new client socket (the cloned socket) should inherit the label/sid from the original socket (the "parent" in the inherit-from-parent label inheritance behavior discussed earlier). The selinux_sctp_assoc_established() function should not change the socket's label/sid at all, only the peer label. > If I understand correctly, new socket's should keep using its original > sid, namely, > the one set from security_socket_[post_]create() on client side. I > AGREE with that. > Now I want to *confirm* this with you, as it's different from the last version's > 'inherit from parent socket' that Richard and Ondrej reviewed. Unfortunately I think we are struggling to communicate because you are not familiar with SELinux concepts and I'm not as well versed in SCTP as you are. As things currently stand, I am getting a disconnect between your explanations and the code you have submitted; they simply aren't consistent from my perspective. In an effort to help provide something that is hopefully a bit more clear, here are the selinux_sctp_sk_clone() and selinux_sctp_assoc_established() functions which I believe we need. If you feel these are incorrect, please explain and/or provide edits: static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk) { struct sk_security_struct *sksec = sk->sk_security; struct sk_security_struct *newsksec = newsk->sk_security; /* If policy does not support SECCLASS_SCTP_SOCKET then call * the non-sctp clone version. */ if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); newsksec->secid = sksec->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; 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; selinux_inet_conn_established(asoc->base.sk, skb); asoc->peer_secid = sksec->peer_sid; } > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > > David has merged the patches probably under the impression that they > > > > > have been reviewed/approved from the SELinux side, which isn't > > > > > completely true. > > > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > > Will he be able to test this new patchset? > > > > While I tend to try to avoid reverts as much as possible, I think the > > right thing to do is to get these patches reverted out of DaveM's tree > > while we continue to sort this out and do all of the necessary testing > > and verification. > > > > Xin Long, please work with the netdev folks to get your patchset > > reverted and then respin this patchset using the feedback provided. > > Hi, Paul, > > The original issue this patchset fixes is a crucial one (it could cause > peeloff sockets on client side to not work) which I think > can already be fixed now. If you think SECSID_WILD is tricky but > no better way yet, my suggestion is to leave it for now until we have > a better solution to follow up. As I couldn't find a better way to work > it out. Also, we may want to hear Richard's opinion on how it should > work and how this should be fixed. While I understand you did not intend to mislead DaveM and the netdev folks with the v2 patchset, your failure to properly manage the patchset's metadata *did* mislead them and as a result a patchset with serious concerns from the SELinux side was merged. You need to revert this patchset while we continue to discuss, develop, and verify a proper fix that we can all agree on. If you decide not to revert this patchset I will work with DaveM to do it for you, and that is not something any of us wants.
On Wed, 2021-11-03 at 23:17 -0400, Paul Moore wrote: > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> > > wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> > > > wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> > > > > wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek > > > > > <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some > > > > > > > code > > > > > > > duplication in selinux_sctp_assoc_established(), as > > > > > > > Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it > > > > > > > actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So > > > > > > > reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using > > > > > > > that > > > > > > > secid after calling selinux_sctp_sk_clone() for client > > > > > > > side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff > > > > > > socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() > > > > > > (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to > > > > > > avoid the > > > > > > sock_create() call and just rely on the > > > > > > security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the > > > > > > weird > > > > > > created-but-also-cloned socket hybrid and just make the > > > > > > peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). > > > Assuming > > > we have security_sctp_sk_clone() working properly I would expect > > > that > > > the new socket would be setup properly when sctp_do_peeloff() > > > returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want > > > to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. > > > If I understand correctly, new socket's should keep using its > > original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the > > last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock > *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; This should be: newsksec->sid = sksec->sid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > 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; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > }
On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > we have security_sctp_sk_clone() working properly I would expect that > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. > > > If I understand correctly, new socket's should keep using its original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > 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; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > } This code would be functionally equivalent to the v1 patchset for the client side, but on server side you want to set newsksec->secid to asoc->secid, as this contains the "connection secid" computed by selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone() does for non-SCTP sockets. So I think we should rather go back to the v1 patchset variant, where the parent socket's sid is stashed in asoc->secid to be picked up by selinux_sctp_sk_clone(). As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious about the difference between the sock vs. socket structs, so this would be a bit more difficult to fix than replacing one function call. But if we end up just overwriting the label assigned in selinux_socket_post_create() as it is now, then the only difference is an unexpected SCTP_SOCKET__CREATE permission check and a pointless computation of socket_sockcreate_sid(), so it can be addressed separately. I'll try to suggest a patch and then we can discuss whether it makes sense or not. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Wed, Nov 3, 2021 at 11:17 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > we have security_sctp_sk_clone() working properly I would expect that > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket """ 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]. [2] I'm guessing the client associations might also want to follow the setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more info. """ What I got is to take it's label from the parent process, which means we get it from socket_sockcreate_sid(), not directly copy from parent socket. It seems I misunderstood that, Sorry, maybe we should just use the v1 patchset. > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. Right, that's what it currently does in this patchset, no *socket* sid is changed, and only *socket*'s peer label. { struct sk_security_struct *sksec = asoc->base.sk->sk_security; selinux_inet_conn_established(asoc->base.sk, skb); asoc->peer_secid = sksec->peer_sid; asoc->secid = SECSID_WILD; } > > > If I understand correctly, new socket's should keep using its original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } here, SCTP is one-to-many socket, and it means one socket can have multiple associations or connections, so for sksec->sid in one socket it can only save the latest cid, if we peel off an old one, it will get the wrong cid on server side. > > static void selinux_sctp_assoc_established(struct sctp_association *asoc, > struct sk_buff *skb) > { > struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > } > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > > > David has merged the patches probably under the impression that they > > > > > > have been reviewed/approved from the SELinux side, which isn't > > > > > > completely true. > > > > > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > > > Will he be able to test this new patchset? > > > > > > While I tend to try to avoid reverts as much as possible, I think the > > > right thing to do is to get these patches reverted out of DaveM's tree > > > while we continue to sort this out and do all of the necessary testing > > > and verification. > > > > > > Xin Long, please work with the netdev folks to get your patchset > > > reverted and then respin this patchset using the feedback provided. > > > > Hi, Paul, > > > > The original issue this patchset fixes is a crucial one (it could cause > > peeloff sockets on client side to not work) which I think > > can already be fixed now. If you think SECSID_WILD is tricky but > > no better way yet, my suggestion is to leave it for now until we have > > a better solution to follow up. As I couldn't find a better way to work > > it out. Also, we may want to hear Richard's opinion on how it should > > work and how this should be fixed. > > While I understand you did not intend to mislead DaveM and the netdev > folks with the v2 patchset, your failure to properly manage the > patchset's metadata *did* mislead them and as a result a patchset with > serious concerns from the SELinux side was merged. You need to revert > this patchset while we continue to discuss, develop, and verify a > proper fix that we can all agree on. If you decide not to revert this > patchset I will work with DaveM to do it for you, and that is not > something any of us wants. > > -- > paul moore > www.paul-moore.com
From: Paul Moore <paul@paul-moore.com> Date: Wed, 3 Nov 2021 23:17:00 -0400 > > While I understand you did not intend to mislead DaveM and the netdev > folks with the v2 patchset, your failure to properly manage the > patchset's metadata *did* mislead them and as a result a patchset with > serious concerns from the SELinux side was merged. You need to revert > this patchset while we continue to discuss, develop, and verify a > proper fix that we can all agree on. If you decide not to revert this > patchset I will work with DaveM to do it for you, and that is not > something any of us wants. I would prefer a follow-up rathewr than a revert at this point. Please work with Xin to come up with a fix that works for both of you. Thanks.
On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > From: Paul Moore <paul@paul-moore.com> > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > While I understand you did not intend to mislead DaveM and the netdev > > folks with the v2 patchset, your failure to properly manage the > > patchset's metadata *did* mislead them and as a result a patchset with > > serious concerns from the SELinux side was merged. You need to revert > > this patchset while we continue to discuss, develop, and verify a > > proper fix that we can all agree on. If you decide not to revert this > > patchset I will work with DaveM to do it for you, and that is not > > something any of us wants. > > I would prefer a follow-up rathewr than a revert at this point. > > Please work with Xin to come up with a fix that works for both of you. We are working with Xin (see this thread), but you'll notice there is still not a clear consensus on the best path forward. The only thing I am clear on at this point is that the current code in linux-next is *not* something we want from a SELinux perspective. I don't like leaving known bad code like this in linux-next for more than a day or two so please revert it, now. If your policy is to merge substantive non-network subsystem changes into the network tree without the proper ACKs from the other subsystem maintainers, it would seem reasonable to also be willing to revert those patches when the affected subsystems request it. I understand that if a patchset is being ignored you might feel the need to act without an explicit ACK, but this particular patchset wasn't even a day old before you merged into the netdev tree. Not to mention that the patchset was posted during the second day of the merge window, a time when many maintainers are busy testing code, sending pull requests to Linus, and generally managing merge window fallout.
On Thu, Nov 4, 2021 at 6:40 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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. > > > > > > > > > > > > > > > > v1->v2: > > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > > suggested. > > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > > using sock_create() rather than allocating it directly via > > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > > semantic to set up the labels? Would anything break if > > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > > security_socket_[post_]create() hook calls, just > > > > > > > security_sctp_sk_clone()). > > > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > > we have security_sctp_sk_clone() working properly I would expect that > > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > > on success. > > > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > > old socket's. > > > > In the case of security_sctp_sk_clone() the new client socket (the > > cloned socket) should inherit the label/sid from the original socket > > (the "parent" in the inherit-from-parent label inheritance behavior > > discussed earlier). The selinux_sctp_assoc_established() function > > should not change the socket's label/sid at all, only the peer label. > > > > > If I understand correctly, new socket's should keep using its original > > > sid, namely, > > > the one set from security_socket_[post_]create() on client side. I > > > AGREE with that. > > > Now I want to *confirm* this with you, as it's different from the last version's > > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > > > Unfortunately I think we are struggling to communicate because you are > > not familiar with SELinux concepts and I'm not as well versed in SCTP > > as you are. As things currently stand, I am getting a disconnect > > between your explanations and the code you have submitted; they simply > > aren't consistent from my perspective. > > > > In an effort to help provide something that is hopefully a bit more > > clear, here are the selinux_sctp_sk_clone() and > > selinux_sctp_assoc_established() functions which I believe we need. > > If you feel these are incorrect, please explain and/or provide edits: > > > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > > struct sock *sk, struct sock *newsk) > > { > > struct sk_security_struct *sksec = sk->sk_security; > > struct sk_security_struct *newsksec = newsk->sk_security; > > > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > > * the non-sctp clone version. > > */ > > if (!selinux_policycap_extsockclass()) > > return selinux_sk_clone_security(sk, newsk); > > > > newsksec->secid = sksec->secid; > > newsksec->peer_sid = asoc->peer_secid; > > newsksec->sclass = sksec->sclass; > > 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; > > > > selinux_inet_conn_established(asoc->base.sk, skb); > > asoc->peer_secid = sksec->peer_sid; > > } > > This code would be functionally equivalent to the v1 patchset for the > client side, but on server side you want to set newsksec->secid to > asoc->secid, as this contains the "connection secid" computed by > selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed > to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone() > does for non-SCTP sockets. So I think we should rather go back to the > v1 patchset variant, where the parent socket's sid is stashed in > asoc->secid to be picked up by selinux_sctp_sk_clone(). > > As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious > about the difference between the sock vs. socket structs, so this > would be a bit more difficult to fix than replacing one function call. > But if we end up just overwriting the label assigned in > selinux_socket_post_create() as it is now, then the only difference is > an unexpected SCTP_SOCKET__CREATE permission check and a pointless > computation of socket_sockcreate_sid(), so it can be addressed > separately. I'll try to suggest a patch and then we can discuss > whether it makes sense or not. Okay, I'll wait on that patchset before commenting further.
On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > > From: Paul Moore <paul@paul-moore.com> > > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > > > While I understand you did not intend to mislead DaveM and the netdev > > > folks with the v2 patchset, your failure to properly manage the > > > patchset's metadata *did* mislead them and as a result a patchset with > > > serious concerns from the SELinux side was merged. You need to revert > > > this patchset while we continue to discuss, develop, and verify a > > > proper fix that we can all agree on. If you decide not to revert this > > > patchset I will work with DaveM to do it for you, and that is not > > > something any of us wants. > > > > I would prefer a follow-up rathewr than a revert at this point. > > > > Please work with Xin to come up with a fix that works for both of you. > > We are working with Xin (see this thread), but you'll notice there is > still not a clear consensus on the best path forward. The only thing > I am clear on at this point is that the current code in linux-next is > *not* something we want from a SELinux perspective. I don't like > leaving known bad code like this in linux-next for more than a day or > two so please revert it, now. If your policy is to merge substantive > non-network subsystem changes into the network tree without the proper > ACKs from the other subsystem maintainers, it would seem reasonable to > also be willing to revert those patches when the affected subsystems > request it. > > I understand that if a patchset is being ignored you might feel the > need to act without an explicit ACK, but this particular patchset > wasn't even a day old before you merged into the netdev tree. Not to > mention that the patchset was posted during the second day of the > merge window, a time when many maintainers are busy testing code, > sending pull requests to Linus, and generally managing merge window > fallout. > > -- > paul moore > www.paul-moore.com Hi Paul, It's applied on net tree, I think mostly because I posted this on net.git tree. Also, it's well related to the network part and affects SCTP protocol quite a lot. I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the commit on top is written in 2019: commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main, origin/main, origin/HEAD) Author: Paul Moore <paul@paul-moore.com> Date: Tue Sep 17 15:02:56 2019 -0400 selinux: add a SELinux specific README.md DO NOT SUBMIT UPSTREAM Then I thought this tree was no longer active, sorry about that.
On Thu, Nov 4, 2021 at 3:49 PM Xin Long <lucien.xin@gmail.com> wrote: > On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > > > From: Paul Moore <paul@paul-moore.com> > > > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > > > > > While I understand you did not intend to mislead DaveM and the netdev > > > > folks with the v2 patchset, your failure to properly manage the > > > > patchset's metadata *did* mislead them and as a result a patchset with > > > > serious concerns from the SELinux side was merged. You need to revert > > > > this patchset while we continue to discuss, develop, and verify a > > > > proper fix that we can all agree on. If you decide not to revert this > > > > patchset I will work with DaveM to do it for you, and that is not > > > > something any of us wants. > > > > > > I would prefer a follow-up rathewr than a revert at this point. > > > > > > Please work with Xin to come up with a fix that works for both of you. > > > > We are working with Xin (see this thread), but you'll notice there is > > still not a clear consensus on the best path forward. The only thing > > I am clear on at this point is that the current code in linux-next is > > *not* something we want from a SELinux perspective. I don't like > > leaving known bad code like this in linux-next for more than a day or > > two so please revert it, now. If your policy is to merge substantive > > non-network subsystem changes into the network tree without the proper > > ACKs from the other subsystem maintainers, it would seem reasonable to > > also be willing to revert those patches when the affected subsystems > > request it. > > > > I understand that if a patchset is being ignored you might feel the > > need to act without an explicit ACK, but this particular patchset > > wasn't even a day old before you merged into the netdev tree. Not to > > mention that the patchset was posted during the second day of the > > merge window, a time when many maintainers are busy testing code, > > sending pull requests to Linus, and generally managing merge window > > fallout. > > Hi Paul, > > It's applied on net tree, I think mostly because I posted this on net.git tree. > Also, it's well related to the network part and affects SCTP protocol > quite a lot. Yes, I know it is in the net tree, that is how it made its way into linux-next. I wouldn't have merged it yet, and if not me who else would have merged it beside the netdev folks? Am I misunderstanding your comment? > I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the > commit on top is written in 2019: > > commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main, > origin/main, origin/HEAD) > Author: Paul Moore <paul@paul-moore.com> > Date: Tue Sep 17 15:02:56 2019 -0400 > > selinux: add a SELinux specific README.md > > DO NOT SUBMIT UPSTREAM > > Then I thought this tree was no longer active, sorry about that. Like many kernel trees the default/main branch for the SELinux tree doesn't contain anything useful, for the SELinux tree (and audit for that matter) it is basically just the most recent major/minor tag from Linus tree with a single tree specific README.md file patch so that the GitHub mirror has a pretty landing page and a canonical reference for how the tree is maintained. * https://github.com/SELinuxProject/selinux-kernel The general approach to the SELinux tree, as documented in the README.md, is to do all of the linux-next work in the selinux/next branch with the stable work happening in the selinux/stable-X.Y branches. FWIW, once we've resolved things I would be happy to have the patchset live in the SELinux tree as opposed to the netdev tree.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a9977a2ae8ac..341cd5dccbf5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); - newsksec->sid = asoc->secid; + if (asoc->secid != SECSID_WILD) + newsksec->sid = asoc->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); } +static void selinux_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ + struct sk_security_struct *sksec = asoc->base.sk->sk_security; + + selinux_inet_conn_established(asoc->base.sk, skb); + asoc->peer_secid = sksec->peer_sid; + asoc->secid = SECSID_WILD; +} + static int selinux_secmark_relabel_packet(u32 sid) { const struct task_security_struct *__tsec; @@ -7290,6 +7301,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. v1->v2: - call selinux_inet_conn_established() to reduce some code duplication in selinux_sctp_assoc_established(), as Ondrej suggested. - when doing peeloff, it calls sock_create() where it actually gets secid for socket from socket_sockcreate_sid(). So reuse SECSID_WILD to ensure the peeloff socket keeps using that secid after calling selinux_sctp_sk_clone() for client side. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- security/selinux/hooks.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)