- 
                Notifications
    
You must be signed in to change notification settings  - Fork 5k
 
Update related to elastic integrations issue 14767 #47266
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
base: main
Are you sure you want to change the base?
Conversation
Adds the field 'process.args_count' to the winlogbeat security ingest pipeline
          🤖 GitHub commentsExpand to view the GitHub comments
 Just comment with: 
  | 
    
| 
           This pull request does not have a backport label. 
 To fixup this pull request, you need to add the backport labels for the needed 
  | 
    
| 
           Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)  | 
    
| 
           /test  | 
    
This enhancement adds the 'process.args_count' field to the winlogbeat windows security ingest pipeline, allowing for better tracking of process arguments.
| 
           Figured out how to add the changelog fragment. Should be ready for review, I hope.  | 
    
| def args = ctx?.process.args; | ||
| if (args != null && args != "") { | ||
| ctx.process.args_count = ctx.process.args.length; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def args = ctx?.process.args; | |
| if (args != null && args != "") { | |
| ctx.process.args_count = ctx.process.args.length; | |
| } | |
| if (ctx.process?.args instanceof List) { ctx.process.args_count = ctx.process.args.size(); } | 
This will ensure process.args is a non null List before trying to access the .size().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's needed.
If I'm reading correctly, lines 3690 to 3711 above this area handle the null checking since populating process.args is directly derived from line 3690 where there's a validation of whether the CommandLine field is null.
If not null, it sets the CommandLine value into an array list and isolates each component into the array list via the variable args that then populates the process.args (lines 3712 to 3716).
The suggested code change does another null check on the args condition, as derived from the previous null check leveraged against the CommandLine field.
All that being said, I think the non-null checking is already present. I'm very new to diving this deep, so please correct me if I'm mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewkroh What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is safe to use independent of the what happens before.
If you want to depend on the previous block's behavior then move the addition of the process.args_count into that block and directly reference the ArrayList's size().
         ctx.process.put("args", al);
+        ctx.process.put("args_count", al.size());
         ctx.process.put("command_line", ctx.winlog.event_data.CommandLine);
Adds the field 'process.args_count' to the winlogbeat security ingest pipeline.
What: I reviewed the ingest pipeline used for Sysmon and the Script processor that implements the "Windows-like SplitCommandLine" logic. Lines 542 to 546 implement the logic for Sysmon, so I adapted those lines into the correct order of operations defined in the Security ingest pipeline, inserted at line 3718.
Why: To make
process.args_countavailable for both Sysmon and Security telemetry for consistent searching, alerting, and exclusion handling.Checklist
- [ ] I have commented my code, particularly in hard-to-understand areas- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog tool.Disruptive User Impact
None identified
Author's Checklist
N/A
How to test this PR locally
Related issues
Use cases
Explained in the "WHY" above.
Screenshots
I don't think any are needed
Logs
Not applicable