Date: Tue, 14 May 2024 13:29:18 +0200
From: Ondrej Mosnacek <omosnace@...hat.com>
To: Paul Moore <paul@...l-moore.com>
Cc: netdev@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH 2/2] cipso: make cipso_v4_skbuff_delattr() fully remove
 the CIPSO options

On Thu, Apr 25, 2024 at 11:48 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Wed, Apr 17, 2024 at 9:03 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> > On Tue, Apr 16, 2024 at 8:39 PM Paul Moore <paul@...l-moore.com> wrote:
> > > On Apr 16, 2024 Ondrej Mosnacek <omosnace@...hat.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.

> > > > Implement the proper option removal to make it consistent and producing
> > > > more optimal IP packets when there are CIPSO options set.
> > > >
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@...hat.com>
> > > > ---
> > > >  net/ipv4/cipso_ipv4.c | 89 ++++++++++++++++++++++++++++---------------
> > > >  1 file changed, 59 insertions(+), 30 deletions(-)
> > >
> > > Outside of the SELinux test suite, what testing have you done when you
> > > have a Linux box forwarding between a CIPSO network segment and an
> > > unlabeled segment?  I'm specifically interested in stream based protocols
> > > such as TCP.  Also, do the rest of the netfilter callbacks handle it okay
> > > if the skb changes size in one of the callbacks?  Granted it has been
> > > *years* since this code was written (decades?), but if I recall
> > > correctly, at the time it was a big no-no to change the skb size in a
> > > netfilter callback.
> >
> > I didn't test that, TBH. But all of cipso_v4_skbuff_setattr(),
> > calipso_skbuff_setattr(), and calipso_skbuff_delattr() already do
> > skb_push()/skb_pull(), so they would all be broken if that is (still?)
> > true. And this new cipso_v4_skbuff_delattr() doesn't do anything
> > w.r.t. the skb and the IP header that those wouldn't do already.
>
> Fair point on skbuff size changes in netfilter and
> cipso_v4_skbuff_setattr(), that wasn't part of the original
> NetLabel/CIPSO support and I forgot about that aspect.  On the other
> hand, I believe cipso_v4_skbuff_delattr() was part of the original
> work and used the NOOP hack both to preserve the packet length in the
> netfilter chain and to help ensure a consistent IP header overhead on
> both sides of a forwarding CIPSO<->unlabeled labeling/access control
> system.  Which brings me around to the reason why I asked about
> testing; I think we need to confirm that nothing bad happens to
> bidirectional stream-based connections, e.g. TCP, when crossing over a
> CIPSO/unlabeled network boundary and the IP overhead changes.  It's
> probably okay, but I would like to see that you've tested it with a
> couple different client OSes and everything works as expected.

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?

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

Powered by blists - more mailing lists