netdev - Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Apr 2022 17:04:25 +0200
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        "linux-sctp @ vger . kernel . org" <linux-sctp@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>
Subject: Re: [PATCH net] sctp: use the correct skb for security_sctp_assoc_request

On Wed, Apr 6, 2022 at 4:21 PM Xin Long <lucien.xin@...il.com> wrote:
> On Wed, Apr 6, 2022 at 9:34 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> >
> > On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@...il.com> wrote:
> > > On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@...hat.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@...il.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.