Re: [PATCH] net: Add netfilter hooks to track SRv6-encapsulated flows

[Andrea Mayer <andrea.mayer@uniroma2.it>]

Dear Pablo,
thanks for your time.

On Thu, 8 Jul 2021 15:38:59 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> Dear Andrea,
> 
> On Thu, Jul 08, 2021 at 03:31:38AM +0200, Andrea Mayer wrote:
> > Dear Ryoga,
> > looking at your patch I noted several issues.
> > I start from the decap part but the same critical aspects are present also in
> > the encap one.
> > 
> > On Tue, 6 Jul 2021 14:25:48 +0900
> > Ryoga Saito <proelbtn@gmail.com> wrote:
> > > [...]
> > >
> > >
> > > +static int seg6_local_input(struct sk_buff *skb)
> > > +{
> > > +    if (skb->protocol != htons(ETH_P_IPV6)) {
> > > +        kfree_skb(skb);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_IN, dev_net(skb->dev), NULL,
> > > +               skb, skb->dev, NULL, seg6_local_input_core);
> > > +}
> > > +
> > 
> > The main problem here concerns the use that has been made of the netfilter
> > HOOKs combined with the SRv6 processing logic.
> > 
> > In seg6_local_input, it is not absolutely guaranteed that the packet is
> > intended to be processed and delivered locally. In fact, depending on the given
> > configuration and behavior, the packet can either be i) processed and delivered
> > locally or ii) processed and forwarded to another node.
> 
> What SRv6 decides to do with the packet is irrelevant, see below.
> 
> > In seg6_local_input, depending on the given configuration and behavior, the
> > packet can either be i) processed and delivered locally or ii) processed and
> > forwarded to another node. On the other hand, your code assumes that the packet
> > is intended to be processed and delivered locally.
> >
> > Calling the nfhook NFPROTO_IPV6 with NF_INET_LOCAL_IN can have several side
> > effects.
> 
> This is how UDP tunnel encap_rcv infrastructure works: the packet
> follows the INPUT path. The encap_rcv() might decide to reinject the
> decapsulated packet to stack or forward it somewhere else.
> 

the problem is that in SRv6 a packet to be processed by a node with an 
SRv6 End Behavior does not follow the INPUT path and its processing is 
different from the UDP tunnel example that you have in mind (more info 
below)

Note that I'm referring to the SRv6 Behaviors as implemented in seg6_local.c

> > I'll show you one below with an example:
> > 
> > suppose you have a transit SRv6 node (which we call T) configured with an SRv6
> > Behavior End (in other words, node T receives SRv6 traffic to be processed by
> > SRv6 End and forwarded to another node). Such node T is configured with
> > firewall rules on the INPUT CHAIN that prevent it from receiving traffic that
> > was *NOT* generated by the node itself (speaking of conntrack...). This
> > configuration can be enforced either through an explicit rule (i.e. XXX -j
> > DROP) or by setting the default INPUT CHAIN policy to DROP (as it would be done
> > in a traditional firewall configuration).
> > 
> > In this patch, what happens is that when an SRv6 packet passes through the
> > node, the call to the nfhook with NF_INET_LOCAL_IN triggers the call to the
> > firewall and the DROP policy on INPUT kicks in. As a result, the packet is
> > discarded. What makes the situation even worse is that using the nfhook in this
> > way breaks the SRv6 Behavior counter system (making that totally unusable).
> 
> By default there are no registered hooks, ie. no filtering policy in
> place, the user needs to explicity specify a filtering policy, the
> mechanism is not breaking anything, the user policy needs to be
> consistent, that's all.
> 

An example of consistent user policies in node T that can be installed
now and are broken by the patch is the following:
 
   ip6tables -A INPUT -i lo -j ACCEPT

   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type neighbor-solicitation -j ACCEPT                 
   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type neighbor-advertisement -j ACCEPT
                                                              
   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type router-solicitation -j ACCEPT
   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type router-advertisement -j ACCEPT

   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type echo-request -j ACCEPT
   ip6tables -A INPUT \
          -p icmpv6 --icmpv6-type echo-reply -j ACCEPT

   ip6tables -P INPUT DROP  

The IPv6 destination address fc01::2 (SRv6 SID) is configured as an SRv6 End
Behavior in node T. On node T we expect to receive an IPv6+SRH packet with
DA equals to fc01::2.
Please note that the fc01::2 is NOT a local address for the T node.

In node T only the following protocols are allowed in INPUT:

  icmpv6 RA, neigh adv/sol and echo request/reply

while all other protocols in INPUT are discarded.

with this consistent configuration, node T is able to correctly process and
forward packets with IPv6+SRH destination address fc01::2 because the INPUT
path is not taken when SRv6 implementation is processing an SRv6 End Behavior.

After the introduction of the patch, this correct behavior is broken.

If you want I can provide a diagram with the full description of this 
scenario and all the scripts to reproduce the issue.

Andrea