Re: [PATCH v3] icmp: support rfc5837

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, Mar 19, 2021 at 7:54 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 3/19/21 10:11 AM, Ishaan Gandhi wrote:
> > Thank you. Would it be better to do instead:
> >
> > +     if_index = skb->skb_iif;
> >
> > or
> >
> > +     if_index = ip_version == 4 ? inet_iif(skb) : skb->skb_iif;
> >
>
> If the packet comes in via an interface assigned to a VRF, skb_iif is
> most likely the VRF index which is not what you want.
>
> The general problem of relying on skb_iif was discussed on v1 and v2 of
> your patch. Returning an iif that is a VRF, as an example, leaks
> information about the networking configuration of the device which from
> a quick reading of the RFC is not the intention.
>
> Further, the Security Considerations section recommends controls on what
> information can be returned where you have added a single sysctl that
> determines if all information or none is returned. Further, it is not a
> a per-device control but a global one that applies to all net devices -
> though multiple entries per netdevice has a noticeable cost in memory at
> scale.
>
> In the end it seems to me the cost benefit is not there for a feature
> like this.

The sysctl was my suggestion. The detailed filtering suggested in the
RFC would add more complexity, not helping that cost benefit analysis.
I cared mostly about being able to disable this feature outright as it has
obvious risks.

But perhaps that is overly simplistic. The RFC suggests deciding trusted
recipients based on destination address. With a sysctl this feature can be
only deployed when all possible recipients are trusted, i.e., on an isolated
network. That is highly limiting.

Perhaps a per-netns trusted subnet prefix?

The root admin should always be able to override and disable this outright.