Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Ondrej Mosnacek]

Adding LSM and SELinux lists to CC for awareness; the original patch
is available at:
https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/

On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> Yi Chen reported an unexpected sctp connection abort, and it occurred when
> COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> is included in chunk->head_skb instead of chunk->skb, it failed to check
> IP header version in security_sctp_assoc_request().
>
> According to Ondrej, SELinux only looks at IP header (address and IPsec
> options) and XFRM state data, and these are all included in head_skb for
> SCTP HW GSO packets. So fix it by using head_skb when calling
> security_sctp_assoc_request() in processing COOKIE_ECHO.

The logic looks good to me, but I still have one unanswered concern.
The head_skb member of struct sctp_chunk is defined inside a union:

struct sctp_chunk {
        [...]
        union {
                /* In case of GSO packets, this will store the head one */
                struct sk_buff *head_skb;
                /* In case of auth enabled, this will point to the shkey */
                struct sctp_shared_key *shkey;
        };
        [...]
};

What guarantees that this chunk doesn't have "auth enabled" and the
head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
obvious to a Linux SCTP expert, but at least for me as an outsider it
isn't - that's usually a good hint that there should be a code comment
explaining it.

>
> Fixes: e215dab1c490 ("security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce")
> Reported-by: Yi Chen <yiche@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/sm_statefuns.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 7f342bc12735..883f9b849ee5 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -781,7 +781,7 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
>                 }
>         }
>
> -       if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
> +       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);
>         }
> @@ -2262,7 +2262,7 @@ 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->skb)) {
> +       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);
>         }
> --
> 2.31.1
>

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Xin Long]

On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Adding LSM and SELinux lists to CC for awareness; the original patch
> is available at:
> https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
> https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/
>
> On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > Yi Chen reported an unexpected sctp connection abort, and it occurred when
> > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> > is included in chunk->head_skb instead of chunk->skb, it failed to check
> > IP header version in security_sctp_assoc_request().
> >
> > According to Ondrej, SELinux only looks at IP header (address and IPsec
> > options) and XFRM state data, and these are all included in head_skb for
> > SCTP HW GSO packets. So fix it by using head_skb when calling
> > security_sctp_assoc_request() in processing COOKIE_ECHO.
>
> The logic looks good to me, but I still have one unanswered concern.
> The head_skb member of struct sctp_chunk is defined inside a union:
>
> struct sctp_chunk {
>         [...]
>         union {
>                 /* In case of GSO packets, this will store the head one */
>                 struct sk_buff *head_skb;
>                 /* In case of auth enabled, this will point to the shkey */
>                 struct sctp_shared_key *shkey;
>         };
>         [...]
> };
>
> What guarantees that this chunk doesn't have "auth enabled" and the
> head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
> obvious to a Linux SCTP expert, but at least for me as an outsider it
> isn't - that's usually a good hint that there should be a code comment
> explaining it.
Hi Ondrej,

shkey is for tx skbs only, while head_skb is for skbs on rx path.

Thanks.

>
> >
> > Fixes: e215dab1c490 ("security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce")
> > Reported-by: Yi Chen <yiche@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/sm_statefuns.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index 7f342bc12735..883f9b849ee5 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -781,7 +781,7 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
> >                 }
> >         }
> >
> > -       if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
> > +       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);
> >         }
> > @@ -2262,7 +2262,7 @@ 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->skb)) {
> > +       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);
> >         }
> > --
> > 2.31.1
> >
>
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Ondrej Mosnacek]

On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Adding LSM and SELinux lists to CC for awareness; the original patch
> > is available at:
> > https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
> > https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/
> >
> > On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > Yi Chen reported an unexpected sctp connection abort, and it occurred when
> > > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> > > is included in chunk->head_skb instead of chunk->skb, it failed to check
> > > IP header version in security_sctp_assoc_request().
> > >
> > > According to Ondrej, SELinux only looks at IP header (address and IPsec
> > > options) and XFRM state data, and these are all included in head_skb for
> > > SCTP HW GSO packets. So fix it by using head_skb when calling
> > > security_sctp_assoc_request() in processing COOKIE_ECHO.
> >
> > The logic looks good to me, but I still have one unanswered concern.
> > The head_skb member of struct sctp_chunk is defined inside a union:
> >
> > struct sctp_chunk {
> >         [...]
> >         union {
> >                 /* In case of GSO packets, this will store the head one */
> >                 struct sk_buff *head_skb;
> >                 /* In case of auth enabled, this will point to the shkey */
> >                 struct sctp_shared_key *shkey;
> >         };
> >         [...]
> > };
> >
> > What guarantees that this chunk doesn't have "auth enabled" and the
> > head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
> > obvious to a Linux SCTP expert, but at least for me as an outsider it
> > isn't - that's usually a good hint that there should be a code comment
> > explaining it.
> Hi Ondrej,
>
> shkey is for tx skbs only, while head_skb is for skbs on rx path.

That makes sense, thanks. I would still be happier if this was
documented, but the comment would best fit in the struct sctp_chunk
definition and that wouldn't fit in this patch...

Actually I have one more question - what about the
security_sctp_assoc_established() call in sctp_sf_do_5_1E_ca()? Is
COOKIE ACK guaranteed to be never bundled?

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Xin Long]

On Wed, Apr 6, 2022 at 9:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Adding LSM and SELinux lists to CC for awareness; the original patch
> > > is available at:
> > > https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
> > > https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/
> > >
> > > On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
> > > >
> > > > Yi Chen reported an unexpected sctp connection abort, and it occurred when
> > > > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> > > > is included in chunk->head_skb instead of chunk->skb, it failed to check
> > > > IP header version in security_sctp_assoc_request().
> > > >
> > > > According to Ondrej, SELinux only looks at IP header (address and IPsec
> > > > options) and XFRM state data, and these are all included in head_skb for
> > > > SCTP HW GSO packets. So fix it by using head_skb when calling
> > > > security_sctp_assoc_request() in processing COOKIE_ECHO.
> > >
> > > The logic looks good to me, but I still have one unanswered concern.
> > > The head_skb member of struct sctp_chunk is defined inside a union:
> > >
> > > struct sctp_chunk {
> > >         [...]
> > >         union {
> > >                 /* In case of GSO packets, this will store the head one */
> > >                 struct sk_buff *head_skb;
> > >                 /* In case of auth enabled, this will point to the shkey */
> > >                 struct sctp_shared_key *shkey;
> > >         };
> > >         [...]
> > > };
> > >
> > > What guarantees that this chunk doesn't have "auth enabled" and the
> > > head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
> > > obvious to a Linux SCTP expert, but at least for me as an outsider it
> > > isn't - that's usually a good hint that there should be a code comment
> > > explaining it.
> > Hi Ondrej,
> >
> > shkey is for tx skbs only, while head_skb is for skbs on rx path.
>
> That makes sense, thanks. I would still be happier if this was
> documented, but the comment would best fit in the struct sctp_chunk
> definition and that wouldn't fit in this patch...
>
> Actually I have one more question - what about the
> security_sctp_assoc_established() call in sctp_sf_do_5_1E_ca()? Is
> COOKIE ACK guaranteed to be never bundled?
COOKIE ACK could also be bundled with DATA.
I didn't change it as it would not break SCTP.
(security_inet_conn_established() returns void)
But I don't mind changing it if you think it's necessary.

Thanks.

>
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Ondrej Mosnacek]

On Wed, Apr 6, 2022 at 4:21 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Wed, Apr 6, 2022 at 9:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Adding LSM and SELinux lists to CC for awareness; the original patch
> > > > is available at:
> > > > https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
> > > > https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/
> > > >
> > > > On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
> > > > >
> > > > > Yi Chen reported an unexpected sctp connection abort, and it occurred when
> > > > > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> > > > > is included in chunk->head_skb instead of chunk->skb, it failed to check
> > > > > IP header version in security_sctp_assoc_request().
> > > > >
> > > > > According to Ondrej, SELinux only looks at IP header (address and IPsec
> > > > > options) and XFRM state data, and these are all included in head_skb for
> > > > > SCTP HW GSO packets. So fix it by using head_skb when calling
> > > > > security_sctp_assoc_request() in processing COOKIE_ECHO.
> > > >
> > > > The logic looks good to me, but I still have one unanswered concern.
> > > > The head_skb member of struct sctp_chunk is defined inside a union:
> > > >
> > > > struct sctp_chunk {
> > > >         [...]
> > > >         union {
> > > >                 /* In case of GSO packets, this will store the head one */
> > > >                 struct sk_buff *head_skb;
> > > >                 /* In case of auth enabled, this will point to the shkey */
> > > >                 struct sctp_shared_key *shkey;
> > > >         };
> > > >         [...]
> > > > };
> > > >
> > > > What guarantees that this chunk doesn't have "auth enabled" and the
> > > > head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
> > > > obvious to a Linux SCTP expert, but at least for me as an outsider it
> > > > isn't - that's usually a good hint that there should be a code comment
> > > > explaining it.
> > > Hi Ondrej,
> > >
> > > shkey is for tx skbs only, while head_skb is for skbs on rx path.
> >
> > That makes sense, thanks. I would still be happier if this was
> > documented, but the comment would best fit in the struct sctp_chunk
> > definition and that wouldn't fit in this patch...
> >
> > Actually I have one more question - what about the
> > security_sctp_assoc_established() call in sctp_sf_do_5_1E_ca()? Is
> > COOKIE ACK guaranteed to be never bundled?
> COOKIE ACK could also be bundled with DATA.
> I didn't change it as it would not break SCTP.
> (security_inet_conn_established() returns void)
> But I don't mind changing it if you think it's necessary.

security_inet_conn_established? Are you looking at an old version of
the code, perhaps? In mainline, sctp_sf_do_5_1E_ca() now calls the new
security_sctp_assoc_established() hook, which may return an error. But
even if it didn't, I believe we want to make sure that an skb with
valid inet headers and XFRM state is passed to the hooks as SELinux
relies on these to correctly process the SCTP association.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

[Xin Long]

On Wed, Apr 6, 2022 at 11:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, Apr 6, 2022 at 4:21 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Apr 6, 2022 at 9:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Adding LSM and SELinux lists to CC for awareness; the original patch
> > > > > is available at:
> > > > > https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/T/
> > > > > https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@gmail.com/
> > > > >
> > > > > On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > >
> > > > > > Yi Chen reported an unexpected sctp connection abort, and it occurred when
> > > > > > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header
> > > > > > is included in chunk->head_skb instead of chunk->skb, it failed to check
> > > > > > IP header version in security_sctp_assoc_request().
> > > > > >
> > > > > > According to Ondrej, SELinux only looks at IP header (address and IPsec
> > > > > > options) and XFRM state data, and these are all included in head_skb for
> > > > > > SCTP HW GSO packets. So fix it by using head_skb when calling
> > > > > > security_sctp_assoc_request() in processing COOKIE_ECHO.
> > > > >
> > > > > The logic looks good to me, but I still have one unanswered concern.
> > > > > The head_skb member of struct sctp_chunk is defined inside a union:
> > > > >
> > > > > struct sctp_chunk {
> > > > >         [...]
> > > > >         union {
> > > > >                 /* In case of GSO packets, this will store the head one */
> > > > >                 struct sk_buff *head_skb;
> > > > >                 /* In case of auth enabled, this will point to the shkey */
> > > > >                 struct sctp_shared_key *shkey;
> > > > >         };
> > > > >         [...]
> > > > > };
> > > > >
> > > > > What guarantees that this chunk doesn't have "auth enabled" and the
> > > > > head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's
> > > > > obvious to a Linux SCTP expert, but at least for me as an outsider it
> > > > > isn't - that's usually a good hint that there should be a code comment
> > > > > explaining it.
> > > > Hi Ondrej,
> > > >
> > > > shkey is for tx skbs only, while head_skb is for skbs on rx path.
> > >
> > > That makes sense, thanks. I would still be happier if this was
> > > documented, but the comment would best fit in the struct sctp_chunk
> > > definition and that wouldn't fit in this patch...
> > >
> > > Actually I have one more question - what about the
> > > security_sctp_assoc_established() call in sctp_sf_do_5_1E_ca()? Is
> > > COOKIE ACK guaranteed to be never bundled?
> > COOKIE ACK could also be bundled with DATA.
> > I didn't change it as it would not break SCTP.
> > (security_inet_conn_established() returns void)
> > But I don't mind changing it if you think it's necessary.
>
> security_inet_conn_established? Are you looking at an old version of
> the code, perhaps? In mainline, sctp_sf_do_5_1E_ca() now calls the new
> security_sctp_assoc_established() hook, which may return an error. But
> even if it didn't, I believe we want to make sure that an skb with
> valid inet headers and XFRM state is passed to the hooks as SELinux
> relies on these to correctly process the SCTP association.
Sorry, I was looking at the old one.
OK, I will post v2 with the fix in sctp_sf_do_5_1E_ca().

Thanks for reviewing.

>
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>