From 72d3a9e44972a7ab29e58015fde059ded2c2f38d Mon Sep 17 00:00:00 2001 From: Marshall Main <55718608+marshallmain@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:01:18 -0500 Subject: [PATCH] [Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868) ## Summary Fixes https://github.com/elastic/kibana/issues/198461 When a rule import file has both single-namespace and namespace-agnostic exception lists, there was a bug in the logic that fetched the existing exception lists after importing them. A missing set of parentheses caused a KQL query that should have read `(A OR B) AND (C OR D)` to be `(A OR B) AND C OR D`, meaning that the logic was satisfied by `D` alone instead of requiring `A` or `B` to be true along with `D`. In this case `A` and `B` are filters on `exception-list` and `exception-list-agnostic` SO attributes so that we (should) only be looking at the list container objects, i.e. `exception-list.attributes.list_type: list`. `C` and `D` are filters by `list_id`, e.g. `exception-list.attributes.list_id: (test_list_id)`. Without the extra parentheses around `C OR D`, the query finds both `list` and `item` documents for the list IDs specified in `D`. When the `findExceptionList` logic encounters a list item unexpectedly, it still tries to convert the SO into our internal representation of an exception list with `transformSavedObjectToExceptionList`. Most fields are shared between lists and items, which makes it confusing to debug. However, the `type` of items can only be `simple`, whereas lists have a variety of types. During the conversion, the `type` field of the resulting object is defaulted to `detection` if the `type` field of the SO doesn't match the allowed list type values. Since the related SDH involved importing a `rule_default` exception list instead, the list types didn't match up when the import route compared the exception list on the rule to import vs the "existing list" (which was actually a list item coerced into a list container schema with `type: detection`) and import fails. (cherry picked from commit 0cc2e5677b46393ffd066ddaa1c548c664af311b) --- .../utils/get_exception_list_filter.test.ts | 9 ++- .../utils/get_exception_list_filter.ts | 2 +- .../find_all_exception_list_types.test.ts | 4 +- .../import/find_all_exception_list_types.ts | 79 ++++++++----------- .../import_rules.ts | 52 ++++++++++++ 5 files changed, 91 insertions(+), 55 deletions(-) diff --git a/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.test.ts b/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.test.ts index 9e0b06f8482c..676df7cdf09f 100644 --- a/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.test.ts +++ b/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.test.ts @@ -22,7 +22,7 @@ describe('getExceptionListFilter', () => { savedObjectTypes: ['exception-list-agnostic'], }); expect(filter).toEqual( - '(exception-list-agnostic.attributes.list_type: list) AND exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"' + '(exception-list-agnostic.attributes.list_type: list) AND (exception-list-agnostic.attributes.name: "Sample Endpoint Exception List")' ); }); @@ -40,7 +40,7 @@ describe('getExceptionListFilter', () => { savedObjectTypes: ['exception-list'], }); expect(filter).toEqual( - '(exception-list.attributes.list_type: list) AND exception-list.attributes.name: "Sample Endpoint Exception List"' + '(exception-list.attributes.list_type: list) AND (exception-list.attributes.name: "Sample Endpoint Exception List")' ); }); @@ -56,11 +56,12 @@ describe('getExceptionListFilter', () => { test('it should create a filter that searches for both agnostic and single lists with additional filters if searching for both single and agnostic lists', () => { const filter = getExceptionListFilter({ - filter: 'exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"', + filter: + 'exception-list-agnostic.attributes.name: "Sample Endpoint Exception List" OR exception-list.attributes.name: "Sample Rule Exception List"', savedObjectTypes: ['exception-list-agnostic', 'exception-list'], }); expect(filter).toEqual( - '(exception-list-agnostic.attributes.list_type: list OR exception-list.attributes.list_type: list) AND exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"' + '(exception-list-agnostic.attributes.list_type: list OR exception-list.attributes.list_type: list) AND (exception-list-agnostic.attributes.name: "Sample Endpoint Exception List" OR exception-list.attributes.name: "Sample Rule Exception List")' ); }); }); diff --git a/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.ts b/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.ts index 44a9be320755..8aedaa13bcab 100644 --- a/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.ts +++ b/x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.ts @@ -20,6 +20,6 @@ export const getExceptionListFilter = ({ .join(' OR '); if (filter != null) { - return `(${listTypesFilter}) AND ${filter}`; + return `(${listTypesFilter}) AND (${filter})`; } else return `(${listTypesFilter})`; }; diff --git a/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.test.ts b/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.test.ts index 292455db7a17..4e309afe0313 100644 --- a/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.test.ts +++ b/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.test.ts @@ -60,7 +60,7 @@ describe('find_all_exception_list_item_types', () => { expect(findExceptionList).toHaveBeenCalledWith({ filter: 'exception-list-agnostic.attributes.list_id:(1)', - namespaceType: ['agnostic'], + namespaceType: ['single', 'agnostic'], page: undefined, perPage: 100, savedObjectsClient, @@ -74,7 +74,7 @@ describe('find_all_exception_list_item_types', () => { expect(findExceptionList).toHaveBeenCalledWith({ filter: 'exception-list.attributes.list_id:(1)', - namespaceType: ['single'], + namespaceType: ['single', 'agnostic'], page: undefined, perPage: 100, savedObjectsClient, diff --git a/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts b/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts index 870acc6cc446..93c5491a84dd 100644 --- a/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts +++ b/x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts @@ -52,57 +52,40 @@ export const findAllListTypes = async ( nonAgnosticListItems: ExceptionListQueryInfo[], savedObjectsClient: SavedObjectsClientContract ): Promise => { - // Agnostic filter - const agnosticFilter = getListFilter({ - namespaceType: 'agnostic', - objects: agnosticListItems, - }); - - // Non-agnostic filter - const nonAgnosticFilter = getListFilter({ - namespaceType: 'single', - objects: nonAgnosticListItems, - }); - if (!agnosticListItems.length && !nonAgnosticListItems.length) { return null; - } else if (agnosticListItems.length && !nonAgnosticListItems.length) { - return findExceptionList({ - filter: agnosticFilter, - namespaceType: ['agnostic'], - page: undefined, - perPage: CHUNK_PARSED_OBJECT_SIZE, - pit: undefined, - savedObjectsClient, - searchAfter: undefined, - sortField: undefined, - sortOrder: undefined, - }); - } else if (!agnosticListItems.length && nonAgnosticListItems.length) { - return findExceptionList({ - filter: nonAgnosticFilter, - namespaceType: ['single'], - page: undefined, - perPage: CHUNK_PARSED_OBJECT_SIZE, - pit: undefined, - savedObjectsClient, - searchAfter: undefined, - sortField: undefined, - sortOrder: undefined, - }); - } else { - return findExceptionList({ - filter: `${agnosticFilter} OR ${nonAgnosticFilter}`, - namespaceType: ['single', 'agnostic'], - page: undefined, - perPage: CHUNK_PARSED_OBJECT_SIZE, - pit: undefined, - savedObjectsClient, - searchAfter: undefined, - sortField: undefined, - sortOrder: undefined, - }); } + + const filters: string[] = []; + if (agnosticListItems.length > 0) { + filters.push( + getListFilter({ + namespaceType: 'agnostic', + objects: agnosticListItems, + }) + ); + } + + if (nonAgnosticListItems.length > 0) { + filters.push( + getListFilter({ + namespaceType: 'single', + objects: nonAgnosticListItems, + }) + ); + } + + return findExceptionList({ + filter: filters.join(' OR '), + namespaceType: ['single', 'agnostic'], + page: undefined, + perPage: CHUNK_PARSED_OBJECT_SIZE, + pit: undefined, + savedObjectsClient, + searchAfter: undefined, + sortField: undefined, + sortOrder: undefined, + }); }; /** diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts index b40b5b785ca5..279e45f6cda5 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts @@ -1213,6 +1213,58 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('should be able to import a rule with both single space and space agnostic exception lists', async () => { + const ndjson = combineToNdJson( + getCustomQueryRuleParams({ + exceptions_list: [ + { + id: 'agnostic', + list_id: 'test_list_agnostic_id', + type: 'detection', + namespace_type: 'agnostic', + }, + { + id: 'single', + list_id: 'test_list_id', + type: 'rule_default', + namespace_type: 'single', + }, + ], + }), + { ...getImportExceptionsListSchemaMock('test_list_id'), type: 'rule_default' }, + getImportExceptionsListItemNewerVersionSchemaMock('test_item_id', 'test_list_id'), + { + ...getImportExceptionsListSchemaMock('test_list_agnostic_id'), + type: 'detection', + namespace_type: 'agnostic', + }, + { + ...getImportExceptionsListItemNewerVersionSchemaMock( + 'test_item_id', + 'test_list_agnostic_id' + ), + namespace_type: 'agnostic', + } + ); + + const { body } = await supertest + .post(`${DETECTION_ENGINE_RULES_URL}/_import`) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .attach('file', Buffer.from(ndjson), 'rules.ndjson') + .expect(200); + + expect(body).toMatchObject({ + success: true, + success_count: 1, + rules_count: 1, + errors: [], + exceptions_errors: [], + exceptions_success: true, + exceptions_success_count: 2, + }); + }); + it('should only remove non existent exception list references from rule', async () => { // create an exception list const { body: exceptionBody } = await supertest