Skip to content

Commit

Permalink
[8.16] [Security Solution][Detection Engine] Fix importing rules with…
Browse files Browse the repository at this point in the history
… multiple types of exception lists (#198868) (#200085)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[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":"55718608+marshallmain@users.noreply.github.com"},"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 <55718608+marshallmain@users.noreply.github.com>
  • Loading branch information
kibanamachine and marshallmain authored Nov 13, 2024
1 parent ea1af26 commit f60eaef
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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")'
);
});

Expand All @@ -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")'
);
});

Expand All @@ -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")'
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ export const getExceptionListFilter = ({
.join(' OR ');

if (filter != null) {
return `(${listTypesFilter}) AND ${filter}`;
return `(${listTypesFilter}) AND (${filter})`;
} else return `(${listTypesFilter})`;
};
Original file line number Diff line number Diff line change
Expand Up @@ -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: 1000,
savedObjectsClient,
Expand All @@ -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: 1000,
savedObjectsClient,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,57 +52,40 @@ export const findAllListTypes = async (
nonAgnosticListItems: ExceptionListQueryInfo[],
savedObjectsClient: SavedObjectsClientContract
): Promise<FoundExceptionListSchema | null> => {
// 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,
});
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1215,6 +1215,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
Expand Down

0 comments on commit f60eaef

Please sign in to comment.