phaag / nfdump

Netflow processing tools
Other
770 stars 202 forks source link

Incorrect IPFIX/NF9 element length being used for "absent" elements in some cases #76

Closed gauravagarwal closed 6 years ago

gauravagarwal commented 7 years ago

Hi Peter, I think I've hit an issue where an incorrect ipfix/v9 element is getting picked up under following scenario: "netflow v9 records that contain NF9_INPUT_SNMP field but not NF9_OUTPUT_SNMP"

In this scenario, the PushSequence corresponding to the first entry for NF9_OUTPUT_SNMP element will be done whereas the NF9_INPUT_SNMP could have been for 4 bytes (thus filling extension with 4 bytes for INPUT_SNMP and 2 bytes for OUTPUT_SNMP). Since both these map to same extension, and there is no length check done for "not-present" fields when building "cache" in netflow_v9.c/ipfix.c, this issue is being encountered.

Please let me know if I am not clear enough - I will provide a more detailed description. (I am on version 1.6.13 but on quick check it appears that same issue would be present in 1.6.15 as well)

phaag commented 6 years ago

Hi, I see what you mean, but the function PushSequence already should handle this correctly. If you have a condition, where this does not work, let me know. Please have me a pcap file of the netflow export to test that.

gauravagarwal commented 6 years ago

Sure - I'll provide you with the file in a day or so. We were hitting this on a real test and thus I discovered it.

gauravagarwal commented 6 years ago

Hi Peter, it seems that this problem is not present in master any more. I am attaching the pcap file and the corresponding nfcapd files using master and using version nfdump-1.6.13 (which I had earlier downloaded from sourceforge).

Command: nfcapd -l /tmp -T 1,-2,3,13,16,18,19,20,21,22,23 -f netflow_4500.pcap

I will have to compare source to see how you fixed it and revert or hack around it :-) Or if you could point me to the relevant fixes, that would be great.

recs.zip

gauravagarwal commented 6 years ago

Ithink this is the relevant code snippet that handles this case:


    if ( cache.lookup_info[Type].found ) {
            table->sequence[i].id = v9_element_map[index].sequence;
            table->sequence[i].input_offset  = cache.lookup_info[Type].offset;
            table->sequence[i].output_offset = *offset;
            table->sequence[i].stack = stack;
            dbg_printf("Fill ");
    } else {
            // in case only on half of an extension is sent from the collector, make
            // sure the right zero sequence is taken, where two length types exists
            index += pair_offset;
            table->sequence[i].id = v9_element_map[index].zero_sequence;
            table->sequence[i].input_offset  = 0;
            table->sequence[i].output_offset = *offset;
            table->sequence[i].stack = NULL;
            dbg_printf("Zero ");
    }```

Apologies for false alarm.