Message-ID: <CAHC9VhQeaOeX-5SENhpScKN9kF1rAKoZX23KOUqQ5=uz6v92iA@mail.gmail.com>
Date:   Mon, 16 Jan 2023 11:45:54 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Xin Long <lucien.xin@...il.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 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;
+
       /* 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".

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.

I've got some basic patches written for both, but I need to at least
give them a quick sanity test before posting.

--
paul-moore.com

Powered by blists - more mailing lists