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

Add event IDs 4797, 5379, 5380, 5381, and 5382 #34294

Merged
merged 13 commits into from
Jan 19, 2023
Merged

Conversation

MakoWish
Copy link
Contributor

@MakoWish MakoWish commented Jan 17, 2023

Type of change

  • Enhancement

What does this PR do?

This PR adds event parsing to the Security Ingest Pipeline for Winlogbeat for Event ID's 4797, 5379, 5380, 5381, and 5382.

Why is it important?

These Event ID's are quite common in our environment, so ensuring the correct event.action is populated will help identify the nature of the event at a quick glance.

Checklist

  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@MakoWish MakoWish requested a review from a team as a code owner January 17, 2023 19:11
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 17, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 17, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @MakoWish? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 17, 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-01-19T21:10:56.228+0000

  • Duration: 39 min 59 sec

Test stats 🧪

Test Results
Failed 0
Passed 336
Skipped 0
Total 336

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 17, 2023
@efd6
Copy link
Contributor

efd6 commented Jan 17, 2023

/test

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Are you able to provide events (as individual evtx) for these event types?

@MakoWish
Copy link
Contributor Author

I was able to grab some of them, but they contain sensitive usernames, hostnames, etc. Do you have a location I can send them to you out of the public eye?

@efd6
Copy link
Contributor

efd6 commented Jan 17, 2023

I think we could get around needing the evtx if we start from the XML by exporting this from the event viewer. This would mean that we can include the data in the tree without leaking your secrets if you scrub before letting us have the data. Does that sound reasonable to you?

@MakoWish
Copy link
Contributor Author

Attaching five scrubbed sample events for each of the Event ID's.
4797_5379_5380_5381_5382.zip

@MakoWish
Copy link
Contributor Author

Can we back-port this?

@efd6
Copy link
Contributor

efd6 commented Jan 18, 2023

This is an enhancement, so we will not back-port.

Tests cases mechanically derived from user-provided XML scrubbed event data.
@efd6
Copy link
Contributor

efd6 commented Jan 18, 2023

It looks like there needs to be some additional changes to make sure that things like related.host and related.user are properly updated for these event types. There are hooks in the security.yml file that allow this to be done by adding the appropriate event ID to the filter array.

@efd6
Copy link
Contributor

efd6 commented Jan 18, 2023

/test

@MakoWish
Copy link
Contributor Author

I don't see any processors that would add related.hosts, so I just focused on those that would add to related.user.

@efd6
Copy link
Contributor

efd6 commented Jan 18, 2023

/test

@efd6
Copy link
Contributor

efd6 commented Jan 18, 2023

/test

@@ -2604,7 +2639,7 @@ processors:
"4750", "4751", "4752", "4753", "4754", "4755", "4756", "4757",
"4758", "4759", "4760", "4761", "4762", "4763", "4764", "4767",
"4781", "4798", "4799", "4817", "4904", "4905", "4907", "4912",
"4648"].contains(ctx.event.code)) {
"4648", "4797"].contains(ctx.event.code)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to include the other types here? They also appear to have SubjectUserName fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was not. I had added them to the related user appending, but I forgot to add them to "Copy Subject User from Event Data" script. Since event 4797 also has a TargetUserName, I also just added that one to the "Copy Target User to Target" script.

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add_event_ids upstream/add_event_ids
git merge upstream/main
git push upstream add_event_ids

@efd6
Copy link
Contributor

efd6 commented Jan 19, 2023

/test

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Thanks

@efd6 efd6 merged commit de99256 into elastic:main Jan 19, 2023
@jamiehynds
Copy link

@efd6 could mappings for these events be applied to the system.security pipeline for agent, to ensure alignment between the winlogbeat and agent pipelines?

@MakoWish
Copy link
Contributor Author

@jamiehynds , please see #5085 that mirrors these changes to the Security Integration for Elastic Agent.

@jamiehynds
Copy link

Thanks @MakoWish, I missed that one. Thanks for all your recent contributions and collaboration!

@MakoWish
Copy link
Contributor Author

Thanks @MakoWish, I missed that one.

Didn't miss it. I created it after your comment. 👍

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
Tests cases mechanically derived from user-provided XML scrubbed event data.

Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7-candidate backport-skip Skip notification from the automated backport with mergify enhancement Winlogbeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Parsing for Event IDs 4797, 5379, 5380, 5381, and 5382
4 participants