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

[Frank Kardel <kardel%netbsd.org@localhost>]

On 05/08/17 11:04, Roy Marples wrote:
> Hi Frank!
>
> On 07/05/2017 22:07, Frank Kardel wrote:
> > 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.
> Ah yes.
> I didn't actually read your initial report .... my bad!
No worries - I am just trying to keep us from running too far in the 
wrong direction.
> OK, so I don't fully understand the use case for sending packets from an
> address we don't have locally. Could you fill me in on this please?
The mapping is as follows

Machine A (Client)        Machine B (Gateway/NAT/redirect box)      
Machine C (target)
a.b.c.d:x        ->       W.X.Y.Z           -> 10.200.1.2 (will never 
see a packet from a.b.c.dx)
                          map 127.0.0.1 25 <- 10.200.1.2 25
                          redirects to 127.0.0.1 25 (local)
                          A must see the association a.b.c.d:x <-> 
10.200.1.2:25

                          Thus local postfix will see a.b.c.d x <-> 
127.0.0.1 25

                          So dest addr 10.200.1.2 will be replaced 
incoming with
                          127.0.0.1 25 in pf_fil_run_hooks in ip_input 
and on the way back
                          the src addr of 127.0.0.1 is replaced with 
10.200.1.2 (not local)
                          in pfil_run_hooks in ip_output.

To quote the condensed analysis from the bug + where in the stack it 
happens:

 Following happens:
 1) ip_input: packet arrives on interface (a.b.c.d:x -> 10.200.1.2:25
 1a) ip_input call pfil_run_hooks -> NPF
 2) NPF: NAT assoc gets created
 3) NPF: packet gets NATed (=> a.b.c.d:x -> 127.0.0.1:25)
 3a) NPF returns
 3b) ip_input hands over to tcp_input
 4) tcp_input: packet gets entered in syn_cache
 5) tcp_input: syn_cache attempts to send SYN,ACK response (127.0.0.1:25 ->
 a.b.c.d:x) via ip_output()
 6) ip_output: ip_output calls pfil_hooks (->NPF)
 7) NPF: find nat assoc
 8) NPF: SYN,ACK packet gets reverse NATed (10.200.1.2:25 -> a.b.c.d:x)
 8a) NPF returns
 9) ip_output: ip_output tries for find the interface adress (ia) for 10.200.1.2 and
 fails (ia == NULL)
 10) ip_output: ip_output fumbles badly at ip_output:1.276:620 and declares the
 packet coming from an invalid address(ip_ifaddrvalid(NULL))
 11) confusion commences from here on

> >  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.
> I *think* the idea was IP_FORWARDING would be set and we could skip
> source address validation because the filter may have changed it.
> https://nxr.netbsd.org/xref/src/sys/net/npf/npf_sendpkt.c#178
That's what I thought was the intention. As you see above the 
transmitted SYN,ACK is
not created by NPF and not by npf_sendpkt.c#178. NPF and other filters 
do an
in place header modification and the flags variable is not part of 
pfil_run_hooks(). So
the IP_FORWARDING logic cannot be used by the packet filters AFAICS.
> Does NAT not hit that?
Nope - see above - coming directly from 
tcp_input:syn_cache_add:syn_cache_respond. IP_FORWARD is not set there.
Before the packet filter we still have a valid local address, Maybe we 
should move the check there - IPv6 also does the check before IIANM.
>   I ask because I do run NPF+NAT on my erlite
> router which uses -current, but my mapping rule uses a local address -
> hence asking for your use case about a non local address.
>
> Could NAT set another flag we could check? rmind?
OTOMHI see
    1) extend pfil_run_hooks interface to pass a pointer to flags- 
relatively cheap, somewhat dirty
    2) add a tag the the mbuf -  more expensive b/c we also need to add 
checks

Would we gain enough speed/functionality/abstraction to warrant an 
interface change?
Does the condition "before packet filter we have either IP_FORWARD set 
or a local address as source" always hold?
If so, we could just move the check and set up ia correctly for 
IFASTATS.IPv6 checks before packet filter runs.

I have not analyzed the invariants enough to make a quick recommendation 
here.

I hope I explained the setup good enough to describe the issue with NAT 
redirection.

> Roy
Frank