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

[ProblemChild] Security Detection Rules Don't Allow for Wildcard Whitelisting #7579

Merged
merged 14 commits into from
Sep 1, 2023

Conversation

MakoWish
Copy link
Contributor

@MakoWish MakoWish commented Aug 28, 2023

Type of change

  • Enhancement

What does this PR do?

This PR converts the two security detection rules to EQL and adds a couple exceptions for Tenable Nessus processes.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have incremented the version in my package's manifest.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

@MakoWish MakoWish marked this pull request as ready for review August 28, 2023 21:27
@MakoWish MakoWish requested review from a team as code owners August 28, 2023 21:27
@elasticmachine
Copy link

elasticmachine commented Aug 28, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-01T15:07:44.712+0000

  • Duration: 14 min 30 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

No specific ML changes here, but LGTM for CODEOWNERS approval.

Copy link
Member

@susan-shu-c susan-shu-c left a comment

Choose a reason for hiding this comment

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

Will merge tomorrow if things are good

Copy link
Contributor

@ajosh0504 ajosh0504 left a comment

Choose a reason for hiding this comment

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

I'd suggest tackling known FPs earlier in the pipeline, rather than in rules which are the last step. Provided some options below.

@MakoWish
Copy link
Contributor Author

MakoWish commented Aug 30, 2023

We wouldn't want a drop processor, because we don't want to lose the event; we just don't want to be alerted of a ProblemChild event if it matches an exception.

As for using the existing conditional on the pipeline processor, I am not sure how you could iterate over an array of arguments in the conditional.

I think the only way to do it would be a painless script to check for one of the exceptions and add an ephemeral field/value like skip_problemchild: true. You could then add something like && !ctx.skip_problemchild == true in the existing conditional, then finally clean up the ephemeral field immediately after.

In any case, it might be quite an extra load on the ingest nodes for just trying to clean up a few dozen alerts. If the exceptions are done on the Detection Rule, we are only running a wildcard on events that already matched the ProblemChild criteria. If we were to check the arguments in the Ingest Pipeline as suggested, that would be running an expensive iterative regex on every argument in every event we are checking. In our environment, that would be ~250 billion events every 24 hours (winlogbeat and endpoint.events.process combined). Again, all just to keep from being alerted to a few dozen events we cannot create exceptions for elsewhere.

@ajosh0504
Copy link
Contributor

@MakoWish All fair points! In that case yeah let's tackle this at the rule level, and if you have identified patterns that would be useful outside of your environment as well, feel free to add those as well!

@MakoWish
Copy link
Contributor Author

@MakoWish ... if you have identified patterns that would be useful outside of your environment as well, feel free to add those as well!

I cannot think of any others at this time, but if anybody else has any they would like to include on this PR, I can get them added.

@ajosh0504
Copy link
Contributor

Nothing at the moment! Thanks once again for your contributions!

Copy link
Contributor

@ajosh0504 ajosh0504 left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but let's merge #7618 first and then this one?

@susan-shu-c
Copy link
Member

I've merged #7618

@susan-shu-c susan-shu-c merged commit 75b9931 into elastic:main Sep 1, 2023
3 checks passed
@elasticmachine
Copy link

Package problemchild - 1.1.2 containing this change is available at https://epr.elastic.co/search?package=problemchild

gizas pushed a commit that referenced this pull request Sep 5, 2023
Co-authored-by: Susan <23287722+susan-shu-c@users.noreply.github.com>
@elasticmachine
Copy link

Package problemchild - 2.0.0 containing this change is available at https://epr.elastic.co/search?package=problemchild

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.

[ProblemChild] Security Detection Rules Don't Allow for Wildcard Whitelisting
5 participants