| Message ID | 20211021153846.745289-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | sctp: initialize endpoint LSM labels also on the client side | expand |
On Thu, Oct 21, 2021 at 5:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > The secid* fields in struct sctp_endpoint are used to initialize the > labels of a peeloff socket created from the given association. Currently > they are initialized properly when a new association is created on the > server side (upon receiving an INIT packet), but not on the client side. > > As a result, when the client obtains a peeloff socket via > sctp_peeloff(3) under SELinux, it ends up unlabeled, leading to > unexpected denials. > > Fix this by calling the security_sctp_assoc_request() hook also upon > receiving a valid INIT-ACK response from the server, so that the > endpoint labels are properly initialized also on the client side. > > Fixes: 2277c7cd75e3 ("sctp: Add LSM hooks") > Cc: Richard Haines <richard_c_haines@btinternet.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > include/net/sctp/structs.h | 11 ++++++----- > net/sctp/sm_statefuns.c | 5 +++++ > 2 files changed, 11 insertions(+), 5 deletions(-) See also the selinux-testsuite [1] patch [2] that verifies this fix (i.e. the new tests fail without this patch and pass with it). Not being very familiar with SCTP, I'm not 100% sure if this fix is correct or complete, so reviews are very much welcome. [1] https://github.com/SELinuxProject/selinux-testsuite/ [2] https://patchwork.kernel.org/project/selinux/patch/20211021144543.740762-1-omosnace@redhat.com/ > > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > index 651bba654d77..033a955592dd 100644 > --- a/include/net/sctp/structs.h > +++ b/include/net/sctp/structs.h > @@ -1356,11 +1356,12 @@ struct sctp_endpoint { > > __u8 strreset_enable; > > - /* Security identifiers from incoming (INIT). These are set by > - * security_sctp_assoc_request(). These will only be used by > - * SCTP TCP type sockets and peeled off connections as they > - * cause a new socket to be generated. security_sctp_sk_clone() > - * will then plug these into the new socket. > + /* Security identifiers from incoming (INIT/INIT-ACK). These > + * are set by security_sctp_assoc_request(). These will only > + * be used by SCTP TCP type sockets and peeled off connections > + * as they cause a new socket to be generated. > + * security_sctp_sk_clone() will then plug these into the new > + * socket. > */ > > u32 secid; > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c > index 32df65f68c12..cb291c7f5fb7 100644 > --- a/net/sctp/sm_statefuns.c > +++ b/net/sctp/sm_statefuns.c > @@ -521,6 +521,11 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net, > if (!sctp_vtag_verify(chunk, asoc)) > return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); > > + /* Update socket peer label if first association. */ > + if (security_sctp_assoc_request((struct sctp_endpoint *)ep, > + chunk->skb)) > + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); > + > /* 6.10 Bundling > * An endpoint MUST NOT bundle INIT, INIT ACK or > * SHUTDOWN COMPLETE with any other chunks. > -- > 2.31.1 >
On Thu, Oct 21, 2021 at 05:38:46PM +0200, Ondrej Mosnacek wrote: > The secid* fields in struct sctp_endpoint are used to initialize the > labels of a peeloff socket created from the given association. Currently > they are initialized properly when a new association is created on the > server side (upon receiving an INIT packet), but not on the client side. +Cc Xin
On Thu, Oct 21, 2021 at 11:55 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Thu, Oct 21, 2021 at 05:38:46PM +0200, Ondrej Mosnacek wrote: > > The secid* fields in struct sctp_endpoint are used to initialize the > > labels of a peeloff socket created from the given association. Currently > > they are initialized properly when a new association is created on the > > server side (upon receiving an INIT packet), but not on the client side. > > +Cc Xin Thanks Marcelo, security_sctp_assoc_request() is not supposed to call on the client side, as we can see on TCP. The client side's labels should be set to the connection by selinux_inet_conn_request(). But we can't do it based on the current hooks. The root problem is that the current hooks incorrectly treat sctp_endpoint in SCTP as request_sock in TCP, while it should've been sctp_association. We need a bigger change on the current security sctp code. I will post the patch series in hand, please take a look.
On Fri, Oct 22, 2021 at 8:33 AM Xin Long <lucien.xin@gmail.com> wrote: > On Thu, Oct 21, 2021 at 11:55 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Thu, Oct 21, 2021 at 05:38:46PM +0200, Ondrej Mosnacek wrote: > > > The secid* fields in struct sctp_endpoint are used to initialize the > > > labels of a peeloff socket created from the given association. Currently > > > they are initialized properly when a new association is created on the > > > server side (upon receiving an INIT packet), but not on the client side. > > > > +Cc Xin > Thanks Marcelo, > > security_sctp_assoc_request() is not supposed to call on the client side, > as we can see on TCP. The client side's labels should be set to the > connection by selinux_inet_conn_request(). But we can't do it based > on the current hooks. > > The root problem is that the current hooks incorrectly treat sctp_endpoint > in SCTP as request_sock in TCP, while it should've been sctp_association. > We need a bigger change on the current security sctp code. > > I will post the patch series in hand, please take a look. Thanks, your patches indeed seem to do the right thing and they also do pass selinux-testsuite with the added client peeloff tests (as also confirmed by Richard already). I have just a few minor comments, which I'll send as replies to the individual patches. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 651bba654d77..033a955592dd 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1356,11 +1356,12 @@ struct sctp_endpoint { __u8 strreset_enable; - /* Security identifiers from incoming (INIT). These are set by - * security_sctp_assoc_request(). These will only be used by - * SCTP TCP type sockets and peeled off connections as they - * cause a new socket to be generated. security_sctp_sk_clone() - * will then plug these into the new socket. + /* Security identifiers from incoming (INIT/INIT-ACK). These + * are set by security_sctp_assoc_request(). These will only + * be used by SCTP TCP type sockets and peeled off connections + * as they cause a new socket to be generated. + * security_sctp_sk_clone() will then plug these into the new + * socket. */ u32 secid; diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 32df65f68c12..cb291c7f5fb7 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -521,6 +521,11 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net, if (!sctp_vtag_verify(chunk, asoc)) return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + /* Update socket peer label if first association. */ + if (security_sctp_assoc_request((struct sctp_endpoint *)ep, + chunk->skb)) + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + /* 6.10 Bundling * An endpoint MUST NOT bundle INIT, INIT ACK or * SHUTDOWN COMPLETE with any other chunks.
The secid* fields in struct sctp_endpoint are used to initialize the labels of a peeloff socket created from the given association. Currently they are initialized properly when a new association is created on the server side (upon receiving an INIT packet), but not on the client side. As a result, when the client obtains a peeloff socket via sctp_peeloff(3) under SELinux, it ends up unlabeled, leading to unexpected denials. Fix this by calling the security_sctp_assoc_request() hook also upon receiving a valid INIT-ACK response from the server, so that the endpoint labels are properly initialized also on the client side. Fixes: 2277c7cd75e3 ("sctp: Add LSM hooks") Cc: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- include/net/sctp/structs.h | 11 ++++++----- net/sctp/sm_statefuns.c | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-)