Re: [PATCH] net/ipv6/netfilter/ip6t_NPT: rewrite addresses in ICMPv6 original packet

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi,

On Mon, Jul 20, 2020 at 11:17:01PM +1000, Michael Zhou wrote:
> Detect and rewrite a prefix embedded in an ICMPv6 original packet that was
> rewritten by a corresponding DNPT/SNPT rule so it will be recognised by
> the host that sent the original packet.

Thanks for submitting your patch, a few comments below.

> Example
> 
> Rules in effect on the 1:2:3:4::/64 + 5:6:7:8::/64 side router:
> * SNPT src-pfx 1:2:3:4::/64 dst-pfx 5:6:7:8::/64
> * DNPT src-pfx 5:6:7:8::/64 dst-pfx 1:2:3:4::/64
> 
> No rules on the 9:a:b:c::/64 side.
> 
> 1. 1:2:3:4::1 sends UDP packet to 9:a:b:c::1
> 2. Router applies SNPT changing src to 5:6:7:8::ffef::1
> 3. 9:a:b:c::1 receives packet with (src 5:6:7:8::ffef::1 dst 9:a:b:c::1)
> 	and replies with ICMPv6 port unreachable to 5:6:7:8::ffef::1,
> 	including original packet (src 5:6:7:8::ffef::1 dst 9:a:b:c::1)
> 4. Router forwards ICMPv6 packet with (src 9:a:b:c::1 dst 5:6:7:8::ffef::1)
> 	including original packet (src 5:6:7:8::ffef::1 dst 9:a:b:c::1)
> 	and applies DNPT changing dst to 1:2:3:4::1
> 5. 1:2:3:4::1 receives ICMPv6 packet with (src 9:a:b:c::1 dst 1:2:3:4::1)
> 	including original packet (src 5:6:7:8::ffef::1 dst 9:a:b:c::1).
> 	It doesn't recognise the original packet as the src doesn't
> 	match anything it originally sent
> 
> With this change, at step 4, DNPT will also rewrite the original packet
> src to 1:2:3:4::1, so at step 5, 1:2:3:4::1 will recognise the ICMPv6
> error and provide feedback to the application properly.
> 
> Conversely, SNPT will help when ICMPv6 errors are sent from the
> translated network.
> 
> 1. 9:a:b:c::1 sends UDP packet to 5:6:7:8::ffef::1
> 2. Router applies DNPT changing dst to 1:2:3:4::1
> 3. 1:2:3:4::1 receives packet with (src 9:a:b:c::1 dst 1:2:3:4::1)
> 	and replies with ICMPv6 port unreachable to 9:a:b:c::1
> 	including original packet (src 9:a:b:c::1 dst 1:2:3:4::1)
> 4. Router forwards ICMPv6 packet with (src 1:2:3:4::1 dst 9:a:b:c::1)
> 	including original packet (src 9:a:b:c::1 dst 1:2:3:4::1)
> 	and applies SNPT changing src to 5:6:7:8::ffef::1
> 5. 9:a:b:c::1 receives ICMPv6 packet with
> 	(src 5:6:7:8::ffef::1 dst 9:a:b:c::1) including
> 	original packet (src 9:a:b:c::1 dst 1:2:3:4::1).
> 	It doesn't recognise the original packet as the dst doesn't
> 	match anything it already sent
> 
> The change to SNPT means the ICMPv6 original packet dst will be
> rewritten to 5:6:7:8::ffef::1 in step 4, allowing the error to be
> properly recognised in step 5.
> 
> Signed-off-by: Michael Zhou <mzhou@cse.unsw.edu.au>
> ---
>  net/ipv6/netfilter/ip6t_NPT.c | 37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/net/ipv6/netfilter/ip6t_NPT.c b/net/ipv6/netfilter/ip6t_NPT.c
> index 9ee077bf4f49..b25e786607ed 100644
> --- a/net/ipv6/netfilter/ip6t_NPT.c
> +++ b/net/ipv6/netfilter/ip6t_NPT.c
> @@ -77,16 +77,42 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
>  	return true;
>  }
>  
> +static struct ipv6hdr *ip6t_npt_icmpv6_bounced_ipv6hdr(struct sk_buff *skb)
> +{
> +	if (ipv6_hdr(skb)->nexthdr != IPPROTO_ICMPV6)
> +		return NULL;
> +
> +	if (!icmpv6_is_err(icmp6_hdr(skb)->icmp6_type))
> +		return NULL;
> +
> +	if ((const unsigned char *)&icmp6_hdr(skb)[1] + sizeof(struct ipv6hdr) >
> +			skb_tail_pointer(skb))
> +		return NULL;
> +
> +	return (struct ipv6hdr *)&icmp6_hdr(skb)[1];

This ICMPv6 header might fall withing the non-linear data of the
skbuff.

BTW, does rfc6296 describes what to do with icmp traffic?