Syslog parser drop invalid #3565

bazsi · 2021-02-06T13:34:58Z

This PR contains a combination of a long-needed msg-format related refactor (moving common code out of format modules into the msg-format layer) and the ability for syslog-parser() to drop incorrectly formatted messages (right now it is only RFC5424 that can indicate invalid messages, RFC3164 consumes everything).

The motivation behind this feature is that more and more rfc5424 style messages are actually incorrectly formatted, while still containing the initial "1 " version indicator right after the priority field. Instead of making the parser less strict, the drop-invalid() setting allows the easy identification of these messages, so that an alternative parser can be applied, like this:

@version: 3.30

log {
        source { tcp(port(2000) flags(no-parse)); };

        if {
                parser { syslog-parser(drop-invalid(yes) flags(syslog-protocol)); };
        } else {
                # drop RFC5424 indicator as it was not right
                rewrite { subst("^(<[0-9]+>)1 ", "$1"); };
                parser { syslog-parser(drop-invalid(no)); };
        };
        destination { file("log"); };
};

This was a long requested feature by the https://github.com/splunk/splunk-connect-for-syslog team @rfaircloth-splunk

kira-syslogng · 2021-02-06T13:50:25Z

Build FAILURE

lgtm-com · 2021-02-06T13:59:30Z

This pull request introduces 2 alerts when merging 6f67a4cb2c1f65ab7d4dbe86c486f64670d59464 into 975d950 - view on LGTM.com

new alerts:

2 for Implicit function declaration

kira-syslogng · 2021-02-15T08:50:51Z

Build FAILURE

bazsi · 2021-02-15T12:26:19Z

dropping "WIP", this should be good enough for review. The changes are not complex, however there's a series of them. I'd recommend going patch-by-patch.

The motiviation behind all the changes, the feature itself is trivial, once all the preparation is there.

kira-syslogng · 2021-02-15T12:45:06Z

Build FAILURE

mitzkia · 2021-02-18T13:29:47Z

@kira-syslogng test this please;

kira-syslogng · 2021-02-18T13:55:28Z

Build FAILURE

ryanfaircloth · 2021-02-26T01:39:56Z

looking forward to having this in the toolbox

gaborznagy

I only have some questions/comments.

The new feature looks good to me!

lib/msg-format.c

modules/syslogformat/syslog-parser.c

kira-syslogng · 2021-02-27T14:03:40Z

Build FAILURE

kira-syslogng · 2021-02-27T18:31:44Z

Build FAILURE

bazsi · 2021-02-27T18:32:56Z

@kira-syslogng retest this please;

kira-syslogng · 2021-02-27T18:55:05Z

Build FAILURE

kira-syslogng · 2021-02-27T20:14:41Z

Build FAILURE

gaborznagy · 2021-02-28T19:14:54Z

@kira-syslogng retest this please branch=syslog-parser-update-wrong-format-test;

kira-syslogng · 2021-02-28T19:38:49Z

Build SUCCESS

gaborznagy · 2021-02-28T19:45:07Z

I haven't checked why does the test_secure_logging UT fail only with cmake.
I've updated a test that was affected by the injected error message format, so kira is green now.

kira-syslogng · 2021-03-04T14:07:12Z

Build FAILURE

gaborznagy · 2021-03-04T14:53:12Z

@kira-syslogng retest this please branch=syslog-parser-update-wrong-format-test;

kira-syslogng · 2021-03-04T15:15:57Z

Build FAILURE

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

This makes it easier for me the read the argument list, "LogMessage *msg" becomes the first one right after the "self"-style argument, while the input and the error indication come last. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

msg_format_parse() might be called outside of the normal parsing (ie. syslog-parser), so move the message to log_msg_new() where it indeed happens. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

This function would return if it was successful instead of simply handling the error itself. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

msg_format_inject_parse_error() uses the string before that error position, which can read from buf[-1] in case position is zero, potentially leaking the byte in front of the data buffer, or if that address is unmapped, can also cause a SIGSEGV. In reality, that byte is part of the heap allocation header, so it shouldn't be unmapped, so SIGSEGV is not very probable, at least on common platforms. This patch also changes to using a dynamic buffer instead of a statically sized one, avoiding the truncation of the message at 2048 bytes. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Previously we retained flags like "mark" or "internal" which means that these values leak through a log_msg_clear(), but if we consider log_msg_clear() to be a function that gives us an empty slate, we should get rid of those flags too. Same as if we created a new LogMessage instance. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

The condition to use "kernel" as program name depended on LogMessage->flags set prior to the message being parsed. Since the location where these flags were set is pretty far from the syslog message parser code, this dependency was not easy to recognize and the previous set of refactoring steps even broke the assumption. I've decided to make the dependency clearer while retaining the workaround where it is today: * LF_INTERNAL check is removed, the syslog parser is never invoked on internal() logs (historically it probably was...) * the check on LF_LOCAL was converted to a parse_options->flags check on LP_LOCAL. Parse_options clearly affects how the message is parsed and is used in a number of different locations within the same function. * the check on <pri> value is retained, as parsing the pri value is logically in the same location. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

…t test Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Kokan · 2021-03-11T13:45:19Z

modules/python/tests/test_python_template.c

-  MsgFormatOptions template_parse_options = {0};
-  msg_format_options_defaults(&template_parse_options);
-  syslog_format_handler(&template_parse_options, (const guchar *)raw_msg, strlen(raw_msg), msg);
+  LogMessage *msg = log_msg_new(raw_msg, strlen(raw_msg), &parse_options);


I would prefer this UT not using syslogformat in the first place. But this was not introduced by this PR, so I am okay with the follow up.

Kokan · 2021-03-11T21:42:47Z

Also if possible please write a news entry under news/ directory. But this does not block the merge, if somebody merges this as is, they can write one and open a PR.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

bazsi · 2021-03-12T11:55:43Z

added news file + rebased

kira-syslogng · 2021-03-12T12:19:43Z

Build FAILURE

bazsi · 2021-03-12T12:24:08Z

@kira-syslogng retest this please branch=syslog-parser-update-wrong-format-test;

kira-syslogng · 2021-03-12T12:43:40Z

Build SUCCESS

gaborznagy · 2021-03-19T14:15:24Z

I've created an internal doc update ticket about the new option and the changed behaviour of msg-format flags (i.e. from now on if "no-parse" flag is used additional flags e.g. "no-multi-line" can be used as well.)

gaborznagy · 2021-03-19T15:25:32Z

@bazsi I wonder: did we intentionally not document the injected error message?
This is the only thing in the documentation:

If syslog-ng OSE cannot parse a message, it results in an error

I would mention the details of the error message (rewritten program and message fields, modified facility and severity values).

bazsi · 2021-03-19T17:46:35Z

There was no intention, it just happened this way. Documenting the error handling might indeed be useful, as well as the drop-invalid option in the context of syslog-parser ().

…

On Fri, Mar 19, 2021, 16:25 Gábor Nagy ***@***.***> wrote: @bazsi <https://github.com/bazsi> I wonder: did we intentionally not document the injected error message? This is the only thing in the documentation: If syslog-ng OSE cannot parse a message, it results in an error I would mention the details of the error message (rewritten program and message fields, modified facility and severity values). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3565 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFOK5XAMP52MBS76KXMZP3TENUH7ANCNFSM4XGJJW2Q> .

gaborznagy · 2021-03-23T13:20:08Z

Thanks, I've submitted a ticket to the doc team.

bazsi force-pushed the syslog-parser-drop-invalid branch from 6f67a4c to 108a383 Compare February 15, 2021 08:32

bazsi force-pushed the syslog-parser-drop-invalid branch from 108a383 to 35480bb Compare February 15, 2021 12:24

bazsi changed the title ~~WIP: Syslog parser drop invalid~~ Syslog parser drop invalid Feb 15, 2021

bazsi force-pushed the syslog-parser-drop-invalid branch from 35480bb to 880f3a4 Compare February 15, 2021 12:27

gaborznagy added this to the syslog-ng-3.31 milestone Feb 25, 2021

gaborznagy self-requested a review February 25, 2021 09:21

gaborznagy reviewed Feb 26, 2021

View reviewed changes

bazsi force-pushed the syslog-parser-drop-invalid branch from 880f3a4 to 58afacd Compare February 27, 2021 13:56

bazsi force-pushed the syslog-parser-drop-invalid branch 2 times, most recently from e63a217 to bd9d626 Compare February 27, 2021 18:31

bazsi force-pushed the syslog-parser-drop-invalid branch from 1747ca3 to 8e72c8a Compare March 4, 2021 13:39

bazsi added 11 commits March 11, 2021 14:51

msg-format: extract error handling from syslog-format

a7d56e4

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

msg-format: move initial parsing message to the correct spot

480233e

msg_format_parse() might be called outside of the normal parsing (ie. syslog-parser), so move the message to log_msg_new() where it indeed happens. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

msg-format: extend error message about missing format plugin

61607e2

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

msg-format: add msg_format_parse_conditional() function

8e4c88e

This function would return if it was successful instead of simply handling the error itself. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

syslogformat: add drop-invalid option to syslog-parser()

6510d32

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

secure-logging: add syslogformat dependency to test_secure_loggin uni…

d6e76ff

…t test Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

lib/msg-format: fix style issues reported by the CI

0a3c70d

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

Kokan previously approved these changes Mar 11, 2021

View reviewed changes

news/news-3565: add new file

dba4f98

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>

bazsi dismissed Kokan’s stale review via dba4f98 March 12, 2021 11:55

bazsi force-pushed the syslog-parser-drop-invalid branch from b07566e to dba4f98 Compare March 12, 2021 11:55

Kokan approved these changes Mar 12, 2021

View reviewed changes

gaborznagy approved these changes Mar 19, 2021

View reviewed changes

gaborznagy added the user-visible-feature User visible feature label Mar 19, 2021

gaborznagy merged commit de171ed into syslog-ng:master Mar 19, 2021

bazsi deleted the syslog-parser-drop-invalid branch November 17, 2021 12:46

gaborznagy mentioned this pull request Apr 5, 2022

rfc5424 "dropinvalid(yes)" does not drop when sdata is invalid #3973

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syslog parser drop invalid #3565

Syslog parser drop invalid #3565

bazsi commented Feb 6, 2021 •

edited

kira-syslogng commented Feb 6, 2021

lgtm-com bot commented Feb 6, 2021

kira-syslogng commented Feb 15, 2021

bazsi commented Feb 15, 2021

kira-syslogng commented Feb 15, 2021

mitzkia commented Feb 18, 2021

kira-syslogng commented Feb 18, 2021

ryanfaircloth commented Feb 26, 2021

gaborznagy left a comment

kira-syslogng commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

bazsi commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

gaborznagy commented Feb 28, 2021

kira-syslogng commented Feb 28, 2021

gaborznagy commented Feb 28, 2021

kira-syslogng commented Mar 4, 2021

gaborznagy commented Mar 4, 2021

kira-syslogng commented Mar 4, 2021

Kokan Mar 11, 2021

Kokan commented Mar 11, 2021

bazsi commented Mar 12, 2021

kira-syslogng commented Mar 12, 2021

bazsi commented Mar 12, 2021

kira-syslogng commented Mar 12, 2021

gaborznagy commented Mar 19, 2021

gaborznagy commented Mar 19, 2021

bazsi commented Mar 19, 2021 via email

gaborznagy commented Mar 23, 2021

Syslog parser drop invalid #3565

Syslog parser drop invalid #3565

Conversation

bazsi commented Feb 6, 2021 • edited

kira-syslogng commented Feb 6, 2021

lgtm-com bot commented Feb 6, 2021

kira-syslogng commented Feb 15, 2021

bazsi commented Feb 15, 2021

kira-syslogng commented Feb 15, 2021

mitzkia commented Feb 18, 2021

kira-syslogng commented Feb 18, 2021

ryanfaircloth commented Feb 26, 2021

gaborznagy left a comment

Choose a reason for hiding this comment

kira-syslogng commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

bazsi commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

kira-syslogng commented Feb 27, 2021

gaborznagy commented Feb 28, 2021

kira-syslogng commented Feb 28, 2021

gaborznagy commented Feb 28, 2021

kira-syslogng commented Mar 4, 2021

gaborznagy commented Mar 4, 2021

kira-syslogng commented Mar 4, 2021

Kokan Mar 11, 2021

Choose a reason for hiding this comment

Kokan commented Mar 11, 2021

bazsi commented Mar 12, 2021

kira-syslogng commented Mar 12, 2021

bazsi commented Mar 12, 2021

kira-syslogng commented Mar 12, 2021

gaborznagy commented Mar 19, 2021

gaborznagy commented Mar 19, 2021

bazsi commented Mar 19, 2021 via email

gaborznagy commented Mar 23, 2021

bazsi commented Feb 6, 2021 •

edited