Message-ID: <CADvbK_cR5paEunENmWd62XfXtMSf+MHhhc-S1z_gLWp_dUx=8w@mail.gmail.com>
Date:   Mon, 16 Jan 2023 12:36:07 -0500
From:   Xin Long <lucien.xin@...il.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     network dev <netdev@...r.kernel.org>, davem@...emloft.net,
        kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...il.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Pravin B Shelar <pshelar@....org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Ilya Maximets <i.maximets@....org>,
        Aaron Conole <aconole@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Guillaume Nault <gnault@...hat.com>,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@...l-moore.com> wrote:
>
> On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@...il.com> wrote:
> > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@...l-moore.com> wrote:
> > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@...il.com> wrote:
> > > >
> > > > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > > > the iph->tot_len update should use iph_set_totlen().
> > > >
> > > > Note that for these non GSO packets, the new iph tot_len with extra
> > > > iph option len added may become greater than 65535, the old process
> > > > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > > > iph options shouldn't be added for these big packets in here, a fix
> > > > may be needed here in the future. For now this patch is only to set
> > > > iph->tot_len to 0 when it happens.
> > >
> > > I'm not entirely clear on the paragraph above, but we do need to be
> > > able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> > > order to support CIPSO labeling.  I'm open to better and/or
> > > alternative solutions compared to what we are doing now, but I can't
> > > support a change that is a bug waiting to bite us.  My apologies if
> > > I'm interpreting your comments incorrectly and that isn't the case
> > > here.
> > setting the IP options may cause the packet size to grow (both iph->tot_len
> > and skb->len), for example:
> >
> > before setting it, iph->tot_len=65535,
> > after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
> > however, tot_len is 16 bit, and can't be set to "65535 + 14".
> >
> > Hope the above makes it clearer.
>
> Thanks, it does.
>
> > This problem exists with or without this patch. Not sure how it should
> > be fixed in cipso_v4 ...
> >
> > Not sure if we can skip the big packet, or segment/fragment the big packet
> > in cipso_v4_skbuff_setattr().
>
> We can't skip the CIPSO labeling as that would be the network packet
> equivalent of not assigning a owner/group/mode to a file on the
> filesystem, which is a Very Bad Thing :)
>
> I spent a little bit of time this morning looking at the problem and I
> think the right approach is two-fold: first introduce a simple check
> in cipso_v4_skbuff_setattr() which returns -E2BIG if the packet length
> grows beyond 65535.  It's rather crude, but it's a tiny patch and
> should at least ensure that the upper layers (NetLabel and SELinux)
> don't send the packet with a bogus length field; it will result in
> packet drops, but honestly that seems preferable to a mangled packet
> which will likely be dropped at some point in the network anyway.
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 6cd3b6c559f0..f19c9beda745 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -2183,8 +2183,10 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
>         * that the security label is applied to the packet - we do the same
>         * thing when using the socket options and it hasn't caused a problem,
>         * if we need to we can always revisit this choice later */
> -
>        len_delta = opt_len - opt->optlen;
> +       if ((skb->len + len_delta) > 65535)
> +               return -E2BIG;
> +
Right, looks crude. :-)
Note that for BIG TCP packets skb->len is bigger than 65535.
(I assume CIPSO labeling will drop BIG TCP packets.)

>        /* if we don't ensure enough headroom we could panic on the skb_push()
>         * call below so make sure we have enough, we are also "mangling" the
>         * packet so we should probably do a copy-on-write call anyway */
>
> The second step will be to add a max-length IPv4 option filled with
> IPOPT_NOOP to the local sockets in the case of
> netlbl_sock_setattr()/NETLBL_NLTYPE_ADDRSELECT.  In this case we would
> either end up replacing the padding with a proper CIPSO option or
> removing it completely in netlbl_skbuff_setattr(); in either case the
> total packet length remains the same or decreases so we should be
> "safe".
sounds better.

>
> The forwarded packet case is still a bit of an issue, but I think the
> likelihood of someone using 64k max-size IPv4 packets on the wire
> *and* passing this traffic through a Linux cross-domain router with
> CIPSO (re)labeling is about as close to zero as one can possibly get.
> At least given the size check present in step one (patchlet above) the
> packet will be safely (?) dropped on the Linux system so an admin will
> have some idea where to start looking.
don't know the likelihood of CIPSO (re)labeling on a Linux cross-domain router.
But 64K packets could be GRO packets merged on the interface (GRO enabled)
of the router, not directly from the wire.

>
> I've got some basic patches written for both, but I need to at least
> give them a quick sanity test before posting.
>
Good that you can take care of it from the security side.

I think a similar problem exists in calipso_skbuff_setattr() in
net/ipv6/calipso.c.

Thanks.

Powered by blists - more mailing lists