[TIP] add field existence check to the painless script #144344

maxcold · 2022-11-01T17:35:58Z

Fixes https://github.com/elastic/security-team/issues/5296

Summary

The painless script that generates threat.indicator.name runtime field was missing the check for field existence. Specificially it impacted the cases where there is a file type indicator document without threat.indicator.file.hash.sha256 as it was being checked first for file type IoCs. The app crashes in such cases and/or behaves weirdly depending on the data grid action used

PhilippeOberti · 2022-11-03T20:11:04Z

x-pack/plugins/threat_intelligence/server/utils/indicator_name.ts

@@ -40,6 +40,7 @@ const mappingsArray: Mappings = [
      RawIndicatorFieldId.FileImphash,
      RawIndicatorFieldId.FilePehash,
      RawIndicatorFieldId.FileVhash,
+      RawIndicatorFieldId.FileTelfhash,


this was missing from https://github.com/elastic/kibana/blob/main/x-pack/plugins/threat_intelligence/common/types/indicator.ts#L63

PhilippeOberti · 2022-11-03T20:12:10Z

x-pack/plugins/threat_intelligence/common/types/indicator.ts

@@ -56,12 +56,25 @@ export enum RawIndicatorFieldId {
 * (reverse of https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/cti/constants.ts#L35)
 */
 export const IndicatorFieldEventEnrichmentMap: { [id: string]: string[] } = {
+  [RawIndicatorFieldId.FileSha256]: ['file.hash.sha256'],


this fixes an unrelated bug: happens when the grid is trying to display the investigate in timeline button, the data grid crashes.
This bug was hidden behind the one the PR is trying to fix.

Are we sure that this fixes it for good? What if there is no threat.indicator.file.hash.* field on the document. See for example https://github.com/elastic/seceng/issues/4768#issuecomment-1303165947 , their documents have the prefix threatintel.* instead of threat.* therefore neither of these mappings will work. But I believe we should still handle this gracefully and don't render the Investigate In Timeline action if it is not supported

I also think this is a good candidate for 8.5 backporting

I think it all depends on how we handle the original bug: if we show the value in the grid with a - for example (as you suggested earlier) then this might break. If we don't show the entry in the grid this should be good, as this list matches the list we use in the painless script.

Once we figure out the original bug, I can update this piece of code as well

I added a bit more safety to the use_investigate_in_timeline hook to avoid issues when a new hash is added to one list and not added to another. Ideally, we let the type checker do that and do not rely on syncing two lists manually

x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts

PhilippeOberti · 2022-11-03T20:18:34Z

@maxcold I pushed a commit with the code I have implemented. These are the steps I'm using to debug things:

run cypress using yarn cypress:open-as-ci
remove/add values here and see the change reflect in the indicators table in the UI

Notes:
You might want to run ES (yarn es snapshot --license trial) and kibana (yarn start --no-base-path) as you run Cypress, to help on reload. I found that without running kibana the UI isn't refreshing properly after making code changes in the public folder. Also when you change the code in the server folder, you have to restart Cypress completely (yarn cypress:open-as-ci).

maxcold · 2022-11-04T12:06:54Z

x-pack/plugins/threat_intelligence/public/modules/timeline/hooks/use_investigate_in_timeline.ts

    return {} as unknown as UseInvestigateInTimelineValue;
  }

-  const dataProviders: DataProvider[] = [...IndicatorFieldEventEnrichmentMap[key], key].map(


I believe this bug shows that we should be more strict with the types as we rely on a very specific set of fields for this feature

PhilippeOberti · 2022-11-04T21:30:20Z

x-pack/plugins/threat_intelligence/common/types/indicator.ts

+  FileSSDeep = 'threat.indicator.file.hash.ssdeep',
+  FileTlsh = 'threat.indicator.file.hash.tlsh',
+  FileImpfuzzy = 'threat.indicator.file.hash.impfuzzy',
+  FileImphash = 'threat.indicator.file.hash.imphash',
+  FilePehash = 'threat.indicator.file.hash.pehash',
+  FileVhash = 'threat.indicator.file.hash.vhash',


modified these using this source of truth

PhilippeOberti · 2022-11-04T21:30:33Z

x-pack/plugins/threat_intelligence/common/types/indicator.ts

-  [RawIndicatorFieldId.FilePehash]: ['file.pehash'],
-  [RawIndicatorFieldId.FileVhash]: ['file.vhash'],
-  [RawIndicatorFieldId.FileTelfhash]: ['file.elf.telfhash'],
+  [RawIndicatorFieldId.FileSSDeep]: ['file.hash.ssdeep'],


same here, modifying following this

PhilippeOberti · 2022-11-04T21:32:43Z

x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts

@@ -45,29 +48,68 @@ const THREAT_INTELLIGENCE = '/app/security/threat_intelligence/indicators';
 const URL_WITH_CONTRADICTORY_FILTERS =
  '/app/security/threat_intelligence/indicators?indicators=(filterQuery:(language:kuery,query:%27%27),filters:!((%27$state%27:(store:appState),meta:(alias:!n,disabled:!f,index:%27%27,key:threat.indicator.type,negate:!f,params:(query:file),type:phrase),query:(match_phrase:(threat.indicator.type:file))),(%27$state%27:(store:appState),meta:(alias:!n,disabled:!f,index:%27%27,key:threat.indicator.type,negate:!f,params:(query:url),type:phrase),query:(match_phrase:(threat.indicator.type:url)))),timeRange:(from:now/d,to:now/d))';

-Cypress.Cy.prototype.onUncaughtException = function () {};
+describe('Invalid Indicators', () => {


this section tests that the table shows up even with missing fields. The following logic is applied:

test with all fields present

test with missing sha256

test with missing sha256 and md5

continue removing fields until there are none left

The UI Indicator name column should display the proper value for the present fields.
If no fields are present, the Indicator name column shows -. and the investigate in timeline button is not rendered

added more tests for the following:

missing threat field entirely

missing type field

missing indicator field

I think this is overkill but we should be safe

PhilippeOberti · 2022-11-04T21:33:16Z

x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts

  });

-  describe('verify the grid loads even with missing fields', () => {
+  describe('verify the grid loads even with missing mappings and missing fields', () => {


same tests as above but this time with a missing mappings.json file. Not sure these tests are really useful though...

PhilippeOberti · 2022-11-04T21:33:47Z

x-pack/plugins/threat_intelligence/server/utils/indicator_name.ts

@@ -63,7 +62,7 @@ const fieldTypeCheck = (type: string) =>
 * Generates Painless condition checking if given `field` has value
 */
 const fieldValueCheck = (field: string) =>
-  `if (doc['${field}'].size()!=0 && doc['${field}'].value!=null)`;
+  `if (doc.containsKey('${field}') && !doc['${field}'].empty && doc['${field}'].size()!=0 && doc['${field}'].value!=null)`;


following this documentation for missing keys and missing values

- fix fields in RawIndicatorFieldId (for investigate in timeline hook crash) - fix logic to check if a field exist in the painless script - add more e2e to test with missing fields and also missing mappings file

PhilippeOberti · 2022-11-08T15:46:40Z

x-pack/plugins/threat_intelligence/server/utils/indicator_name.ts

@@ -56,12 +56,13 @@ const mappingsArray: Mappings = [
 * Generates Painless condition checking if given `type` is matched
 */
 const fieldTypeCheck = (type: string) =>
-  `if (doc['threat.indicator.type'].value != null && doc['threat.indicator.type'].value.toLowerCase()=='${type.toLowerCase()}')`;
+  `if (doc.containsKey('threat.indicator.type') && !doc['threat.indicator.type'].empty && doc['threat.indicator.type'].size()!=0 && doc['threat.indicator.type'].value!=null && doc['threat.indicator.type'].value.toLowerCase()=='${type.toLowerCase()}')`;


got this change from @maxcold comment and work here

maxcold · 2022-11-08T15:58:02Z

The changes looks good to me!

kibana-ci · 2022-11-08T16:49:32Z

💚 Build Succeeded

Buildkite Build
Commit: dc75859

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`threatIntelligence`	43.1KB	43.6KB	+450.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	108	113	+5
`securitySolution`	440	446	+6
total			+19

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	67	73	+6
`osquery`	109	115	+6
`securitySolution`	517	523	+6
total			+20

History

💚 Build #85610 succeeded 7c87fe0
💔 Build #85425 failed 23b4da9
💔 Build #85289 failed 5d319e2
💚 Build #84879 succeeded cff312b
💚 Build #84425 succeeded aa80a04

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elasticmachine · 2022-11-14T17:22:56Z

Pinging @elastic/security-solution (Team: SecuritySolution)

maxcold added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team: Protections Experience v8.6.0 labels Nov 1, 2022

maxcold requested a review from a team as a code owner November 1, 2022 17:35

PhilippeOberti reviewed Nov 3, 2022

View reviewed changes

x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts Outdated Show resolved Hide resolved

PhilippeOberti reviewed Nov 3, 2022

View reviewed changes

x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts Outdated Show resolved Hide resolved

maxcold commented Nov 4, 2022

View reviewed changes

PhilippeOberti reviewed Nov 4, 2022

View reviewed changes

PhilippeOberti mentioned this pull request Nov 4, 2022

[TIP] Improve safety check on painless script and fix wrong indicator fields #144651

Merged

[TIP] add field existence check to the painless script

dc75859

- fix fields in RawIndicatorFieldId (for investigate in timeline hook crash) - fix logic to check if a field exist in the painless script - add more e2e to test with missing fields and also missing mappings file

PhilippeOberti force-pushed the tip-add-safety-check-to-painless branch from 7c87fe0 to dc75859 Compare November 8, 2022 15:43

PhilippeOberti reviewed Nov 8, 2022

View reviewed changes

PhilippeOberti approved these changes Nov 8, 2022

View reviewed changes

PhilippeOberti merged commit 0082424 into main Nov 8, 2022

PhilippeOberti deleted the tip-add-safety-check-to-painless branch November 8, 2022 16:51

maxcold added release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. and removed release_note:skip Skip the PR/issue when compiling release notes labels Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIP] add field existence check to the painless script #144344

[TIP] add field existence check to the painless script #144344

maxcold commented Nov 1, 2022

PhilippeOberti Nov 3, 2022

PhilippeOberti Nov 3, 2022 •

edited

maxcold Nov 4, 2022

maxcold Nov 4, 2022

PhilippeOberti Nov 4, 2022

maxcold Nov 8, 2022

PhilippeOberti commented Nov 3, 2022

maxcold Nov 4, 2022

PhilippeOberti Nov 4, 2022

PhilippeOberti Nov 4, 2022

PhilippeOberti Nov 4, 2022

PhilippeOberti Nov 8, 2022

PhilippeOberti Nov 4, 2022

PhilippeOberti Nov 4, 2022

PhilippeOberti Nov 8, 2022

maxcold commented Nov 8, 2022

kibana-ci commented Nov 8, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

elasticmachine commented Nov 14, 2022

[TIP] add field existence check to the painless script #144344

[TIP] add field existence check to the painless script #144344

Conversation

maxcold commented Nov 1, 2022

Summary

Choose a reason for hiding this comment

PhilippeOberti Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeOberti commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcold commented Nov 8, 2022

kibana-ci commented Nov 8, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

elasticmachine commented Nov 14, 2022

PhilippeOberti Nov 3, 2022 •

edited