Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Decoding of IPFIX templates with Enterprise Number field #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shyam334
Copy link
Contributor

@shyam334 shyam334 commented Mar 25, 2020

Bug: Decoding of IPFIX templates with Enterprise Number field

The goflow IPFIX template decoder isn't aware of the Enterprise Number field, which results in malformed template(s) and processing, when decoding IPFIX templates with Enterprise Number field.

Following is the field specifier format from RFC7011#section-3.2, Figure G :

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |E|  Information Element ident. |        Field Length           |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                      Enterprise Number                        |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

When goflow tries to decode an IPFIX template with Enterprise Number, It attempts to decode the Enterprise Number field as a regular Field in the template.

This results in a malformed template. As a result the corresponding IPFIX Datasets would not be processed. (i.e. goflow would not emit any records)

Patch

The patch adds a seperate path for IPFIX template parsing, where it checks for the Enterprise bit condition and skips the Enterprise Number field.

This will enable the IPFIX Template(s) to be decoded correctly and the corresponding IPFIX Datasets to be processed.

goflow fails to correctly decode IPFIX templates with Enterprise Number field. (RFC7011#section-3.2)
The patch skips the Enterprise Number field, so the template can be decoded correctly.
@lspgn
Copy link
Contributor

lspgn commented Mar 25, 2020

Thanks a lot for the bugfix! (also referencing #31 as it was mentioning Enterprise Templates).
Do you have some samples I could test this with?

@shyam334
Copy link
Contributor Author

@lspgn Thanks for promptly looking into this.
I just realised that I don't have a representative pcap that I can share publicly. Let me work that out and get back.

Separately, Is there a more ad-hoc channel to collaborate. (slack or such)

@shyam334
Copy link
Contributor Author

shyam334 commented Mar 27, 2020

@lspgn
Copy link
Contributor

lspgn commented Mar 27, 2020

There is no slack for GoFlow but feel free to email me: louis at cloudflare.com

Thank you for the sample, will test it out.

@raghurampai
Copy link

@lspgn I guess merge is pending for this request. So, is this planned for next release?

field := Field{}
err = utils.BinaryDecoder(payload, &field)
if field.Type > math.MaxInt16 {
field.Type = field.Type - math.MaxInt16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is off by one, you need

field.Type -= 1 << 15                                                                                  

Because:

1000000000000001

needs to turn into just '1', so you need to subtract

1000000000000000

not

0111111111111111

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will update. (could've sworn it was in my fork)

if field.Type > math.MaxInt16 {
field.Type = field.Type - math.MaxInt16
// Skip Enterprise Number field - rfc7011#section-3.2
_ = payload.Next(4)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping the 4 bytes fixes the parsing, but I think it needs to track that this was a PEN field inside the Field type. In the data that I have the resulting Type will be 1 or 2. The later bits will then just treat those fields as as IPFIX_FIELD_octetDeltaCount or IPFIX_FIELD_packetDeltaCount which would be wrong.

tgragnato pushed a commit to tgragnato/goflow that referenced this pull request Aug 14, 2024
Bugfix: issues when reading a partial chunk of protobuf from stdin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants