Skip to content

Commit

Permalink
[Security Solutions][Detection Engine] Fixes critical clashing with s…
Browse files Browse the repository at this point in the history
…ource indexes that already contain a "signal" field (elastic#82191)

## Summary

Fixes: elastic#82148


We have errors and do not generate a signal when a source index already has utilized and reserved the "signal" field for their own data purposes. This fix is a bit tricky and has one medium sized risk which is we also support "signals generated on top of existing signals". Therefore we have to be careful and do a small runtime detection of the "data shape" of the signal's data type. If it looks like the user is using the "signal" field within their mapping instead of us, we move the customer's signal into "original_signal" inside our "signal" structure we create when we copy their data set when creating a signal.   

To help mitigate the risks associated with this critical bug with regards to breaking signals on top of signals I have:

* This adds unit tests
* This adds end to end tests for testing generating signals including signals on signals to help mitigate risk

The key test for this shape in the PR are in the file:

```
detection_engine/signals/build_event_type_signal.ts
```

like so:
```ts
export const isEventTypeSignal = (doc: BaseSignalHit): boolean => {
  return doc._source.signal?.rule?.id != null && typeof doc._source.signal?.rule?.id === 'string';
};
```
 
Example of what happens when it does a "move" of an existing numeric signal keyword type:

```ts
# This causes a clash with us using the name signal as a numeric.
PUT clashing-index/_doc/1
{
  "@timestamp": "2020-10-28T05:08:53.000Z",
  "signal": 1
}
```

Before, this was an error. With this PR it now will restructure this data like so when creating a signal along with additional signal ancestor information, meta data. I omitted some of the data from the output signal for this example. 

```ts
{
... Other data copied ...
"signal": 
{
    "original_signal": 1 <--- We "move it" here now
    "parents": 
    [
        {
            "id": "BhbXBmkBR346wHgn4PeZ",
            "type": "event",
            "index": "your-index-name",
            "depth": 0
        },
    ],
    "ancestors":
    [
        {
            "id": "BhbXBmkBR346wHgn4PeZ",
            "type": "event",
            "index": "your-index-name",
            "depth": 0
        },
    ],
    "status": "open",
    "depth": 1,
    "parent":
    {
        "id": "BhbXBmkBR346wHgn4PeZ",
        type: "event",
        "index": "your-index-name",
        "depth": 0
    },
    "original_time": "2019-02-19T17:40:03.790Z",
    "original_event": 
    {
        "action": "socket_closed",
        "dataset": "socket",
        "kind": "event",
        "module": "system"
    },
}

```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
  • Loading branch information
FrankHassanabad committed Nov 6, 2020
1 parent b8f2342 commit 3e1e65b
Show file tree
Hide file tree
Showing 17 changed files with 988 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@
"original_time": {
"type": "date"
},
"original_signal": {
"type": "object",
"dynamic": false,
"enabled": false
},
"original_event": {
"properties": {
"action": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
objectPairIntersection,
objectArrayIntersection,
} from './build_bulk_body';
import { SignalHit } from './types';
import { SignalHit, SignalSourceHit } from './types';
import { getListArrayMock } from '../../../../common/detection_engine/schemas/types/lists.mock';

describe('buildBulkBody', () => {
Expand Down Expand Up @@ -441,6 +441,206 @@ describe('buildBulkBody', () => {
};
expect(fakeSignalSourceHit).toEqual(expected);
});

test('bulk body builds "original_signal" if it exists already as a numeric', () => {
const sampleParams = sampleRuleAlertParams();
const sampleDoc = sampleDocNoSortId();
delete sampleDoc._source.source;
const doc = ({
...sampleDoc,
_source: {
...sampleDoc._source,
signal: 123,
},
} as unknown) as SignalSourceHit;
const { '@timestamp': timestamp, ...fakeSignalSourceHit } = buildBulkBody({
doc,
ruleParams: sampleParams,
id: sampleRuleGuid,
name: 'rule-name',
actions: [],
createdAt: '2020-01-28T15:58:34.810Z',
updatedAt: '2020-01-28T15:59:14.004Z',
createdBy: 'elastic',
updatedBy: 'elastic',
interval: '5m',
enabled: true,
tags: ['some fake tag 1', 'some fake tag 2'],
throttle: 'no_actions',
});
const expected: Omit<SignalHit, '@timestamp'> & { someKey: string } = {
someKey: 'someValue',
event: {
kind: 'signal',
},
signal: {
original_signal: 123,
parent: {
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
parents: [
{
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
],
ancestors: [
{
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
],
original_time: '2020-04-20T21:27:45+0000',
status: 'open',
rule: {
actions: [],
author: ['Elastic'],
building_block_type: 'default',
id: '04128c15-0d1b-4716-a4c5-46997ac7f3bd',
rule_id: 'rule-1',
false_positives: [],
max_signals: 10000,
risk_score: 50,
risk_score_mapping: [],
output_index: '.siem-signals',
description: 'Detecting root and admin users',
from: 'now-6m',
immutable: false,
index: ['auditbeat-*', 'filebeat-*', 'packetbeat-*', 'winlogbeat-*'],
interval: '5m',
language: 'kuery',
license: 'Elastic License',
name: 'rule-name',
query: 'user.name: root or user.name: admin',
references: ['http://google.com'],
severity: 'high',
severity_mapping: [],
tags: ['some fake tag 1', 'some fake tag 2'],
threat: [],
type: 'query',
to: 'now',
note: '',
enabled: true,
created_by: 'elastic',
updated_by: 'elastic',
version: 1,
updated_at: fakeSignalSourceHit.signal.rule?.updated_at,
created_at: fakeSignalSourceHit.signal.rule?.created_at,
throttle: 'no_actions',
exceptions_list: getListArrayMock(),
},
depth: 1,
},
};
expect(fakeSignalSourceHit).toEqual(expected);
});

test('bulk body builds "original_signal" if it exists already as an object', () => {
const sampleParams = sampleRuleAlertParams();
const sampleDoc = sampleDocNoSortId();
delete sampleDoc._source.source;
const doc = ({
...sampleDoc,
_source: {
...sampleDoc._source,
signal: { child_1: { child_2: 'nested data' } },
},
} as unknown) as SignalSourceHit;
const { '@timestamp': timestamp, ...fakeSignalSourceHit } = buildBulkBody({
doc,
ruleParams: sampleParams,
id: sampleRuleGuid,
name: 'rule-name',
actions: [],
createdAt: '2020-01-28T15:58:34.810Z',
updatedAt: '2020-01-28T15:59:14.004Z',
createdBy: 'elastic',
updatedBy: 'elastic',
interval: '5m',
enabled: true,
tags: ['some fake tag 1', 'some fake tag 2'],
throttle: 'no_actions',
});
const expected: Omit<SignalHit, '@timestamp'> & { someKey: string } = {
someKey: 'someValue',
event: {
kind: 'signal',
},
signal: {
original_signal: { child_1: { child_2: 'nested data' } },
parent: {
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
parents: [
{
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
],
ancestors: [
{
id: sampleIdGuid,
type: 'event',
index: 'myFakeSignalIndex',
depth: 0,
},
],
original_time: '2020-04-20T21:27:45+0000',
status: 'open',
rule: {
actions: [],
author: ['Elastic'],
building_block_type: 'default',
id: '04128c15-0d1b-4716-a4c5-46997ac7f3bd',
rule_id: 'rule-1',
false_positives: [],
max_signals: 10000,
risk_score: 50,
risk_score_mapping: [],
output_index: '.siem-signals',
description: 'Detecting root and admin users',
from: 'now-6m',
immutable: false,
index: ['auditbeat-*', 'filebeat-*', 'packetbeat-*', 'winlogbeat-*'],
interval: '5m',
language: 'kuery',
license: 'Elastic License',
name: 'rule-name',
query: 'user.name: root or user.name: admin',
references: ['http://google.com'],
severity: 'high',
severity_mapping: [],
tags: ['some fake tag 1', 'some fake tag 2'],
threat: [],
type: 'query',
to: 'now',
note: '',
enabled: true,
created_by: 'elastic',
updated_by: 'elastic',
version: 1,
updated_at: fakeSignalSourceHit.signal.rule?.updated_at,
created_at: fakeSignalSourceHit.signal.rule?.created_at,
throttle: 'no_actions',
exceptions_list: getListArrayMock(),
},
depth: 1,
},
};
expect(fakeSignalSourceHit).toEqual(expected);
});
});

describe('buildSignalFromSequence', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export const buildSignalFromEvent = (
const rule = applyOverrides
? buildRuleWithOverrides(ruleSO, event._source)
: buildRuleWithoutOverrides(ruleSO);
const signal = {
const signal: Signal = {
...buildSignal([event], rule),
...additionalSignalFields(event),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/

import { sampleDocNoSortId } from './__mocks__/es_results';
import { buildEventTypeSignal } from './build_event_type_signal';
import { buildEventTypeSignal, isEventTypeSignal } from './build_event_type_signal';
import { BaseSignalHit } from './types';

describe('buildEventTypeSignal', () => {
beforeEach(() => {
Expand Down Expand Up @@ -44,4 +45,57 @@ describe('buildEventTypeSignal', () => {
};
expect(eventType).toEqual(expected);
});

test('It validates a sample doc with no signal type as "false"', () => {
const doc = sampleDocNoSortId();
expect(isEventTypeSignal(doc)).toEqual(false);
});

test('It validates a sample doc with a signal type as "true"', () => {
const doc: BaseSignalHit = ({
...sampleDocNoSortId(),
_source: {
...sampleDocNoSortId()._source,
signal: {
rule: { id: 'id-123' },
},
},
} as unknown) as BaseSignalHit;
expect(isEventTypeSignal(doc)).toEqual(true);
});

test('It validates a numeric signal string as "false"', () => {
const doc: BaseSignalHit = ({
...sampleDocNoSortId(),
_source: {
...sampleDocNoSortId()._source,
signal: 'something',
},
} as unknown) as BaseSignalHit;
expect(isEventTypeSignal(doc)).toEqual(false);
});

test('It validates an empty object as "false"', () => {
const doc: BaseSignalHit = ({
...sampleDocNoSortId(),
_source: {
...sampleDocNoSortId()._source,
signal: {},
},
} as unknown) as BaseSignalHit;
expect(isEventTypeSignal(doc)).toEqual(false);
});

test('It validates an empty rule object as "false"', () => {
const doc: BaseSignalHit = ({
...sampleDocNoSortId(),
_source: {
...sampleDocNoSortId()._source,
signal: {
rule: {},
},
},
} as unknown) as BaseSignalHit;
expect(isEventTypeSignal(doc)).toEqual(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,16 @@ export const buildEventTypeSignal = (doc: BaseSignalHit): object => {
return { kind: 'signal' };
}
};

/**
* Given a document this will return true if that document is a signal
* document. We can't guarantee the code will call this function with a document
* before adding the _source.event.kind = "signal" from "buildEventTypeSignal"
* so we do basic testing to ensure that if the object has the fields of:
* "signal.rule.id" then it will be one of our signals rather than a customer
* overwritten signal.
* @param doc The document which might be a signal or it might be a regular log
*/
export const isEventTypeSignal = (doc: BaseSignalHit): boolean => {
return doc._source.signal?.rule?.id != null && typeof doc._source.signal?.rule?.id === 'string';
};
Loading

0 comments on commit 3e1e65b

Please sign in to comment.