CESNET / ipfixcol

IPFIXcol is an implementation of an IPFIX (RFC 7011) collector
Other
64 stars 37 forks source link

NetFlow v9 input: enterprise-specific fields not parsed correctly #16

Closed ghost closed 9 years ago

ghost commented 9 years ago

We currently have a trace (basically the same one as for issue #14) that contains NetFlow v9 datagrams. Some fields are non-standard (in this case, defined by ntop), and cause the FastBit storage plugin to go crazy:

DEBUG: fastbit storage: Received new template: 258
ERROR: fastbit storage: Wrong element size (e-516227040id24884 - 0)!
ERROR: fastbit storage: Wrong element size (e0id260 - 0)!
ERROR: fastbit storage: Wrong element size (e-516227040id24884 - 0)!
ERROR: fastbit storage: Wrong element size (e0id260 - 0)!

As you can see, this results in integer overflows and also the e0id260 is a field that is not present in the template/data record. There are actually two fields that have an ID > 256:

I've prepared a short trace for you that triggers the behavior. Please contact me at kirc&secdorks.net to get the trace.

ghost commented 9 years ago

The code that transforms NetFlow v9 messages into IPFIX messages lacks any knowledge about enterprise-specific fields. This is what happens:

Of course, there's the general question on how to encode enterprise-specific fields in NetFlow v9 using IPFIX, i.e., which PEN and field ID to use. I see two options:

The first option is the most accurate one, obviously. In case you decided to take that approach, I'm happy to help contributing to the list.

thorgrin commented 9 years ago

You are definitely right in pointing out that the code is broken for IDs larger than 32767. However, I do not think that we can safely add PEN to such IDs. The problem is that if the company that uses for example ID 50000 is also using ID 17232, then just adding enterprise number for this company will result in a element mismatch.

The mapping that you suggest would have to take this issue into account a correctly map company specific nf9 IDs to their IPFIX IDs. However, we also need to deal with unknown elements from different companies. Therefore, I would suggest to dedicate a special PEN for nf9 compatibility. Do you think that such a solution is acceptable?

ghost commented 9 years ago

Yes. What do you think of the following approach:

This approach should never yield collisions. Do you agree?

thorgrin commented 9 years ago

I've consulted the INVEA-TECH and they do not use such NetFlow v9 elements. Therefore, I think that the problem arises with ntop probe, correct?

I think the the first step is to add the special PEN and until a better one is found, the 'all ones' sounds good to me. Just a minor correction, the enteprise bit must remain set to 1, since then the PEN would not be expected.

Then, we can write an intermediate plugin that can do all the element mapping we need. This would leave the IPFIXcol code clean of any vendor-specific conditions. Does that sound OK?

ghost commented 9 years ago

Correct. In my case, I discovered the issue(s) with nProbe. Yet Cisco exporters use such high field IDs for exporting NBAR2 as well, for example.

Although I agree with you that the code should be kept as clean as possible from vendor-specific extensions and an intermediate plugin could solve the problem from a functional point of view, I see a clear issue here. In my opinion, intermediate plugins add functionality to IPFIXcol and should never be required for the correct operation of IPFIXcol. Thus not running an intermediate plugin should never make IPFIXcol crash or interfere with its main operations. In this case, however, mangled IPFIX messages are the result if we don't make changes to IPFIXcol's main code base, making the 'pure intermediate plugin' approach unfeasible.

I propose to make the main IPFIXcol code base (i.e., base/src/input/*/convert.c) add the 'all ones' PEN to IEs in NetFlow v9 PDUs with IDs larger than 32767. Adding 'real' PENs to 'converted' fields can still be done in an intermediate plugin, in case anyone really needs this functionality.

thorgrin commented 9 years ago

A bit of a misunderstanding here, I meant to suggest exactly the approach that you describe. Can you provide the trace to help with the development?

ghost commented 9 years ago

Please contact me at kirc&secdorks.net to get the trace, since I don't have your e-mail address. By the way, it'll be the same trace as to reproduce #14.

thorgrin commented 9 years ago

Just to let you know, I've started a discussion in the IETF IPFIX group: http://www.ietf.org/mail-archive/web/ipfix/current/msg07287.html

ghost commented 9 years ago

Personally, I believe that the 'all ones' PEN as discussed above is more elegant than the solutions proposed on the mailing list. What is your opinion? Also, what is exactly the status of this issue; is someone already implementing the functionality (which is mostly independent of the PEN used), or will you wait for the IPFIX WG to reach some consensus?

thorgrin commented 9 years ago

I do not really care what number will the new PEN have, it is just a constant. We will implement it with "all ones" PEN and revisit the issue when the IPFIX WG reaches a decision.

We have not started working on this one yet, but it is on a list, alas, I do not have a time estimate right now.

ghost commented 9 years ago

Another remark regarding the conversion of NetFlow v9 PDUs to IPFIX PDUs: the sequence number fields in the PDU headers have different semantics:

At this moment, I have no clue how to fix this or make estimations (for the IPFIX PDUs). I guess that components of IPFIXcol that rely on these sequence numbers (e.g., FlowWatch, part of the FastBit storage plugin) should not really be affected, although the message conveyed by these components differs per input plugin (which is a bit strange in nature).

mikeek commented 9 years ago

Please take a look at the latest IPFIXcol version (991dce62302ab498d95e71a08c18ee830f897638). NetFlow v9 elements with ID larger than 65535 should now be mapped on IPFIX elements with the same ID with Enterprise number set to "all ones".

thorgrin commented 9 years ago

Does this also solve #14?