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

os_regex: refuse to compile empty PCRE2 pattern. #1826

Merged
merged 4 commits into from
Jan 18, 2020

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Jan 16, 2020

The OSPcre2_Compile function should refuse to compile empty patterns and instead return an error. This avoids an off-by-one error that can occur later in the function when an assumption is made that the pattern is always >= 1 characters long.

A distinct error code OS_REGEX_PATTERN_EMPTY (10) is added to os_regex/os_regex.h to be returned by OSPcre2_Compile for empty patterns similar to the OS_REGEX_PATTERN_NULL error.

Since the os_regex PCRE2 code is updated to return an error when asked to compile an empty pattern the empty <program_name_pcre2> element in the example decoder.xml config for
the "pam" decoder also needs to be updated or it will prompt an error from ossec-analysisd when it tries to compile the PCRE2 pattern. Using .* as the program name PCRE2 regex has (what I think is) the intended effect of the two PAM decoders.

Resolves #1811

Daniel added 2 commits January 16, 2020 16:48
The `OSPcre2_Compile` function should refuse to compile empty patterns
and instead return an error. This avoids an off-by-one error that can
occur later in the function when an assumption is made that the pattern
is always >= 1 characters long.

A distinct error code `OS_REGEX_PATTERN_EMPTY` (10) is added to
`os_regex/os_regex.h` to be returned by `OSPcre2_Compile` for empty
patterns similar to the `OS_REGEX_PATTERN_NULL` error.
The `os_regex` PCRE2 code has been updated to return an error when asked
to compile an empty pattern. As a result the empty
`<program_name_pcre2>` element in the example `decoder.xml` config for
the `"pam"` decoder needs to be removed or it will prompt an error from
`ossec-analysisd` when it tries to compile the PCRE2 pattern.
@cpu
Copy link
Contributor Author

cpu commented Jan 16, 2020

Based on the CI failure I think the change to the decoder.xml may have broken the PAM decoder in a way I didn't anticipate. Perhaps a maintainer can make a suggestion here?

Was the empty <program_name_pcre2></program_name_pcre2> functionally equivalent to <program_name_pcre2>.*</program_name_pcre2> pre-bugfix? Or perhaps I don't understand how multiple <decoder>'s with the same name interact? Maybe I'm just plain wrong along another new and surprising dimension :-)

Please advise. Thanks!

@ddpbsd
Copy link
Member

ddpbsd commented Jan 17, 2020

I just played with the pam decoder a bit to try and figure out what's going on. It's been a long day though, so this may be totally stupid. I also haven't spent much time inside of analysisd, and did not test with the submitted changes. Finally, if I just write program_name, assume I mean program_name_pcre2.

It looks like the 2 "pam" decoders end up being an OR. If the program_name is (pam_unix) OR if there is any program_name but the log message starts with one of the pam_unix variations in the decoder, the pam decoder will match.

Removing the empty program_name from the second decoder causes a failure (like the one in CI). I assume this is because analysisd doesn't start looking for the pam string after program_name, but at the beginning of the log message. This seems odd to me, but it's what we have.

**Phase 1: Completed pre-decoding.
       full event: 'Jun 28 23:01:27 xxxx auth: pam_unix(dovecot:auth): authentication failure; logname= uid=0 euid=0 tty=dovecot ruser=lipjigaglgihgoeadcdaa.p.salmon@xxx.xxx.xxx.xxx rhost=91.195.103.44'
       hostname: 'xxxx'
       program_name: 'auth'
       log: 'pam_unix(dovecot:auth): authentication failure; logname= uid=0 euid=0 tty=dovecot ruser=lipjigaglgihgoeadcdaa.p.salmon@xxx.xxx.xxx.xxx rhost=91.195.103.44'

**Phase 2: Completed decoding.
       No decoder matched.

**Phase 3: Completed filtering (rules).
       Rule id: '2501'
       Level: '5'
       Description: 'User authentication failure.'
**Alert to be generated.

Using a program_name_pcre2 of .* gives the expected output:

**Phase 1: Completed pre-decoding.
       full event: 'Jun 28 23:01:27 xxxx auth: pam_unix(dovecot:auth): authentication failure; logname= uid=0 euid=0 tty=dovecot ruser=lipjigaglgihgoeadcdaa.p.salmon@xxx.xxx.xxx.xxx rhost=91.195.103.44'
       hostname: 'xxxx'
       program_name: 'auth'
       log: 'pam_unix(dovecot:auth): authentication failure; logname= uid=0 euid=0 tty=dovecot ruser=lipjigaglgihgoeadcdaa.p.salmon@xxx.xxx.xxx.xxx rhost=91.195.103.44'

**Phase 2: Completed decoding.
       decoder: 'pam'
       dstuser: 'lipjigaglgihgoeadcdaa.p.salmon@xxx.xxx.xxx.xxx'
       srcip: '91.195.103.44'
       srcgeoip: 'CZ'

**Phase 3: Completed filtering (rules).
       Rule id: '5503'
       Level: '5'
       Description: 'User login failed.'
**Alert to be generated.

Based on a quick grep, this is the only instance of an empty program_name_pcre2. I think it would be safe to modify the decoder to use .* or something similar (I think just . works too).

@cpu
Copy link
Contributor Author

cpu commented Jan 17, 2020

I appreciate you digging in @ddpbsd. Thanks!

It looks like the 2 "pam" decoders end up being an OR. If the program_name is (pam_unix) OR if there is any program_name but the log message starts with one of the pam_unix variations in the decoder, the pam decoder will match.

That makes sense 👍

Based on a quick grep, this is the only instance of an empty program_name_pcre2. I think it would be safe to modify the decoder to use .* or something similar (I think just . works too).

I don't have access to my test environment at the moment to run the unit tests but I've pushed a commit (6b5995d) to this branch adding a .* regex to replace the empty <program_name_pcre2> in the 2nd PAM decoder to see how it goes in CI. 🤞

@cpu
Copy link
Contributor Author

cpu commented Jan 17, 2020

I don't have access to my test environment at the moment to run the unit tests but I've pushed a commit (6b5995d) to this branch

... and promptly noticed I didn't close the XML tag correctly. Fixed with ff67c94

@cpu
Copy link
Contributor Author

cpu commented Jan 17, 2020

to see how it goes in CI.

CI is satisfied 🎉 If you folks are happy with this fix/line of reasoning I think the branch is ready to go.

@ddpbsd
Copy link
Member

ddpbsd commented Jan 17, 2020

Thanks! I've pinged @atomicturtle to look it over and make sure I'm not totally off base with everything. But I think it'll be fine to merge.

@atomicturtle
Copy link
Member

Yeah theres no issue here, we havent started merging over to the pcre2 regexes and this does nothing less than enforce better discipline when we start that project up in earnest. And props to your awesome username, how in the world did you get that?? Let us know if you want an invite to our slack: slack@ossec.net

@atomicturtle atomicturtle merged commit b7f645f into ossec:master Jan 18, 2020
@cpu cpu deleted the cpu-os_regex-fix-empty-pcre2-compile branch January 20, 2020 14:52
@cpu
Copy link
Contributor Author

cpu commented Jan 20, 2020

And props to your awesome username, how in the world did you get that??

😆 Thanks :-) Right place at the right time.

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.

os_regex: OSPcre2_Compile off-by-one heap overflow compiling empty regex.
3 participants