| Message ID | 20240826130711.141271-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3a0504d54b3b57f0d7bf3d9184a00c9f8887f6d7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: fix association labeling in the duplicate COOKIE-ECHO case | expand |
On Mon, Aug 26, 2024 at 9:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > sctp_sf_do_5_2_4_dupcook() currently calls security_sctp_assoc_request() > on new_asoc, but as it turns out, this association is always discarded > and the LSM labels never get into the final association (asoc). > > This can be reproduced by having two SCTP endpoints try to initiate an > association with each other at approximately the same time and then peel > off the association into a new socket, which exposes the unitialized > labels and triggers SELinux denials. > > Fix it by calling security_sctp_assoc_request() on asoc instead of > new_asoc. Xin Long also suggested limit calling the hook only to cases > A, B, and D, since in cases C and E the COOKIE ECHO chunk is discarded > and the association doesn't enter the ESTABLISHED state, so rectify that > as well. > > One related caveat with SELinux and peer labeling: When an SCTP > connection is set up simultaneously in this way, we will end up with an > association that is initialized with security_sctp_assoc_request() on > both sides, so the MLS component of the security context of the > association will get swapped between the peers, instead of just one side > setting it to the other's MLS component. However, at that point > security_sctp_assoc_request() had already been called on both sides in > sctp_sf_do_unexpected_init() (on a temporary association) and thus if > the exchange didn't fail before due to MLS, it won't fail now either > (most likely both endpoints have the same MLS range). > > Tested by: > - reproducer from https://src.fedoraproject.org/tests/selinux/pull-request/530 > - selinux-testsuite (https://github.com/SELinuxProject/selinux-testsuite/) > - sctp-tests (https://github.com/sctp/sctp-tests) - no tests failed > that wouldn't fail also without the patch applied > > Fixes: c081d53f97a1 ("security: pass asoc to sctp_assoc_request and sctp_sk_clone") > Suggested-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Acked-by: Xin Long <lucien.xin@gmail.com>
On Aug 26, 2024 Ondrej Mosnacek <omosnace@redhat.com> wrote: > > sctp_sf_do_5_2_4_dupcook() currently calls security_sctp_assoc_request() > on new_asoc, but as it turns out, this association is always discarded > and the LSM labels never get into the final association (asoc). > > This can be reproduced by having two SCTP endpoints try to initiate an > association with each other at approximately the same time and then peel > off the association into a new socket, which exposes the unitialized > labels and triggers SELinux denials. > > Fix it by calling security_sctp_assoc_request() on asoc instead of > new_asoc. Xin Long also suggested limit calling the hook only to cases > A, B, and D, since in cases C and E the COOKIE ECHO chunk is discarded > and the association doesn't enter the ESTABLISHED state, so rectify that > as well. > > One related caveat with SELinux and peer labeling: When an SCTP > connection is set up simultaneously in this way, we will end up with an > association that is initialized with security_sctp_assoc_request() on > both sides, so the MLS component of the security context of the > association will get swapped between the peers, instead of just one side > setting it to the other's MLS component. However, at that point > security_sctp_assoc_request() had already been called on both sides in > sctp_sf_do_unexpected_init() (on a temporary association) and thus if > the exchange didn't fail before due to MLS, it won't fail now either > (most likely both endpoints have the same MLS range). > > Tested by: > - reproducer from https://src.fedoraproject.org/tests/selinux/pull-request/530 > - selinux-testsuite (https://github.com/SELinuxProject/selinux-testsuite/) > - sctp-tests (https://github.com/sctp/sctp-tests) - no tests failed > that wouldn't fail also without the patch applied > > Fixes: c081d53f97a1 ("security: pass asoc to sctp_assoc_request and sctp_sk_clone") > Suggested-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > Acked-by: Xin Long <lucien.xin@gmail.com> > --- > net/sctp/sm_statefuns.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) Acked-by: Paul Moore <paul@paul-moore.com> (LSM/SELinux) -- paul-moore.com
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 26 Aug 2024 15:07:11 +0200 you wrote: > sctp_sf_do_5_2_4_dupcook() currently calls security_sctp_assoc_request() > on new_asoc, but as it turns out, this association is always discarded > and the LSM labels never get into the final association (asoc). > > This can be reproduced by having two SCTP endpoints try to initiate an > association with each other at approximately the same time and then peel > off the association into a new socket, which exposes the unitialized > labels and triggers SELinux denials. > > [...] Here is the summary with links: - [net] sctp: fix association labeling in the duplicate COOKIE-ECHO case https://git.kernel.org/netdev/net/c/3a0504d54b3b You are awesome, thank you!
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 5adf0c0a6c1a..7d315a18612b 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2260,12 +2260,6 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook( } } - /* Update socket peer label if first association. */ - if (security_sctp_assoc_request(new_asoc, chunk->head_skb ?: chunk->skb)) { - sctp_association_free(new_asoc); - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); - } - /* Set temp so that it won't be added into hashtable */ new_asoc->temp = 1; @@ -2274,6 +2268,22 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook( */ action = sctp_tietags_compare(new_asoc, asoc); + /* In cases C and E the association doesn't enter the ESTABLISHED + * state, so there is no need to call security_sctp_assoc_request(). + */ + switch (action) { + case 'A': /* Association restart. */ + case 'B': /* Collision case B. */ + case 'D': /* Collision case D. */ + /* Update socket peer label if first association. */ + if (security_sctp_assoc_request((struct sctp_association *)asoc, + chunk->head_skb ?: chunk->skb)) { + sctp_association_free(new_asoc); + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + } + break; + } + switch (action) { case 'A': /* Association restart. */ retval = sctp_sf_do_dupcook_a(net, ep, asoc, chunk, commands,
sctp_sf_do_5_2_4_dupcook() currently calls security_sctp_assoc_request() on new_asoc, but as it turns out, this association is always discarded and the LSM labels never get into the final association (asoc). This can be reproduced by having two SCTP endpoints try to initiate an association with each other at approximately the same time and then peel off the association into a new socket, which exposes the unitialized labels and triggers SELinux denials. Fix it by calling security_sctp_assoc_request() on asoc instead of new_asoc. Xin Long also suggested limit calling the hook only to cases A, B, and D, since in cases C and E the COOKIE ECHO chunk is discarded and the association doesn't enter the ESTABLISHED state, so rectify that as well. One related caveat with SELinux and peer labeling: When an SCTP connection is set up simultaneously in this way, we will end up with an association that is initialized with security_sctp_assoc_request() on both sides, so the MLS component of the security context of the association will get swapped between the peers, instead of just one side setting it to the other's MLS component. However, at that point security_sctp_assoc_request() had already been called on both sides in sctp_sf_do_unexpected_init() (on a temporary association) and thus if the exchange didn't fail before due to MLS, it won't fail now either (most likely both endpoints have the same MLS range). Tested by: - reproducer from https://src.fedoraproject.org/tests/selinux/pull-request/530 - selinux-testsuite (https://github.com/SELinuxProject/selinux-testsuite/) - sctp-tests (https://github.com/sctp/sctp-tests) - no tests failed that wouldn't fail also without the patch applied Fixes: c081d53f97a1 ("security: pass asoc to sctp_assoc_request and sctp_sk_clone") Suggested-by: Xin Long <lucien.xin@gmail.com> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- net/sctp/sm_statefuns.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)