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

Avoid to raise exception when the version header field doesn't exist #99

Closed
andsel opened this issue Oct 24, 2022 · 1 comment · Fixed by #98
Closed

Avoid to raise exception when the version header field doesn't exist #99

andsel opened this issue Oct 24, 2022 · 1 comment · Fixed by #98
Assignees

Comments

@andsel
Copy link
Contributor

andsel commented Oct 24, 2022

Logstash information:

Please include the following information:

  1. Logstash version (e.g. bin/logstash --version) any
  2. Logstash installation source (e.g. built from source, with a package manager: DEB/RPM, expanded from tar or zip archive, docker) tar.gz, git
  3. How is Logstash being run (e.g. as a service/service manager: systemd, upstart, etc. Via command line, docker/kubernetes) bin/logstah
  4. How was the Logstash Plugin installed already present

JVM (e.g. java -version): bundled one

If the affected version of Logstash is 7.9 (or earlier), or if it is NOT using the bundled JDK or using the 'no-jdk' version in 7.10 (or higher), please provide the following information:

  1. JVM version (java -version)
  2. JVM installation source (e.g. from the Operating System's package manager, from source, etc).
  3. Value of the JAVA_HOME environment variable if set.

OS version (uname -a if on a Unix-like system):

Linux kalimera 5.15.0-52-generic #58~20.04.1-Ubuntu SMP Thu Oct 13 13:09:46 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Description of the problem including expected versus actual behavior:
When an invalid CEF payload is presented to the codec, it should tag the event with _cefparsefailure instead of let throw an NoMethodError

Steps to reproduce:

Please include a minimal but complete recreation of the problem,
including (e.g.) pipeline definition(s), settings, locale, etc. The easier
you make for us to reproduce it, the more likely that somebody will take the
time to look at it.

  1. run logstash with the pipeline:
input {
 tcp {
   port => 12345
   codec => cef
 }
}

output {
 stdout {
   codec => rubydebug
 }
}
  1. send invalid payload
echo "1234567879" | netcat localhost 12345

Provide logs (if relevant):

[2022-10-24T12:28:11,861][INFO ][logstash.codecs.cef      ][main][aa3932885b23ccefb1e1b043583002dbdea4b1481e11d896c15a38f654b40607] DNADBG>> before crash {:cef_version_field=>"[cef][version]", :original_data=>"1234567879"}
[2022-10-24T12:28:11,863][ERROR][logstash.codecs.cef      ][main][aa3932885b23ccefb1e1b043583002dbdea4b1481e11d896c15a38f654b40607] Failed to decode CEF payload. Generating failure event with payload in message field. {:exception=>NoMethodError, :message=>"undefined method `include?' for nil:NilClass", :backtrace=>["/home/andrea/workspace/logstash_plugins/logstash-codec-cef/lib/logstash/codecs/cef.rb:242:in `handle'", "/home/andrea/workspace/logstash_plugins/logstash-codec-cef/lib/logstash/codecs/cef.rb:197:in `block in decode'", "org/jruby/RubyArray.java:1865:in `each'", "/home/andrea/workspace/logstash_plugins/logstash-codec-cef/lib/logstash/codecs/cef.rb:196:in `decode'", "/home/andrea/workspace/logstash_andsel/vendor/bundle/jruby/2.6.0/gems/logstash-input-tcp-6.3.1-java/lib/logstash/inputs/tcp.rb:194:in `decode_buffer'", "/home/andrea/workspace/logstash_andsel/vendor/bundle/jruby/2.6.0/gems/logstash-input-tcp-6.3.1-java/lib/logstash/inputs/tcp/decoder_impl.rb:23:in `decode'"], :original_data=>"1234567879"}
{
          "tags" => [
        [0] "_cefparsefailure"
    ],
       "message" => "1234567879",
    "@timestamp" => 2022-10-24T10:28:11.867177Z,
      "@version" => "1"
}
@yaauie
Copy link
Contributor

yaauie commented Oct 24, 2022

Description of the problem including expected versus actual behavior:
When an invalid CEF payload is presented to the codec, it should tag the event with _cefparsefailure instead of let throw an NoMethodError

As-described, this plugin handles failure in decoding by catching and logging the exception, and emitting a _cefparsefailure-tagged event whose message contains the entire byte sequence (link). It does not corrupt the codec, escape to the input, or crash the pipeline.

I agree that we should catch invalid CEF payloads as early as possible so that we can emit a more helpful log message.

In this particular case, we encountered a payload that did not have any of the 7 required leading headers, and should fail with a clear indication of such instead of letting a partial- or empty-headers event through to fail in later processing.

@roaksoax roaksoax changed the title Avoid to raise exception when the version header field doesn't exists Avoid to raise exception when the version header field doesn't exist Oct 24, 2022
yaauie added a commit to yaauie/logstash-codec-cef that referenced this issue Oct 25, 2022
When encountering malformed-CEF or non-CEF payloads, this plugin now emits
helpful descriptive log messages, and prevents data-loss and corruption by
emitting an event tagged with `_cefparsefailure` containing the bytes it
received.

This set of changes catches 3 distinct cases of malformed payloads.

  - missing one or more of the 7 required CEF header fields; a payload that
    does not have all 7 unescaped-pipe-terminated header fields cannot be
    reliably interpreted as CEF (prevents corruption).
  - containing something OTHER than a sequence of key=value pairs in the
    extensions space (prevent data-loss; previously when extensions were
    invalid they were silently omitted)
  - containing unescaped newlines (prevents corruption; previously data after
    the first newline was injected into the currently-parsed extension field)

In catching these classes of malformed inputs, this changeset also
resolves logstash-plugins#99 in which our failure
to detect a malformed input proactively caused an unhelpful `NoMethodError`
message to be logged before a `_cefparsefailure`-tagged event was emitted.
yaauie added a commit that referenced this issue Oct 26, 2022
* decode: multiline support in extension values

Per CEF spec v25 (2017-09-28):

> *Multi-line* fields can be sent by `CEF` by encoding the newline character
> as `\n` or `\r`. Note that multiple lines are only allowed in the value part
> of the extensions

While this plugin has long _encoded_ multiline extension values and other
escape sequences, our _decode_ has only supported escaped backslashes or
escaped equals signs, and with this change becomes compliant with this portion
of the spec. Note that due to the preexisting newline-centric normalization,
round-trip encode/decode cycle is only _semantically_ guaranteed.

* version bump & changelog for 6.2.6

* noop: remove $stderr debug logging

* fix: proactively detect malformed CEF

When encountering malformed-CEF or non-CEF payloads, this plugin now emits
helpful descriptive log messages, and prevents data-loss and corruption by
emitting an event tagged with `_cefparsefailure` containing the bytes it
received.

This set of changes catches 3 distinct cases of malformed payloads.

  - missing one or more of the 7 required CEF header fields; a payload that
    does not have all 7 unescaped-pipe-terminated header fields cannot be
    reliably interpreted as CEF (prevents corruption).
  - containing something OTHER than a sequence of key=value pairs in the
    extensions space (prevent data-loss; previously when extensions were
    invalid they were silently omitted)
  - containing unescaped newlines (prevents corruption; previously data after
    the first newline was injected into the currently-parsed extension field)

In catching these classes of malformed inputs, this changeset also
resolves #99 in which our failure
to detect a malformed input proactively caused an unhelpful `NoMethodError`
message to be logged before a `_cefparsefailure`-tagged event was emitted.

* fix: when using `delimiter`, ensure codec flush consumes buffer

Resolves #100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants