-
Notifications
You must be signed in to change notification settings - Fork 516
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
[FR] Generate investigation guides #4358
base: main
Are you sure you want to change the base?
Conversation
Enhancement - GuidelinesThese guidelines serve as a reminder set of considerations when addressing adding a feature to the code. Documentation and Context
Code Standards and Practices
Testing
Additional Checks
|
if 'query' in osquery_item and isinstance(osquery_item['query'], str): | ||
# Transform instances of \ to \\ as calling write will convert \\ to \. | ||
# This will ensure that the output file has the correct number of backslashes. | ||
osquery_item['query'] = osquery_item['query'].replace("\\", "\\\\") |
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 noticed this when trying to run toml-lint on our rules. Without this accounted for, it breaks the loader on formatting issues.
|
||
for rule in self.all_rules: | ||
if not rule.contents.data.is_elastic_rule: | ||
continue # Don't enforce on non-Elastic rules |
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.
Question if we should go ahead an enforce on all rules.
def test_note_contains_triage_and_analysis(self): | ||
"""Ensure the note field contains Triage and analysis content for Elastic rules.""" |
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.
Added a unit test that enforces investigation guides for Elastic prebuilt rules
Is this needed? If we are going to go all in for the generated guides, couldn't we have a weekly/monthly automation that generates guides for new rules and open a PR?
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 preference is have the complete rule at initial release, so users get full picture, and we do not introduce additional updates. I can understand there might be exceptions for urgent releases.
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.
IMO: That adds more complexity when reviewing, potentially distracting reviewers from what matters the most: rule logic
Plus, it adds one more step to get the rules done, as authors would need to fix the generated guide before pushing the PR
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.
This question is related to #4358 (comment) FWIW, we now have a GitHub action job that can run on a detection-rules branch to generate a guide (given the rule uuid), so the idea would be to generate the guide assuming it follows the same general guidance as this PR.
On another vein, an alternative idea would be to remove the unit test and make generating guides a step of the release where guides are created in a single PR prior to shipping, but ideally the original authors would still review those anyway. It may be easier to review in the context of the PR when the rule was created.
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.
This is very good, saving us lots of time, while still providing some very basic guidance to junior SOC personel. My few cents:
- I think we should avoid providing OSQuery queries, or specifically mention somewhere that the OSQueries that are generated might be wrong. I understand this is part of the initial disclaimer, but I still think it's a bit silly to provide wrong queries if we state that "these have been reviewed to improve its accuracy and relevance". Another option would be to just refer to the OSquery documentation, and mention the pre-built OSQuery packs + our hunting queries.
- AI sometimes provides duplicate information or useless/too broad investigation steps. I'm curious whether working with a 2-step model would work. Provide input from this AI into a new AI with more strict rules to filter out these mistakes/inconsistencies. This might also filter out the cases where AI is writing about paths that do not exist for example.
- In general this is great to start. I am just curious whether the prompt could be altered to make more specific investigations for certain activity, because it remains very broad. I understand the more specific you go, the more it will get wrong. Curious to see what options we could have here.
- I also wonder the same thing as @w0rk3r does with regards to unit testing enforcements. Having CI/CD handle that for us would be convenient.
- Review the alert details to identify the specific file change event, including the file name, file path, and process executable involved, to understand the context of the suspicious activity. | ||
- Verify the legitimacy of the process executable by checking the hash of the binary at the path "/usr/sbin/sshd" or "/usr/bin/ssh" against known good hashes to detect any unauthorized modifications. | ||
- Examine the file name and extension involved in the alert to determine if it matches any known patterns of backdoor logging files, such as unusual extensions like "in", "out", "ini", or temporary file patterns like "*~". | ||
- Investigate the file path to determine if it matches any of the suspicious directories listed in the query, such as "/private/etc/ssh/.sshd_auth" or "/usr/lib/*.so.*", which may indicate unauthorized file placement. |
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.
This right here is the main downside of GenAI, it's talking about /private/etc/ssh/.sshd_auth
, which AFAIK either does not exist, or is a MacOS path.
Can we build in checks when prompting it to only provide information when it is 100% sure its correct? Or will it hallucinate anyways?
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.
yea the issue is because it takes the context from the query. which has that path included. =) good tuning opportunity @Aegrah
@Mikaayenson Thank you for the heads up. How many new rule versions are we adding -- 902? cc @xcrzx It's awesome that we're adding investigation guides to the remaining prebuilt rules! |
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.
Reviewed the ML investigation guides, and overall, the content looks good. I've added a few minor comments regarding Osquery and some descriptive inconsistencies in the investigation steps. Wonder if we could make the guides more concise by removing duplicate information or having the model summarize it further?
rules/integrations/beaconing/command_and_control_beaconing.toml
Outdated
Show resolved
Hide resolved
@Mikaayenson @w0rk3r Regarding OSQuery queries, how many are suggested for these new guides? Would it be possible to have the interactive osqueries, like here: We'd potentially need manual review/fixes for them. |
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.
Left some comments on SecML packages. Thanks for doing this!
For potential updates in the future, I'm wondering if we could generate less points in each section (such as "response and remediation") so that the LLM generates the most relevant ones? From my first impression, it feels that there are some less important points that it brought up just to fulfill some sort of bullet point count. But it's mostly a first impression.
rules/integrations/beaconing/command_and_control_beaconing.toml
Outdated
Show resolved
Hide resolved
rules/integrations/beaconing/command_and_control_beaconing.toml
Outdated
Show resolved
Hide resolved
rules/integrations/beaconing/command_and_control_beaconing_high_confidence.toml
Outdated
Show resolved
Hide resolved
rules/integrations/ded/exfiltration_ml_high_bytes_destination_geo_country_iso_code.toml
Outdated
Show resolved
Hide resolved
rules/integrations/ded/exfiltration_ml_high_bytes_destination_ip.toml
Outdated
Show resolved
Hide resolved
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.
Suggesting some small changes.
rules/integrations/ded/exfiltration_ml_high_bytes_destination_geo_country_iso_code.toml
Outdated
Show resolved
Hide resolved
rules/integrations/ded/exfiltration_ml_high_bytes_destination_region_name.toml
Outdated
Show resolved
Hide resolved
rules/integrations/dga/command_and_control_ml_dga_high_sum_probability.toml
Outdated
Show resolved
Hide resolved
rules/integrations/lmd/lateral_movement_ml_spike_in_connections_to_a_destination_ip.toml
Outdated
Show resolved
Hide resolved
Update Jan 10 - Regenerated GuidesI took all the feedback and regenerated guides. Essentially making these changes:
cc. folks who've already provided feedback so far @susan-shu-c @sodhikirti07 @Aegrah @w0rk3r |
Pull Request
Issue link(s): https://github.com/elastic/ia-trade-team/issues/160
Summary - What I changed
Details
How To Test / Review
Sample UI: AWS IAM Password Recovery Requested
Important
Please make the changes and use the "suggest changes" feature in lieu of comments. That way the changes can be accepted as a batch change.
Checklist
bug
,enhancement
,schema
,maintenance
,Rule: New
,Rule: Deprecation
,Rule: Tuning
,Hunt: New
, orHunt: Tuning
so guidelines can be generatedmeta:rapid-merge
label if planning to merge within 24 hours