Fixes for parsing RawPacketHeader

path-network / logstash-codec-sflow

Logstash codec plugin to decrypt sflow

Other

35 stars 17 forks source link

Fixes for parsing RawPacketHeader #18

Closed kzemek closed 5 years ago

kzemek commented 5 years ago

This pull request fixes a couple of parsing errors that we've seen in real deployments, specifically:

RawPacketHeader containing IPv4 with protocol=6 but not containing a TCP header
TCP header inside RawPacketHeader having tcp_header_length>5 but not containing TCP options
TCP header inside RawPacketHeader having bad tcp_header_length (set by a malicious party)

Those changes fix all of the errors that we've seen so far. A generic fix for this kind of issues is to parse as many header fields as fit inside header_size bytes, but sans a modification to BinData, that seems to require a lot of onlyif boilerplate.

ashangit commented 5 years ago

@kzemek thanks for the PR As you have probably seen I don't actively support this plugin anymore. As some seems to need/use it I already tried long time ago to move it to official community logstash plugin github without any luck https://github.com/elastic/logstash/issues/4809 I'm trying again through this issue to move it to community plugin https://github.com/logstash-plugins/logstash-codec-sflow/issues/1 Otherwise would you be interested to take ownership of it?

robcowart commented 5 years ago

If @kzemek in unable to take it, I am willing to maintain it.

kzemek commented 5 years ago

@ashangit We would be happy to maintain it on @path-network . Our RubyGems account is https://rubygems.org/profiles/PathNetwork if you'd like to transfer that as well.

ashangit commented 5 years ago

@kzemek sorry for the noise, I already discuss with @robcowart and he will maintain it

robcowart commented 5 years ago

@kzemek I am happy to let you take over as the maintainer. I only volunteered in case you did not take it. Let me know if you want me to transfer it to you.

kzemek commented 5 years ago

@robcowart We would very much like to maintain it. :) We're processing massive amounts of sFlow records and this codec is an important part of our pipeline, and as you can see from our pull requests we are looking to improve it.