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.

Sign up for GitHub

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

Jump to bottom

[process/windows] - Ignore accessing command line arguments for selected processes (currently, lsass.exe) #198

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[process/windows] - Ignore accessing command line arguments for selected processes (currently, lsass.exe) #198

wants to merge 11 commits into from
+79 −8

Conversation

VihasMakwana
Copy link
Contributor

See elastic/beats#41407 for more details.

TL;DR;

Processes such as lsass.exe has no meaningful cmd line and and can trigger false positives with Windows ASR rules.
We can find pid for that using SYSTEM\\CurrentControlSet\\Control\\Lsa path

Relates elastic/beats#41407

@VihasMakwana VihasMakwana requested a review from a team as a code owner December 17, 2024 18:00
@VihasMakwana VihasMakwana requested review from mauri870 and faec and removed request for a team December 17, 2024 18:00
@VihasMakwana VihasMakwana self-assigned this Dec 17, 2024
@VihasMakwana VihasMakwana added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Dec 17, 2024
@VihasMakwana VihasMakwana mentioned this pull request Dec 17, 2024
Open
andrewkroh
andrewkroh reviewed Dec 17, 2024
View reviewed changes
metric/system/process/process_windows.go Outdated Show resolved Hide resolved
metric/system/process/process_windows.go Outdated Show resolved Hide resolved
@VihasMakwana VihasMakwana changed the title [process/windows] - Ignore accessing command line arguments for selected processes (current lsass.exe) [process/windows] - Ignore accessing command line arguments for selected processes (currently, lsass.exe) Dec 18, 2024
swiatekm
swiatekm reviewed Dec 18, 2024
View reviewed changes
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Looks good, I'd love to see at least one unit test for the new behaviour.

metric/system/process/process.go Show resolved Hide resolved
@VihasMakwana
Copy link
Contributor Author

@swiatekm @mauri870 I've added unit test case and fixed some bug. can you take a look?

@ycombinator
Copy link
Contributor

@gabriellandau could you also re-review this PR, please? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauri870 mauri870 mauri870 approved these changes

@faec faec Awaiting requested review from faec faec is a code owner automatically assigned from elastic/elastic-agent-data-plane

@cmacknz cmacknz Awaiting requested review from cmacknz

@andrewkroh andrewkroh Awaiting requested review from andrewkroh

@gabriellandau gabriellandau Awaiting requested review from gabriellandau

@swiatekm swiatekm Awaiting requested review from swiatekm

Requested changes must be addressed to merge this pull request.

Assignees

@VihasMakwana VihasMakwana

Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@VihasMakwana @ycombinator @cmacknz @andrewkroh @mauri870 @gabriellandau @swiatekm