Broken Cisco NetFlow Decode about tcpdump HOT 8 CLOSED

Submitted by guy_harris

Logged In: YES
user_id=541179

What happens with V6 traces if you remove that field? If
that field is in V6, wouldn't that cause every record after
the first, in a V6 trace, not to print correctly?

If so (as I suspect is the case), the solution is NOT to
remove that field, it's to have the loop variable be a
"const u_char *", advance it by the appropriate size based
on the NetFlow version, and cast it to "const nfrec *" and
use that pointer.

from tcpdump.

Submitted by danuvius

Logged In: YES
user_id=631506
Originator: NO

This would indeed break the code for v6. Also this part is currently only working for netflow v6 traces. I added the following line to the end of the line:
if (ver == 5) nr = (struct nfrec *) ((struct in_addr *)nr - 1);

With this "fix" now traces of netvlow v5 and v6 can be analyzed. (Only tested on netflow v6 data, I assume that it did work allready for version 6). Furthermore, it can't beused to analyze netflow v1/v8 an v9 (they have completely different headers).

from tcpdump.

Well, looks okay to me. I can test it if anyone can get me few v5/v6 PCAPs.
I have a bunch of private v9 PCAPs, but like Guy mentioned their headers are nowhere close to that of v5/v6.

from tcpdump.

Requested on tcpdump-workers.

from tcpdump.

@hdnivara I have got a capture here, but it's not really intended to become public, although it's fine if you use it for some tests. Could I send it to you over some private channel?

from tcpdump.

@qnet-herwin sent his private PCAP to me and tcpdump fails to decode anything after the 1st record, as expected.

Also, there is another bug in the existing code: it doesn't go over all the records.
It fails in the for loop condition at https://github.com/the-tcpdump-group/tcpdump/blob/master/print-cnfp.c#L121 when nr is checked for end of packet.

I couldn't understand why we have (nr + 1) <= ndo->ndo_snapend, which seems to be redundant as we are checking for nrecs-- anyway.

So, I applied the following changes and now it decodes all the v5 records in each packet properly. I'll send in the patch shortly.

$ git diff
diff --git a/print-cnfp.c b/print-cnfp.c
index 07c940c..21d39be 100644
--- a/print-cnfp.c
+++ b/print-cnfp.c
@@ -118,7 +118,7 @@ cnfp_print(netdissect_options *ndo,

        ND_PRINT((ndo, "%2u recs", nrecs));

-       for (; nrecs-- && (const u_char *)(nr + 1) <= ndo->ndo_snapend; nr++) {
+       for (; nrecs--; nr++) {
                char buf[20];
                char asbuf[20];

@@ -183,5 +183,10 @@ cnfp_print(netdissect_options *ndo,
                       EXTRACT_32BITS(&nr->proto_tos) & 0xff,
                       EXTRACT_32BITS(&nr->packets),
                       EXTRACT_32BITS(&nr->octets), buf));
+
+                /* Exclude nh peer data for v5 as it doesn't apply for v5. */
+                if (5 == ver)
+                        nr = (struct nfrec *) ((struct in_addr *) nr - 1);
+
        }
 }

from tcpdump.

Redone according to

http://www.cisco.com/c/en/us/td/docs/net_mgmt/netflow_collection_engine/3-6/user/guide/format.html#wp1005892

with separate structures and separate dissection routines for v1, v5, and v6. Those routines all use ND_TCHECK() to do bounds checking, so they'll report "[|cnfp]" if they go past the snaplen.

Try using the current top-of-trunk code (from bpf.tcpdump.org; I think the fix will get propagated to the github repository within a day or so).

from tcpdump.

For history: these were commits 9ed7ddb and 7af5959. The issue looks fully resolved, closing.

from tcpdump.