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

Cosmetic syntactic and housekeeping fixes #278

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Cosmetic syntactic and housekeeping fixes #278

wants to merge 6 commits into from

Conversation

kravietz
Copy link
Contributor

Clean up rule files

@candlerb
Copy link

candlerb commented Jan 31, 2019

AFAICS, ossec/wazuh configuration files are not XML, I would say they are "XML-like".

Specifically:

  1. XML documents must have a single root element
  2. To match a literal left angle bracket, wazuh uses \< instead of &lt;. This breaks real XML parsers.
# xmllint --noout ./decoders/0340-trend-osce_decoders.xml
./decoders/0340-trend-osce_decoders.xml:19: parser error : StartTag: invalid element name
  <prematch>^20\d\d\d\d\d\d\<;></prematch>
                             ^
./decoders/0340-trend-osce_decoders.xml:20: parser error : StartTag: invalid element name
  <regex offset="after_prematch">^\d+\<;>\S+\<;>(\d+)\<;</regex>
                                       ^
./decoders/0340-trend-osce_decoders.xml:20: parser error : StartTag: invalid element name
  <regex offset="after_prematch">^\d+\<;>\S+\<;>(\d+)\<;</regex>
                                              ^
./decoders/0340-trend-osce_decoders.xml:20: parser error : StartTag: invalid element name
  <regex offset="after_prematch">^\d+\<;>\S+\<;>(\d+)\<;</regex>
                                                       ^

This is also why the regex engine treats \< as a special case, because the parser doesn't decode this sequence but passes both characters straight through.

@kravietz
Copy link
Contributor Author

@candlerb They aren't a well-formed XML but with a little cheating they can be processed using a XML parser. The cheating involves e.g. temporarily wrapping each .xml file with artificial <rules>...</rules> which resolves the multiple roots issue. Since I need to process the rules in batch frequently (rewrite, selectively enable and disable etc) I wrote a simple tool wazuh-rule-manager and obviously reimplementing a custom XML-like parser from scratch didn't really sound like the most exciting plan :)

@candlerb
Copy link

Sure, although your preprocessor would have to convert \< to something else too, at least if you're processing decoders. If your tool is only processing rules, then I don't think there are any examples of that syntax in the current ruleset.

@kravietz kravietz changed the title Rule XML syntactic fixes Cosmetic syntactic and housekeeping fixes Apr 23, 2019
@Lopuiz
Copy link
Contributor

Lopuiz commented Jun 18, 2019

Hello @kravietz,

First of all, thank you for your contribution to the Ruleset repository. And I am sorry for so late answer.
I had approved your modification in rules/0020-syslog_rules.xml and rules/log-entries/kernel in this PR.
Also, there is no difference between the original rules/0350-amazon_rules.xml file and the file in your PR.
I understand that it was a mistake and you just want to add the changes to the rules/log-entries/87300, rules/0485-cylance_rules.xml and rules/0500-owncloud_rules.xml files.
Can you solve it so the PR can be merged?

Kind regards, Eva

@Lopuiz Lopuiz self-requested a review June 18, 2019 09:36
@Lopuiz Lopuiz self-assigned this Jun 18, 2019
@kravietz
Copy link
Contributor Author

kravietz commented Jun 18, 2019

@Lopuiz There is an important change in the amazon_rules.xml commit - the problem is that the original version has Unicode ZERO WIDTH SPACE U+200B character inside the GuardDuty name which is not rendered in the diff (obviously) and only visible in hex editor. I stumbled upon this when I tried to grep logs for GuardDuty but didn't find anything because the string in log message was really Guard<U+200B>Duty. I think I described it in detail in the original commit message. Most likely the original author of the rule just copy-pasted the name from GuardDuty website not noticing it has this character inside.

@Lopuiz
Copy link
Contributor

Lopuiz commented Jun 18, 2019

All right, forgive the misunderstanding.
I will review it and give you an answer as soon as possible.

Regards, Eva

@kravietz
Copy link
Contributor Author

kravietz commented Aug 7, 2019

Hi @Lopuiz any luck with these? This is a small but important fix that will cause no problems.

@Lopuiz Lopuiz requested a review from bah07 August 9, 2019 06:19
@Lopuiz Lopuiz changed the base branch from master to 3.11 August 9, 2019 06:51
Copy link
Contributor

@Lopuiz Lopuiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

We will add these modifications in version 3.11 of Wazuh Ruleset

Regards,
Eva

@chemamartinez chemamartinez assigned chemamartinez and unassigned bah07 and Lopuiz Nov 21, 2019
@chemamartinez chemamartinez changed the base branch from 3.11 to 3.12 November 21, 2019 14:34
@chemamartinez
Copy link
Contributor

Hi @kravietz,

Sorry for the delay in the revisions and response, and thank you for the contribution. It is really appreciated.

I can see you add new rules for syslog events. However, you mentioned it neither in the title nor in the description. I am also missing sample events that match with new rules.

They would be very useful to create unit tests for newly rules. They are a requirement to add new rules to the repository. Here you can see some examples: https://github.com/wazuh/wazuh-ruleset/tree/master/tools/rules-testing/tests

Finally, the pull request has conflicts for the syslog rules file, probably due to other changes that have been added to the file. Can you rebase the 3.12 branch to solve them?

Again many thanks for the contribution, we hope changes are merged soon.

Best regards,
Chema.

@vikman90 vikman90 changed the base branch from 3.12 to develop July 31, 2020 12:11
@vikman90 vikman90 changed the base branch from develop to master September 25, 2020 08:21
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.

None yet

5 participants