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

[SIEM][Detection Engine] - Update DE to work with new exceptions schema #69715

Merged
merged 10 commits into from
Jun 25, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Jun 23, 2020

Summary

The purpose of this PR is to update the detection engine to work with the new exception list schema. Previously, exceptions lived directly in the rule, now they are stored separately, and the rule only stores a reference to said exception lists.

Changes are concentrated in two areas. First x-pack/plugins/lists:

  • Updates the EntryList schema so it now takes a list id and type (as needed for the detections engine logic) as opposed to before where it was just a string for the id
  • Updates the lists plugin unit tests to account for the updated EntryList schema
  • Exposes the exceptions list client (in the lists plugin) to be used by the detection engine

Second in x-pack/plugins/security_solution:

  • Updates the detection engine exceptions_list schema. Previously, exceptions sat directly on the rule itself, now that exceptions are stored separately, the exceptions_list property needs to only keep reference to the exceptions list
  • Updates schema unit tests
  • Updates routes to use new exceptions list schema
  • Updates route unit tests
  • Removes old exception list scripts that are no longer necessary

To Do

Right now we have logic in build_exceptions_query.ts that takes exceptions that don't include large value lists and appends them to the main KQL query which is then translated into the elastic query dsl. We also have logic in filter_events_wtih_list.ts that uses inverse logic to filter events search results against any exceptions with large value lists. On their own, each of these does what is expected. However, after all expansion of the exceptions list functionality, it is clear that these two separate pieces of logic would not suffice.

This PR does not address that issue (it's in a follow up PR), but thought it worth noting here. It gets confusing with the boolean logic, but to try to simplify the issue we came upon with our existing solution is the following:

Imagine we had a rule, with exceptions_list: [{ id: 'exception_list', namespace_type: 'single'}]. The exception_list has 2 exception list items:

// exception list item 1
a && b && c(large value list)

// exception list item 2
d && e(large value list)

The initial search would query (query && a && b) || (query && d). Then the results of that search would get filtered through the large value lists logic, so it would filter for (c || e). What this would result in, is signals that were filtered for:

(query && a && b) || (query && d) || c || e

when what we want is:

(query && a && b && c) || (query && d && e)

These are not logically equivalent. Our current logic would work if there was a single exception item, but as soon as there is more than one as shown in the simplified example above, the logic breaks down. This issue is being addressed in another PR.

Testing

To turn on lists plugin - in kibana.dev.yml

# Enable lists feature
xpack.lists.enabled: true
xpack.lists.listIndex: '.lists-yara'
xpack.lists.listItemIndex: '.items-yara'

Add export ELASTIC_XPACK_SIEM_LISTS_FEATURE=true to your bash file.

Use the scripts in x-pack/plugins/lists/server/scripts to create some sample exception lists and items. You can use the following:

If you've previously played around with lists, run ./hard_reset.sh (this will delete any lists you've created).

Create large value list:

  • Create large value list ./post_list.sh
  • Create large value list item ./post_list_item.sh (I modified the value to be "value": "10.4.2.140")

Create exception list:

  • Create exception list ./post_exception_list.sh
  • Create exception list item ./post_exception_list_item.sh ./exception_lists/new/exception_list_item_with_list.json

Use the scripts in x-pack/plugins/security_solution/server/lib/detection_engine/scripts to create rule:

  • Run ./post_rule.sh ./rules/queries/query_with_list.json (Makes reference to the exception list created in step above)

In the Alerts table, you should see something like the following where you only see events where the event.module is zeek and source.ip is 10.4.2.140 (or whatever ip you specified).
Screen Shot 2020-06-23 at 11 19 07 AM

Checklist

For maintainers

@@ -7,7 +7,7 @@
import * as t from 'io-ts';
import { Either } from 'fp-ts/lib/Either';

const namespaceType = t.keyof({ agnostic: null, single: null });
export const namespaceType = t.keyof({ agnostic: null, single: null });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Exported to use in detection engine.

@@ -52,7 +52,7 @@ export type EntryExists = t.TypeOf<typeof entriesExists>;

export const entriesNested = t.exact(
t.type({
entries: t.array(t.union([entriesMatch, entriesMatchAny, entriesList, entriesExists])),
entries: t.array(entriesMatch),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Looking at KQL nested support, looks like it would only support our match and not any of the other operators. Please feel free to comment if that's wrong.

@@ -34,9 +34,9 @@ export type EntryMatchAny = t.TypeOf<typeof entriesMatchAny>;
export const entriesList = t.exact(
t.type({
field: t.string,
list: t.exact(t.type({ id: t.string, type })),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Changed this to list from value, SOs were screaming when trying to index value as string | string[] | object so updated.

@@ -87,6 +88,16 @@ export const getFormattedEntries = (entries: EntriesArray): FormattedEntry[] =>
return formattedEntries.flat();
};

export const getEntryValue = (entry: Entry): string | string[] | null => {
if (entriesList.is(entry)) {
return entry.list.id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This is temporary, didn't want to dig into changing UI code in this PR that's already large. Thought maybe we wanted to add name to the the EntryList for ease of use here, but that would also then require any updates to large lists to search for references to it in exception list items.

@@ -140,6 +136,18 @@ export const signalRulesAlertType = ({
await ruleStatusService.error(gapMessage, { gap: gapString });
}
try {
const { listClient, exceptionsClient } = await getListsClient({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Now needing to load exception items, where as before they lived on the rule.

@@ -214,18 +222,6 @@ export const signalRulesAlertType = ({
result.bulkCreateTimes.push(bulkCreateDuration);
}
} else {
let listClient: ListClient | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Moved into util seen above.

@yctercero yctercero self-assigned this Jun 23, 2020
@yctercero yctercero marked this pull request as ready for review June 23, 2020 17:38
@yctercero yctercero requested review from a team as code owners June 23, 2020 17:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Check out, tested locally and reviewed code -- LGMT! 👍

And many thanks for all the added tests here as well @yctercero! 🙂

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yctercero yctercero merged commit f7acbbe into elastic:master Jun 25, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Jun 25, 2020
…ma (elastic#69715)

* Updates list entry schema, exposes exception list client, updates tests

* create new de list schema and unit tests

* updated route unit tests and types to match new list schema

* updated existing DE exceptions code so it should now work as is with updated schema

* test and types cleanup

* cleanup

* update unit test

* updates per feedback
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 25, 2020
* master: (45 commits)
  [QA] Unskip functional tests (elastic#69760)
  [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715)
  Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863)
  PR: Provide limit warnings to user when API limits are reached. (elastic#69590)
  [Maps] Remove broken button (elastic#69853)
  Makes usage collection methods available on start (elastic#69836)
  [SIEM][CASE] Improve Jira's labelling (elastic#69892)
  [Logs UI] Access ML via the programmatic plugin API (elastic#68905)
  [ML] DF Analytics: Creation wizard part 3 (elastic#69456)
  Update Resolver generator script documentation (elastic#69912)
  [ML] Changes View results button text on new job page (elastic#69809)
  Add master branch to backport config (elastic#69893)
  [Ingest Manager] Kibana, not EPR, controls removable packages (elastic#69761)
  unskips 'Events columns' test (elastic#69684)
  [ML] Changes the ML overview empty analytics panel text (elastic#69801)
  [DOCS] Emphasizes where File Data Visualizer is located. (elastic#69812)
  add the `exactRoute` property to app registration (elastic#69772)
  Bump backport to 5.4.6 (elastic#69880)
  [Logs UI] ML log integration splash screen (elastic#69288)
  Clean up TSVB type client code to conform to the schema (elastic#68519)
  ...
yctercero added a commit that referenced this pull request Jun 25, 2020
…ma (#69715) (#69932)

## Summary

The purpose of this PR is to update the detection engine to work with the new exception list schema. Previously, exceptions lived directly in the rule, now they are stored separately, and the rule only stores a reference to said exception lists.

Changes are concentrated in two areas. First `x-pack/plugins/lists`:
- Updates the `EntryList` schema so it now takes a list `id` and `type` (as needed for the detections engine logic) as opposed to before where it was just a string for the `id`
- Updates the lists plugin unit tests to account for the updated `EntryList` schema
- Exposes the exceptions list client (in the lists plugin) to be used by the detection engine

Second in `x-pack/plugins/security_solution`:
- Updates the detection engine `exceptions_list` schema. Previously, exceptions sat directly on the rule itself, now that exceptions are stored separately, the `exceptions_list` property needs to only keep reference to the exceptions list
- Updates schema unit tests
- Updates routes to use new exceptions list schema
- Updates route unit tests
- Removes old exception list scripts that are no longer necessary
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 25, 2020
* master: (90 commits)
  [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513)
  [SIEM] Replace WithSource with useWithSource hook (elastic#68722)
  [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708)
  rename old siem kibana config to securitySolution (elastic#69874)
  Remove unused Resolver code (elastic#69914)
  [Observability] Fixing dynamic return type based on the appName (elastic#69894)
  [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419)
  Fixes special clicks and 3rd party icon sizes in nav (elastic#69767)
  [APM] Catch annotations index permission error and log warning (elastic#69881)
  [Endpoint][Ingest Manager] minor code cleanup (elastic#69844)
  [Logs UI] Logs ui context menu (elastic#69915)
  Index pattern serialize and de-serialize (elastic#68844)
  [QA] Unskip functional tests (elastic#69760)
  [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715)
  Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863)
  PR: Provide limit warnings to user when API limits are reached. (elastic#69590)
  [Maps] Remove broken button (elastic#69853)
  Makes usage collection methods available on start (elastic#69836)
  [SIEM][CASE] Improve Jira's labelling (elastic#69892)
  [Logs UI] Access ML via the programmatic plugin API (elastic#68905)
  ...
@yctercero yctercero deleted the de_lists branch October 14, 2020 12:00
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants