Re: kern/52074: -current npf map directive broken

[David Holland <dholland-bugs%netbsd.org@localhost>]

The following reply was made to PR kern/52074; it has been noted by GNATS.

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/52074: -current npf map directive broken
Date: Tue, 16 May 2017 06:12:16 +0000

 Not sent to gnats.
 
    ------
 
 From: Frank Kardel <kardel%netbsd.org@localhost>
 To: Roy Marples <roy%marples.name@localhost>, Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>
 CC: netbsd-bugs%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, Christos Zoulas
 	<christos%NetBSD.org@localhost>
 Subject: Re: kern/52074: -current npf map directive broken
 Date: Sun, 07 May 2017 23:07:42 +0200
 
 Hi Roy !
 
 Hmm, wouldn't this bring us back the bug again? ia == NULL for a non-local
 source addresses (generated via pfil_run_hooks-NAT operation) and
 IP_FORWARDING is not set as tcp_input.c:syn_cache_respond does rightfully not
 set IP_FORWARDING and pfil_run_hooks has no means to set that flag. That gives
 us error == -1 with your sequence.
 So we would return EADDRNOTAVAIL breaking packet filter NAT action again, if I
 didn't overlook something.
 
 From what I understand  this code originally attempted to avoid sending from
 invalid/unusable local address (e. g. duplicate IP - error, tentative and
 detached should just be dropped).
 No validation can be done for non-local addresses at all. IP_FORWARDING
 formerly used to be used to suppress infinite recursion on mcast forwarding,
 but it seems the semantics where extended a little bit in the mean time (like
 here to suppress a check).
 So I cannot say something about the intentions for the IP_FORWARDING check.
 
 For correct packet filter operation we need to distinguish the local and
 non-local address case here. If we keep the previous logic for IP_FORWARDING
 and ip_ifaddrvalid() then
 the new expression
     if (ia != NULL && (flags & IP_FORWARDING) == 0 &&
         (error = ip_ifaddrvalid(ia)) != 0)
 correctly limits the check to local interface addresses only keeping the
 original intention/implementation for IP_FORWARDING and validity checks.
 
 If then IP_FORWARDING and validity check needs to be adjusted that part must
 be limited to local interface addresses only. (ia != NULL).
 
 That's how I understand that code section or the intention there in the normal
 and packet filter context.
 
 I didn't check IPv6.
 
 Frank