Ignore trailing spaces in CEF messages #17253

adriansr · 2020-03-25T21:20:02Z

What does this PR do?

This patch updates the Ragel state machine to skip trailing space at the end of CEF messages.

Some CEF exporters, Check Point for example, have been observed to add a trailing space to CEF messages:

"CEF:0:| [...] src=127.0.0.1 "

Currently, this space character is interpreted as part of the last field's value, which can cause decoding errors if the value is an integer or an IP address:

"error": {
    "message": "sourceAddress: value is not a valid IP address"
  }

For maximizing compatibility, we also want to ignore other kinds of white-space characters (newline, carriage return, tab). For example we can get a trailing newline when processing CEF messages from UDP input instead of syslog, which removes newlines.

Trailing space in other extensions' values is preserved, as the CEF standard permits (but discourages) it's use in non-final extensions.

Why is it important?

We've observed users experiencing this problem, trying to fix it (unsuccessfully) with a processor:

- dissect:
      when:
        equals:
          event.module: "cef"
      tokenizer: "%{sourceAddress} "
      field: "cef.extensions.sourceAddress"
      target_prefix: "cef.extensions"

Why a draft?

While the current solution is complete, I'm not satisfied of the changes that introduces to the Ragel SM definition.

Originally I wanted to get something more elegant like this to work:

extensions = (extension " ")* final_extension space*

So that we can keep the extension machine as-is, and add a specialized final_extension machine that will disallow trailing space. However, I didn't manage to get this kind of pattern to work. It requires rewriting a lot of the capture actions to allow for the necessary backtracking, and I wasn't confident enough to get that right without introducing more problems than I was solving, or dedicating too many hours to this fix, due to my limited experience with ragel.

The current solution works by accident. I decided to try a different approach starting by disallowing trailing space in all extensions, and found out that it works as I wanted and captures white-space as value in all extensions but the last. This is a side effect of how an extension value is captured differently in extension_key vs extension_eof. The former captures the previous value up to mark-1, that is the start of the current key minus the space separator. The later captures up to extValueEnd, which is not incremented for trailing space.

The current machine definition is an ugly mix of " " and space usage, because we want to have the space character as the only separator, but trim al trailing space: [\t\v\f\n\r ]

This patch updates the ragel state machine to skip trailing whitespace at the end of CEF messages. Some CEF exporters, Check Point for example, have been observed to add a trailing whitespace to CEF messages: > "CEF:0:| [...] src=127.0.0.1 " Currently, this space character is interpreted as part of the last field's value, which can cause decoding errors if the value is an integer or an IP address. For maximizing compatibility, we also want to ignore other kinds of whitespace characters (new line, carriage return, tab). For example we can get a trailing newline when processing CEF messages from UDP input instead of syslog, which removes newlines. Whitespace in non-final extensions is preserved, as the CEF standard permits (but discourages) it's use in non-final extensions.

elasticmachine · 2020-03-25T21:20:04Z

Pinging @elastic/siem (Team:SIEM)

andrewkroh

Nice to see you got it working! Two things I'm curious about. 1) Maybe run benchcmp on this to see how/if the result changed. 2) How does this compare vs a TrimRight solution in a benchmark.

andrewkroh · 2020-03-25T22:33:41Z

x-pack/filebeat/processors/decode_cef/cef/cef.rl

-        extensions = " "* extension (" " extension)*;
+        extension_value = (space* extension_value_chars_nospace @extension_value_mark)* >extension_value_start $err(extension_err);
+        extension = extension_key equal extension_value;
+        extensions = " "* extension (space* " " extension)* space* %/extension_eof;


I was surprised that %/extension_eof worked for each individual extension, rather than just the final one. I'm definitely no Ragel expert.

No, I guess I didn't explain myself properly. extension_eof worked as expected. It's just that it captures a value until the last point saved by @extension_value_mark while extension_key saves the previous value up to the start of the separator. This made no difference before, but now it helps to capture trailing spaces in non-final extensions and to drop it in the final extension.

I had to move it to a different place here to adjust for the case where a msg is padded and EOF is never reached by extension_value due to it not accepting trailing spaces.

adriansr · 2020-03-26T11:22:01Z

@andrewkroh I ran some benchmarks with count=10 and comparing with benchstat.

All implementations are compared against the original state machine that doesn't trim.

Modified SM (this PR)

name                      old time/op  new time/op  delta
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef goos:darwin goarch:amd64
ProcessorRun/short_msg-8  5.29µs ± 0%  5.34µs ± 1%  +1.02%  (p=0.000 n=9+9)
ProcessorRun/long_msg-8   65.5µs ± 1%  65.7µs ± 0%    ~     (p=0.165 n=10+10)
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef/cef goos:darwin goarch:amd64
EventUnpack-8             2.17µs ± 1%  2.14µs ± 1%  -1.25%  (p=0.000 n=10+9)

strings.TrimRight [changes]

name                      old time/op  new time/op  delta
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef goos:darwin goarch:amd64
ProcessorRun/short_msg-8  5.29µs ± 0%  5.36µs ± 1%  +1.29%  (p=0.000 n=9+10)
ProcessorRun/long_msg-8   65.5µs ± 1%  64.3µs ± 1%  -1.94%  (p=0.000 n=10+10)
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef/cef goos:darwin goarch:amd64
EventUnpack-8             2.17µs ± 1%  2.17µs ± 1%    ~     (p=0.388 n=10+9)

Custom Loop [changes]

name                      old time/op  new time/op  delta
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef goos:darwin goarch:amd64
ProcessorRun/short_msg-8  5.29µs ± 0%  5.30µs ± 1%    ~     (p=0.660 n=9+10)
ProcessorRun/long_msg-8   65.5µs ± 1%  64.4µs ± 1%  -1.72%  (p=0.000 n=10+9)
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef/cef goos:darwin goarch:amd64
EventUnpack-8             2.17µs ± 1%  2.08µs ± 1%  -4.36%  (p=0.000 n=10+10)

Dedicated state-machine [changes]

Did this one just for the sake of it.

name                      old time/op  new time/op  delta
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef goos:darwin goarch:amd64
ProcessorRun/short_msg-8  5.29µs ± 0%  5.40µs ± 1%  +2.05%  (p=0.000 n=9+10)
ProcessorRun/long_msg-8   65.5µs ± 1%  66.1µs ± 1%  +0.91%  (p=0.001 n=10+10)
pkg:github.com/elastic/beats/v7/x-pack/filebeat/processors/decode_cef/cef goos:darwin goarch:amd64
EventUnpack-8             2.17µs ± 1%  2.16µs ± 1%  -0.61%  (p=0.003 n=10+10)

andrewkroh

Thanks for running the comparison. I didn't know about benchstat 😄 .

LGTM, but needs a changelog.

This patch updates the ragel state machine to skip trailing spaces at the end of CEF messages. Some CEF exporters, Check Point for example, have been observed to add a trailing space to CEF messages: > "CEF:0:| [...] src=127.0.0.1 " Currently, this space character is interpreted as part of the last field's value, which can cause decoding errors if the value is an integer or an IP address. For maximizing compatibility, we also want to ignore other kinds of space characters (new line, carriage return, tab). For example we can get a trailing newline when processing CEF messages from UDP input instead of syslog, which removes newlines. Spaces in non-final extensions are preserved, as the CEF standard permits (but discourages) it's use in non-final extensions. (cherry picked from commit a1ec55a)

This patch updates the ragel state machine to skip trailing spaces at the end of CEF messages. Some CEF exporters, Check Point for example, have been observed to add a trailing space to CEF messages: > "CEF:0:| [...] src=127.0.0.1 " Currently, this space character is interpreted as part of the last field's value, which can cause decoding errors if the value is an integer or an IP address. For maximizing compatibility, we also want to ignore other kinds of space characters (new line, carriage return, tab). For example we can get a trailing newline when processing CEF messages from UDP input instead of syslog, which removes newlines. Spaces in non-final extensions are preserved, as the CEF standard permits (but discourages) it's use in non-final extensions. (cherry picked from commit ee5d5bd)

adriansr added bug review Team:SIEM labels Mar 25, 2020

adriansr requested a review from andrewkroh March 25, 2020 21:20

Removed unreachable generated code

03df709

andrewkroh reviewed Mar 25, 2020

View reviewed changes

andrewkroh approved these changes Mar 26, 2020

View reviewed changes

Changelog

7a908c9

adriansr marked this pull request as ready for review March 26, 2020 14:10

adriansr added the needs_backport PR is waiting to be backported to other branches. label Mar 26, 2020

adriansr changed the title ~~[DRAFT] Ignore trailing spaces in CEF messages~~ Ignore trailing spaces in CEF messages Mar 26, 2020

adriansr merged commit a1ec55a into elastic:master Mar 27, 2020

adriansr mentioned this pull request Mar 27, 2020

Cherry-pick #17253 to 7.x: Ignore trailing spaces in CEF messages #17283

Merged

adriansr added v7.8.0 and removed needs_backport PR is waiting to be backported to other branches. labels Mar 27, 2020

adriansr mentioned this pull request Mar 27, 2020

Cherry-pick #17253 to 7.7: Ignore trailing spaces in CEF messages #17284

Merged

adriansr added the v7.7.0 label Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore trailing spaces in CEF messages #17253

Ignore trailing spaces in CEF messages #17253

adriansr commented Mar 25, 2020 •

edited

Loading

elasticmachine commented Mar 25, 2020

andrewkroh left a comment

andrewkroh Mar 25, 2020

adriansr Mar 26, 2020 •

edited

Loading

adriansr commented Mar 26, 2020 •

edited

Loading

andrewkroh left a comment

Ignore trailing spaces in CEF messages #17253

Ignore trailing spaces in CEF messages #17253

Conversation

adriansr commented Mar 25, 2020 • edited Loading

What does this PR do?

Why is it important?

Why a draft?

elasticmachine commented Mar 25, 2020

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh Mar 25, 2020

Choose a reason for hiding this comment

adriansr Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

adriansr commented Mar 26, 2020 • edited Loading

Modified SM (this PR)

strings.TrimRight [changes]

Custom Loop [changes]

Dedicated state-machine [changes]

andrewkroh left a comment

Choose a reason for hiding this comment

adriansr commented Mar 25, 2020 •

edited

Loading

adriansr Mar 26, 2020 •

edited

Loading

adriansr commented Mar 26, 2020 •

edited

Loading