On Thu, Oct 21, 2021 at 5:38 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
>
> 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.
