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 <[email protected]>
Signed-off-by: Ondrej Mosnacek <[email protected]>
---
include/net/sctp/structs.h | 11 ++++++-----
net/sctp/sm_statefuns.c | 5 +++++
2 files changed, 11 insertions(+), 5 deletions(-)
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 5:38 PM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
> 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/[email protected]/
>
> 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
>
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
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
<[email protected]> 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 <[email protected]> wrote:
> On Thu, Oct 21, 2021 at 11:55 PM Marcelo Ricardo Leitner
> <[email protected]> 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.