Skip to content

Commit a46b954

Browse files
kibanamachinemarshallmain
kibanamachine
and
marshallmain
authored
[8.x] [Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868) (#200086)
# Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868)](#198868) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Marshall Main","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-13T20:01:18Z","message":"[Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868)\n\n## Summary\r\n\r\nFixes https://github.com/elastic/kibana/issues/198461\r\n\r\nWhen a rule import file has both single-namespace and namespace-agnostic\r\nexception lists, there was a bug in the logic that fetched the existing\r\nexception lists after importing them. A missing set of parentheses\r\ncaused a KQL query that should have read `(A OR B) AND (C OR D)` to be\r\n`(A OR B) AND C OR D`, meaning that the logic was satisfied by `D` alone\r\ninstead of requiring `A` or `B` to be true along with `D`. In this case\r\n`A` and `B` are filters on `exception-list` and\r\n`exception-list-agnostic` SO attributes so that we (should) only be\r\nlooking at the list container objects, i.e.\r\n`exception-list.attributes.list_type: list`. `C` and `D` are filters by\r\n`list_id`, e.g. `exception-list.attributes.list_id: (test_list_id)`.\r\nWithout the extra parentheses around `C OR D`, the query finds both\r\n`list` and `item` documents for the list IDs specified in `D`.\r\n\r\nWhen the `findExceptionList` logic encounters a list item unexpectedly,\r\nit still tries to convert the SO into our internal representation of an\r\nexception list with `transformSavedObjectToExceptionList`. Most fields\r\nare shared between lists and items, which makes it confusing to debug.\r\nHowever, the `type` of items can only be `simple`, whereas lists have a\r\nvariety of types. During the conversion, the `type` field of the\r\nresulting object is defaulted to `detection` if the `type` field of the\r\nSO doesn't match the allowed list type values. Since the related SDH\r\ninvolved importing a `rule_default` exception list instead, the list\r\ntypes didn't match up when the import route compared the exception list\r\non the rule to import vs the \"existing list\" (which was actually a list\r\nitem coerced into a list container schema with `type: detection`) and\r\nimport fails.","sha":"0cc2e5677b46393ffd066ddaa1c548c664af311b","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","sdh-linked","backport:prev-major","v8.17.0"],"title":"[Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists","number":198868,"url":"https://github.com/elastic/kibana/pull/198868","mergeCommit":{"message":"[Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868)\n\n## Summary\r\n\r\nFixes https://github.com/elastic/kibana/issues/198461\r\n\r\nWhen a rule import file has both single-namespace and namespace-agnostic\r\nexception lists, there was a bug in the logic that fetched the existing\r\nexception lists after importing them. A missing set of parentheses\r\ncaused a KQL query that should have read `(A OR B) AND (C OR D)` to be\r\n`(A OR B) AND C OR D`, meaning that the logic was satisfied by `D` alone\r\ninstead of requiring `A` or `B` to be true along with `D`. In this case\r\n`A` and `B` are filters on `exception-list` and\r\n`exception-list-agnostic` SO attributes so that we (should) only be\r\nlooking at the list container objects, i.e.\r\n`exception-list.attributes.list_type: list`. `C` and `D` are filters by\r\n`list_id`, e.g. `exception-list.attributes.list_id: (test_list_id)`.\r\nWithout the extra parentheses around `C OR D`, the query finds both\r\n`list` and `item` documents for the list IDs specified in `D`.\r\n\r\nWhen the `findExceptionList` logic encounters a list item unexpectedly,\r\nit still tries to convert the SO into our internal representation of an\r\nexception list with `transformSavedObjectToExceptionList`. Most fields\r\nare shared between lists and items, which makes it confusing to debug.\r\nHowever, the `type` of items can only be `simple`, whereas lists have a\r\nvariety of types. During the conversion, the `type` field of the\r\nresulting object is defaulted to `detection` if the `type` field of the\r\nSO doesn't match the allowed list type values. Since the related SDH\r\ninvolved importing a `rule_default` exception list instead, the list\r\ntypes didn't match up when the import route compared the exception list\r\non the rule to import vs the \"existing list\" (which was actually a list\r\nitem coerced into a list container schema with `type: detection`) and\r\nimport fails.","sha":"0cc2e5677b46393ffd066ddaa1c548c664af311b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198868","number":198868,"mergeCommit":{"message":"[Security Solution][Detection Engine] Fix importing rules with multiple types of exception lists (#198868)\n\n## Summary\r\n\r\nFixes https://github.com/elastic/kibana/issues/198461\r\n\r\nWhen a rule import file has both single-namespace and namespace-agnostic\r\nexception lists, there was a bug in the logic that fetched the existing\r\nexception lists after importing them. A missing set of parentheses\r\ncaused a KQL query that should have read `(A OR B) AND (C OR D)` to be\r\n`(A OR B) AND C OR D`, meaning that the logic was satisfied by `D` alone\r\ninstead of requiring `A` or `B` to be true along with `D`. In this case\r\n`A` and `B` are filters on `exception-list` and\r\n`exception-list-agnostic` SO attributes so that we (should) only be\r\nlooking at the list container objects, i.e.\r\n`exception-list.attributes.list_type: list`. `C` and `D` are filters by\r\n`list_id`, e.g. `exception-list.attributes.list_id: (test_list_id)`.\r\nWithout the extra parentheses around `C OR D`, the query finds both\r\n`list` and `item` documents for the list IDs specified in `D`.\r\n\r\nWhen the `findExceptionList` logic encounters a list item unexpectedly,\r\nit still tries to convert the SO into our internal representation of an\r\nexception list with `transformSavedObjectToExceptionList`. Most fields\r\nare shared between lists and items, which makes it confusing to debug.\r\nHowever, the `type` of items can only be `simple`, whereas lists have a\r\nvariety of types. During the conversion, the `type` field of the\r\nresulting object is defaulted to `detection` if the `type` field of the\r\nSO doesn't match the allowed list type values. Since the related SDH\r\ninvolved importing a `rule_default` exception list instead, the list\r\ntypes didn't match up when the import route compared the exception list\r\non the rule to import vs the \"existing list\" (which was actually a list\r\nitem coerced into a list container schema with `type: detection`) and\r\nimport fails.","sha":"0cc2e5677b46393ffd066ddaa1c548c664af311b"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Marshall Main <[email protected]>
1 parent 85617c6 commit a46b954

File tree

5 files changed

+91
-55
lines changed

5 files changed

+91
-55
lines changed

‎x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ describe('getExceptionListFilter', () => {
2222
savedObjectTypes: ['exception-list-agnostic'],
2323
});
2424
expect(filter).toEqual(
25-
'(exception-list-agnostic.attributes.list_type: list) AND exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"'
25+
'(exception-list-agnostic.attributes.list_type: list) AND (exception-list-agnostic.attributes.name: "Sample Endpoint Exception List")'
2626
);
2727
});
2828

@@ -40,7 +40,7 @@ describe('getExceptionListFilter', () => {
4040
savedObjectTypes: ['exception-list'],
4141
});
4242
expect(filter).toEqual(
43-
'(exception-list.attributes.list_type: list) AND exception-list.attributes.name: "Sample Endpoint Exception List"'
43+
'(exception-list.attributes.list_type: list) AND (exception-list.attributes.name: "Sample Endpoint Exception List")'
4444
);
4545
});
4646

@@ -56,11 +56,12 @@ describe('getExceptionListFilter', () => {
5656

5757
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', () => {
5858
const filter = getExceptionListFilter({
59-
filter: 'exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"',
59+
filter:
60+
'exception-list-agnostic.attributes.name: "Sample Endpoint Exception List" OR exception-list.attributes.name: "Sample Rule Exception List"',
6061
savedObjectTypes: ['exception-list-agnostic', 'exception-list'],
6162
});
6263
expect(filter).toEqual(
63-
'(exception-list-agnostic.attributes.list_type: list OR exception-list.attributes.list_type: list) AND exception-list-agnostic.attributes.name: "Sample Endpoint Exception List"'
64+
'(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")'
6465
);
6566
});
6667
});

‎x-pack/plugins/lists/server/services/exception_lists/utils/get_exception_list_filter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,6 @@ export const getExceptionListFilter = ({
2020
.join(' OR ');
2121

2222
if (filter != null) {
23-
return `(${listTypesFilter}) AND ${filter}`;
23+
return `(${listTypesFilter}) AND (${filter})`;
2424
} else return `(${listTypesFilter})`;
2525
};

‎x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ describe('find_all_exception_list_item_types', () => {
6060

6161
expect(findExceptionList).toHaveBeenCalledWith({
6262
filter: 'exception-list-agnostic.attributes.list_id:(1)',
63-
namespaceType: ['agnostic'],
63+
namespaceType: ['single', 'agnostic'],
6464
page: undefined,
6565
perPage: 1000,
6666
savedObjectsClient,
@@ -74,7 +74,7 @@ describe('find_all_exception_list_item_types', () => {
7474

7575
expect(findExceptionList).toHaveBeenCalledWith({
7676
filter: 'exception-list.attributes.list_id:(1)',
77-
namespaceType: ['single'],
77+
namespaceType: ['single', 'agnostic'],
7878
page: undefined,
7979
perPage: 1000,
8080
savedObjectsClient,

‎x-pack/plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts‎

Lines changed: 31 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -52,57 +52,40 @@ export const findAllListTypes = async (
5252
nonAgnosticListItems: ExceptionListQueryInfo[],
5353
savedObjectsClient: SavedObjectsClientContract
5454
): Promise<FoundExceptionListSchema | null> => {
55-
// Agnostic filter
56-
const agnosticFilter = getListFilter({
57-
namespaceType: 'agnostic',
58-
objects: agnosticListItems,
59-
});
60-
61-
// Non-agnostic filter
62-
const nonAgnosticFilter = getListFilter({
63-
namespaceType: 'single',
64-
objects: nonAgnosticListItems,
65-
});
66-
6755
if (!agnosticListItems.length && !nonAgnosticListItems.length) {
6856
return null;
69-
} else if (agnosticListItems.length && !nonAgnosticListItems.length) {
70-
return findExceptionList({
71-
filter: agnosticFilter,
72-
namespaceType: ['agnostic'],
73-
page: undefined,
74-
perPage: CHUNK_PARSED_OBJECT_SIZE,
75-
pit: undefined,
76-
savedObjectsClient,
77-
searchAfter: undefined,
78-
sortField: undefined,
79-
sortOrder: undefined,
80-
});
81-
} else if (!agnosticListItems.length && nonAgnosticListItems.length) {
82-
return findExceptionList({
83-
filter: nonAgnosticFilter,
84-
namespaceType: ['single'],
85-
page: undefined,
86-
perPage: CHUNK_PARSED_OBJECT_SIZE,
87-
pit: undefined,
88-
savedObjectsClient,
89-
searchAfter: undefined,
90-
sortField: undefined,
91-
sortOrder: undefined,
92-
});
93-
} else {
94-
return findExceptionList({
95-
filter: `${agnosticFilter} OR ${nonAgnosticFilter}`,
96-
namespaceType: ['single', 'agnostic'],
97-
page: undefined,
98-
perPage: CHUNK_PARSED_OBJECT_SIZE,
99-
pit: undefined,
100-
savedObjectsClient,
101-
searchAfter: undefined,
102-
sortField: undefined,
103-
sortOrder: undefined,
104-
});
10557
}
58+
59+
const filters: string[] = [];
60+
if (agnosticListItems.length > 0) {
61+
filters.push(
62+
getListFilter({
63+
namespaceType: 'agnostic',
64+
objects: agnosticListItems,
65+
})
66+
);
67+
}
68+
69+
if (nonAgnosticListItems.length > 0) {
70+
filters.push(
71+
getListFilter({
72+
namespaceType: 'single',
73+
objects: nonAgnosticListItems,
74+
})
75+
);
76+
}
77+
78+
return findExceptionList({
79+
filter: filters.join(' OR '),
80+
namespaceType: ['single', 'agnostic'],
81+
page: undefined,
82+
perPage: CHUNK_PARSED_OBJECT_SIZE,
83+
pit: undefined,
84+
savedObjectsClient,
85+
searchAfter: undefined,
86+
sortField: undefined,
87+
sortOrder: undefined,
88+
});
10689
};
10790

10891
/**

‎x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,6 +1215,58 @@ export default ({ getService }: FtrProviderContext): void => {
12151215
});
12161216
});
12171217

1218+
it('should be able to import a rule with both single space and space agnostic exception lists', async () => {
1219+
const ndjson = combineToNdJson(
1220+
getCustomQueryRuleParams({
1221+
exceptions_list: [
1222+
{
1223+
id: 'agnostic',
1224+
list_id: 'test_list_agnostic_id',
1225+
type: 'detection',
1226+
namespace_type: 'agnostic',
1227+
},
1228+
{
1229+
id: 'single',
1230+
list_id: 'test_list_id',
1231+
type: 'rule_default',
1232+
namespace_type: 'single',
1233+
},
1234+
],
1235+
}),
1236+
{ ...getImportExceptionsListSchemaMock('test_list_id'), type: 'rule_default' },
1237+
getImportExceptionsListItemNewerVersionSchemaMock('test_item_id', 'test_list_id'),
1238+
{
1239+
...getImportExceptionsListSchemaMock('test_list_agnostic_id'),
1240+
type: 'detection',
1241+
namespace_type: 'agnostic',
1242+
},
1243+
{
1244+
...getImportExceptionsListItemNewerVersionSchemaMock(
1245+
'test_item_id',
1246+
'test_list_agnostic_id'
1247+
),
1248+
namespace_type: 'agnostic',
1249+
}
1250+
);
1251+
1252+
const { body } = await supertest
1253+
.post(`${DETECTION_ENGINE_RULES_URL}/_import`)
1254+
.set('kbn-xsrf', 'true')
1255+
.set('elastic-api-version', '2023-10-31')
1256+
.attach('file', Buffer.from(ndjson), 'rules.ndjson')
1257+
.expect(200);
1258+
1259+
expect(body).toMatchObject({
1260+
success: true,
1261+
success_count: 1,
1262+
rules_count: 1,
1263+
errors: [],
1264+
exceptions_errors: [],
1265+
exceptions_success: true,
1266+
exceptions_success_count: 2,
1267+
});
1268+
});
1269+
12181270
it('should only remove non existent exception list references from rule', async () => {
12191271
// create an exception list
12201272
const { body: exceptionBody } = await supertest

0 commit comments

Comments
 (0)