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

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

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

From: Frank Kardel <kardel%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/52074: -current npf map directive broken
Date: Sun, 07 May 2017 11:51:12 +0200

 Debugging this further the analysis is:
 
 Given a map:
 map <some if> dynamic 127.0.0.1 port 25 <- 10.200.1.2 port 25 
 (10.200.1.2 is not a local interface address)
 
 Following happens:
 1) packet arrives on interface (a.b.c.d:x -> 10.200.1.2:25
 2) NAT assoc gets creates
 3) packet gets NATed (=> a.b.c.d:x -> 127.0.0.1:25)
 4) packet gets entered in syn_cache
 5) syn_cache attempts to send SYN,ACK response (127.0.0.1:25 -> 
 a.b.c.d:x) via ip_output()
 6) ip_output calls pfil_hooks (->NPF)
 7) NPF find nat assoc
 8) SYN,ACK packet gets reverse NATed (10.200.1.2:25 -> a.b.c.d:x)
 9) ip_output tries for find the interface adress (ia) for 10.200.1.2 and 
 fails
 10) 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
 
 In ip_ouput ia is only used for interface transfer statistics and a 
 sanity check after pfil_run_hooks().
 ia will/must be NULL when non local source addresses are in the packet.
 
 The condition in 620 currently is (see if()):
      ia = in_get_ia_psref(ip->ip_src, &psref_ia);
 
      /* Ensure we only send from a valid address. */==>
      if ((ia != NULL || (flags & IP_FORWARDING) == 0) && <<<<!
          (error = ip_ifaddrvalid(ia)) != 0)
      {
          ARPLOG(LOG_ERR,
              "refusing to send from invalid address %s (pid %d)\n",
              ARPLOGADDR(ip->ip_src), curproc->p_pid);
          IP_STATINC(IP_STAT_ODROPPED);
          if (error == 1)
              /*
               * Address exists, but is tentative or detached.
               * We can't send from it because it's invalid,
               * so we drop the packet.
               */
              error = 0;
          else
              error = EADDRNOTAVAIL;
          goto bad;
      }
 
 Proposed fix is to replace the || inf the if with && as this also seems 
 to have been the original intention by the author.