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.

Sign up for GitHub

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

[Security Solutions][Detection Engine] Critical bug where value lists were not operational #80368

Merged
merged 5 commits into from
Oct 14, 2020
Merged

[Security Solutions][Detection Engine] Critical bug where value lists were not operational #80368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
538c71d
Fixes things to work with value lists if it is the only index being p…
FrankHassanabad Oct 13, 2020
ffddcb7
Merge branch 'master' into make-value-lists-work
FrankHassanabad Oct 13, 2020
c76d3c4
Added unit tests
FrankHassanabad Oct 13, 2020
d509e54
Fixed wording
FrankHassanabad Oct 13, 2020
db6202a
Merge branch 'master' into make-value-lists-work
FrankHassanabad Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion x-pack/plugins/lists/server/scripts/lists/files/hosts.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
kibana
siem-kibana
siem-windows
rock01
1 change: 1 addition & 0 deletions ...urity_solution/server/lib/detection_engine/signals/threat_mapping/create_threat_signal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const createThreatSignal = async ({
searchAfter: currentThreatList.hits.hits[currentThreatList.hits.hits.length - 1].sort,
sortField: undefined,
sortOrder: undefined,
listClient,
});

const threatFilter = buildThreatMappingFilter({
Expand Down
1 change: 1 addition & 0 deletions ...rity_solution/server/lib/detection_engine/signals/threat_mapping/create_threat_signals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const createThreatSignals = async ({
query: threatQuery,
language: threatLanguage,
index: threatIndex,
listClient,
searchAfter: undefined,
sortField: undefined,
sortOrder: undefined,
Expand Down
68 changes: 64 additions & 4 deletions ...urity_solution/server/lib/detection_engine/signals/threat_mapping/get_threat_list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,83 @@ import { getSortWithTieBreaker } from './get_threat_list';
describe('get_threat_signals', () => {
describe('getSortWithTieBreaker', () => {
test('it should return sort field of just timestamp if given no sort order', () => {
const sortOrder = getSortWithTieBreaker({ sortField: undefined, sortOrder: undefined });
const sortOrder = getSortWithTieBreaker({
sortField: undefined,
sortOrder: undefined,
index: ['index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ '@timestamp': 'asc' }]);
});

test('it should return sort field of just tie_breaker_id if given no sort order for a list item index', () => {
const sortOrder = getSortWithTieBreaker({
sortField: undefined,
sortOrder: undefined,
index: ['list-item-index-123'],
listItemIndex: 'list-item-index-123',
});
expect(sortOrder).toEqual([{ tie_breaker_id: 'asc' }]);
});

test('it should return sort field of timestamp with asc even if sortOrder is changed as it is hard wired in', () => {
const sortOrder = getSortWithTieBreaker({ sortField: undefined, sortOrder: 'desc' });
const sortOrder = getSortWithTieBreaker({
sortField: undefined,
sortOrder: 'desc',
index: ['index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ '@timestamp': 'asc' }]);
});

test('it should return sort field of tie_breaker_id with asc even if sortOrder is changed as it is hard wired in for a list item index', () => {
const sortOrder = getSortWithTieBreaker({
sortField: undefined,
sortOrder: 'desc',
index: ['list-index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ tie_breaker_id: 'asc' }]);
});

test('it should return sort field of an extra field if given one', () => {
const sortOrder = getSortWithTieBreaker({ sortField: 'some-field', sortOrder: undefined });
const sortOrder = getSortWithTieBreaker({
sortField: 'some-field',
sortOrder: undefined,
index: ['index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ 'some-field': 'asc', '@timestamp': 'asc' }]);
});

test('it should return sort field of an extra field if given one for a list item index', () => {
const sortOrder = getSortWithTieBreaker({
sortField: 'some-field',
sortOrder: undefined,
index: ['list-index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ 'some-field': 'asc', tie_breaker_id: 'asc' }]);
});

test('it should return sort field of desc if given one', () => {
const sortOrder = getSortWithTieBreaker({ sortField: 'some-field', sortOrder: 'desc' });
const sortOrder = getSortWithTieBreaker({
sortField: 'some-field',
sortOrder: 'desc',
index: ['index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ 'some-field': 'desc', '@timestamp': 'asc' }]);
});

test('it should return sort field of desc if given one for a list item index', () => {
const sortOrder = getSortWithTieBreaker({
sortField: 'some-field',
sortOrder: 'desc',
index: ['list-index-123'],
listItemIndex: 'list-index-123',
});
expect(sortOrder).toEqual([{ 'some-field': 'desc', tie_breaker_id: 'asc' }]);
});
});
});
32 changes: 28 additions & 4 deletions ...s/security_solution/server/lib/detection_engine/signals/threat_mapping/get_threat_list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const getThreatList = async ({
sortOrder,
exceptionItems,
threatFilters,
listClient,
}: GetThreatListOptions): Promise<SearchResponse<ThreatListItem>> => {
const calculatedPerPage = perPage ?? MAX_PER_PAGE;
if (calculatedPerPage > 10000) {
Expand All @@ -41,11 +42,17 @@ export const getThreatList = async ({
index,
exceptionItems
);

const response: SearchResponse<ThreatListItem> = await callCluster('search', {
body: {
query: queryFilter,
search_after: searchAfter,
sort: getSortWithTieBreaker({ sortField, sortOrder }),
sort: getSortWithTieBreaker({
sortField,
sortOrder,
index,
listItemIndex: listClient.getListItemIndex(),
}),
},
ignoreUnavailable: true,
index,
Expand All @@ -54,14 +61,31 @@ export const getThreatList = async ({
return response;
};

/**
* This returns the sort with a tiebreaker if we find out we are only
* querying against the list items index. If we are querying against any
* other index we are assuming we are 1 or more ECS compatible indexes and
* will query against those indexes using just timestamp since we don't have
* a tiebreaker.
*/
export const getSortWithTieBreaker = ({
sortField,
sortOrder,
index,
listItemIndex,
}: GetSortWithTieBreakerOptions): SortWithTieBreaker[] => {
const ascOrDesc = sortOrder ?? 'asc';
if (sortField != null) {
return [{ [sortField]: ascOrDesc, '@timestamp': 'asc' }];
if (index.length === 1 && index[0] === listItemIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I initially created a rule against a threat index like .items-*, which does not satisfy this condition and thus still suffers from this issue. This is mentioned in the description as "single index" but I wanted to call it out again since it's easy to miss.

@FrankHassanabad do we want to add a note about this in the threat match rule documentation, or is too much of a "power user" scenario?

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's a bit of a "power user" scenario and a bit of a trick. We can support it nicely in the UI because we can query for the index list item name and populate it like it is here and then disallow additional indexes as if you choose to use a plain list one that's all you can choose since value based lists aren't ECS standards.

For API users they will have to know their space names ahead of time and kind of fill in the API so we might want to update at least the API docs before release time to show an example of how to do it or we might need to add a better API for when they have a mapping and want to refer to a value based list.

if (sortField != null) {
return [{ [sortField]: ascOrDesc, tie_breaker_id: 'asc' }];
} else {
return [{ tie_breaker_id: 'asc' }];
}
} else {
return [{ '@timestamp': 'asc' }];
if (sortField != null) {
return [{ [sortField]: ascOrDesc, '@timestamp': 'asc' }];
} else {
return [{ '@timestamp': 'asc' }];
}
}
};
4 changes: 3 additions & 1 deletion x-pack/plugins/security_solution/server/lib/detection_engine/signals/threat_mapping/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,14 @@ export interface GetThreatListOptions {
sortOrder: 'asc' | 'desc' | undefined;
threatFilters: PartialFilter[];
exceptionItems: ExceptionListItemSchema[];
listClient: ListClient;
}

export interface GetSortWithTieBreakerOptions {
sortField: string | undefined;
sortOrder: 'asc' | 'desc' | undefined;
index: string[];
listItemIndex: string;
}

/**
Expand All @@ -171,6 +174,5 @@ export interface ThreatListItem {
}

export interface SortWithTieBreaker {
'@timestamp': 'asc';
[key: string]: string;
}