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

[Ryoga Saito <proelbtn@gmail.com>]

Dear Andrea
Thanks for your reviews and comments.


> On Jul 9, 2021, at 1:32, Andrea Mayer <andrea.mayer@uniroma2.it> wrote:
> 
> 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 thought SRv6 processing to be the same as encapsulation/decapsulation
processing and I implemented patch this way intentionally.

>>> 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.

I considered srv6 Behaviors (e.g. T.Encaps) to be the same as the encapsulation
in other tunneling protocols, and srv6local Behaviors (e.g. End, End.DT4,
End.DT6, ...) to be the same as the decapsulation in other tunneling protocols
even if decapsulation isn't happened. I'm intended that SRv6 packets whose
destination address is SRv6 End Behavior go through INPUT path.

I think it works correctly on your situation with the following rule:

ip6tables -A INPUT --dst fc01::2 -j ACCEPT

To say more generally, SRv6 locator blocks should be allowed with ip6tables if
you want to change default policy of INPUT chain to DROP/REJECT.
 
Ryoga