[Click] AggregateIPFlows NAT Issue . . .

[John Russell Lane]

I've run into a minor issue with AggregateIPFlows and NAT where the
outbound host is the local machine (i.e., where the source IP ==
destination IP).

The problem is that AggregateIPFlows does not properly tag a "reverse"
flow when the source and destination are the same IP address.  That
is, both the "forward" and "reverse" flow end up being tagged as new
("forward").  The reason for this is that the key formed to identify
both "forward" and "reverse" endpoint flows is essentially something
like:

  (SRC_IP < DST_IP) ? <SRC_IP, DST_IP> : <DST_IP, SRC_IP>

When a "reply" packet comes in, this forms an ordering over the IP
address pairs.  Ports are then "flipped" to match the given order,
stored in another structure and later used to identify an individual
flow between the endpoints.  However, when SRC_IP == DST_IP, this
"tie" is not handled, the port flipping (which had relied solely on
the IP address ordering) is not done and the resulting ports used to
lookup the flow differ for the inbound and outbound packets.

Though it is a bit of a hack, the following patch resolves this issue
in AggregateIPFlows by simply handling SRC_IP == DST_IP as a special
case, examining the given ports in that case and indicating the flip
should be performed when SRC_PRT > DST_PRT.  This should ensure that a
first packet and return packet share the same (port-based)
flow-identifying key on inbound and outbound packets from the same
host.

However, in confirming that this patch works (with 1.5/1.6 via git), I
also had to add a workaround for IPRewriter.  The problem there is
that TCP checksums are not computed correctly under the above
conditions (SRC_IP == DST_IP).  Adding SetIPChecksum / SetTCPChecksum
manually (downstream from the rewrite element) fixes this; so it seems
the same problem lies somewhere within the map used in IPRewriter.
I'm pressed for time at the moment and this got me moving again; if no
one cares to, I'll hopefully have a look at a better fix in a few
weeks.  :-)

If I've missed something, please let me know.  Thanks!

jrl.

---Begin Patch---
diff -ru src/elements/analysis/aggregateipflows.cc
src.nat_fix/elements/analysis/aggregateipflows.cc
--- src/elements/analysis/aggregateipflows.cc   2007-12-07
11:16:12.000000000 +0900
+++ src.nat_fix/elements/analysis/aggregateipflows.cc   2007-11-23
04:20:18.000000000 +0900
@@ -529,6 +529,17 @@
        return handle_fragment(p, paint, m, hpinfo);

     uint32_t ports = *(reinterpret_cast<const uint32_t *>(udp_ptr));
+    if (hosts.a == hosts.b) {
+      const uint16_t * sport = reinterpret_cast<const uint16_t *>(udp_ptr),
+                     * dport = reinterpret_cast<const uint16_t
*>(udp_ptr+sizeof(uint16_t));
+      // This causes a flip below in order to form a source-destination
+      // invariant ordering over address/port pairs even when the host pair
+      // is equal.  Without this, one flow ends up being created for the
+      // forward packet and another is created for reply packets because
+      // the source-destination port ordering is only dependent upon the IPs
+      // ("ties" are not handled).
+      if (*sport > *dport) paint ^= 1;
+    }
     if (paint & 1)
        ports = flip_ports(ports);
     FlowInfo *finfo = find_flow_info(m, hpinfo, ports, paint & 1, p);