[PATCH 2/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Paul Moore]

On Tue, May 14, 2024 at 7:29 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> On Thu, Apr 25, 2024 at 11:48 PM Paul Moore <paul at paul-moore.com> wrote:
> >
> > On Wed, Apr 17, 2024 at 9:03 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > > On Tue, Apr 16, 2024 at 8:39 PM Paul Moore <paul at paul-moore.com> wrote:
> > > > On Apr 16, 2024 Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > > > >
> > > > > As the comment in this function says, the code currently just clears the
> > > > > CIPSO part with IPOPT_NOP, rather than removing it completely and
> > > > > trimming the packet. This is inconsistent with the other
> > > > > cipso_v4_*_delattr() functions and with CALIPSO (IPv6).
> > > >
> > > > This sentence above implies an equality in handling between those three
> > > > cases that doesn't exist.  IPv6 has a radically different approach to
> > > > IP options, comparisions between the two aren't really valid.
> > >
> > > I don't think it's that radically different.
> >
> > They are very different in my mind.  The IPv4 vs IPv6 option format
> > and handling should be fairly obvious and I'm sure there are plenty of
> > things written that describe the differences and motivations in
> > excruciating detail so I'm not going to bother trying to do that here;
> > as usual, Google is your friend.  I will admit that the skbuff vs
> > socket-based labeling differences are a bit more subtle, but I believe
> > if you look at how the packets are labeled in the two approaches as
> > well as how they are managed and hooked into the LSMs you will start
> > to get a better idea.  If that doesn't convince you that these three
> > cases are significantly different, I'm not sure what else I can say
> > other than we have a difference of opinion.  Regardless, I stand by my
> > original comment that I don't like the text you chose and would like
> > you to remove or change it.
>
> Ok, I amended this part for v2 to hopefully better express what I'm
> alluding to. I also added a paragraph about the routers dropping
> packets with IP options, which explains the motivation better, anyway.

Okay, I'll refrain from further comment until I see the v2 patch.

> I tried to test what you describe - hopefully I got close enough:
>
> My test setup has 3 VMs (running Fedora 39 from the stock qcow2 image)
> A, B, and R, connected via separate links as A <--> R <--> B, where R
> acts as a router between A and B (net.ipv4.ip_forward is set to 1 on
> R, and the appropriate routes are set on A, B, R).
>
> The A <--> R link has subnet 10.123.123.0/24, A having address
> 10.123.123.2 and R having 10.123.123.1.
> The B <--> R link has subnet 10.123.124.0/24, B having address
> 10.123.124.2 and R having 10.123.124.1.
>
> The links are implemented as GRE tunnels over the main network that is
> shared between the VMs.
>
> Netlabel configuration on A:
> netlabelctl cipsov4 add pass doi:16 tags:5
> netlabelctl map del default
> netlabelctl map add default address:0.0.0.0/0 protocol:unlbl
> netlabelctl map add default address:::/0 protocol:unlbl
> netlabelctl map add default address:10.123.123.0/24 protocol:cipsov4,16
> netlabelctl map add default address:10.123.124.0/24 protocol:cipsov4,16
>
> Netlabel configuration on R:
> netlabelctl cipsov4 add pass doi:16 tags:5
> netlabelctl map del default
> netlabelctl map add default address:0.0.0.0/0 protocol:unlbl
> netlabelctl map add default address:::/0 protocol:unlbl
> netlabelctl map add default address:10.123.123.0/24 protocol:cipsov4,16
>
> B has no netlabel configured.
>
> (I.e. A tries to send CIPSO-labeled packets to B, but R treats the
> 10.123.124.0/24 network as unlabeled, so should strip/add the CIPSO
> label when forwarding between A and B.)
>
> A basic TCP connection worked just fine in both directions with and
> without these patches applied (I installed the patched kernel on all
> machines, though it should only matter on machine R). I ignored the
> actual labels/CIPSO content - i.e. I didn't change the default SELinux
> policy and put SELinux into permissive mode on machines A and R.
>
> Capturing the packets on R showed the following IP option content
> without the patches:
> A -> R: CIPSO
> R -> B: NOPs
> B -> R: (empty)
> R -> A: CIPSO
>
> With the patches this changed to:
> A -> R: CIPSO
> R -> B: (empty)
> B -> R: (empty)
> R -> A: CIPSO
>
> Is this convincing enough or do you have different scenarios in mind?

Thanks for verifying your patch, the methodology looks good to me, but
as I mentioned in my previous email I would feel much better if you
verified this with a different client OS/stack.  Do you have access to
Windows/MacOS/BSD/non-Linux system you could use in place of B in your
test above?

-- 
paul-moore.com