[Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query #93176

marshallmain · 2021-03-02T07:22:13Z

The value list ID was being inserted directly into the KQL filter without being escaped or surrounded by quotes, so special KQL characters in the ID would break the query generation. This PR encloses the IDs in quotes and adds simple escaping logic to the ID (replacing " with \") so IDs with quotes or other special characters won't break the KQL parsing.

We should be on the lookout for other places where we generate KQL queries and insert values that could be user supplied.

Related: when importing value lists, the filename comes in URI encoded and we store it in this URI encoded form. Do we want to decode the URI here so that the filename will be stored in its original form (and also displayed in the original form on the UI)? I found this because quotes in the filename get URI encoded and never decoded if you import using the UI, whereas with the API you can create a list that has quotes in the list_id.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…uotes

elasticmachine · 2021-03-02T07:40:19Z

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

elasticmachine · 2021-03-02T07:40:20Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

rw-access · 2021-03-02T14:47:45Z

We should be on the lookout for other places where we generate KQL queries and insert values that could be user supplied.

I wonder if there's an opportunity here to allow for parameters, like how SQL uses ?. I've seen a few other interpolation issues, and something more resilient could be a better approach in the long run.

marshallmain · 2021-03-02T18:12:16Z

@rw-access yeah I was hoping to find something like that in Kibana already but didn't see it anywhere, doing my own escaping felt dangerous. The KQL parsing in general has a couple different areas where it could be improved, as it also generates sub-optimal queries when the KQL has many terms - each term creates a new level of nesting in the generated DSL, so you can hit nesting limits quickly.

marshallmain · 2021-03-02T18:12:29Z

@elasticmachine merge upstream

kibanamachine · 2021-03-02T20:09:06Z

💚 Build Succeeded

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
`triggersActionsUi`	1.6MB	1.5MB	-23.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	104.0KB	104.1KB	+82.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	41	42	+1

History

💔 Build #110216 failed d926295

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

FrankHassanabad · 2021-03-03T16:59:03Z

x-pack/plugins/lists/server/services/utils/escape_query.ts

+
+export const escapeQuotes = (str: string): string => {
+  return str.replace(/[\\"]/g, '\\$&');
+};


Is this the same as this one?
x-pack/plugins/data_enhanced/public/autocomplete/providers/kql_query_suggestion/lib/escape_kuery.ts

That looks like it only works client side and not server and isn't part of a common section for usage in both areas which is a bummer.

Wish it was in one common spot. I'm fine with this duplicated here though. I would just maybe? copy maybe over their tests and put a note that there is a duplication between the two in case someone sees them deviate later/change.

The other tests above and the e2e is more than good enough though, wouldn't split hairs on that.

Yep it's the same function, definitely would like to have it in a common place along with the other escaping utilities (and maybe a more robust parameterization scheme like Ross suggested above). For now though it seems a simple enough function to just do the ol' copy paste.

FrankHassanabad · 2021-03-03T16:59:33Z

x-pack/test/lists_api_integration/security_and_spaces/tests/delete_lists.ts

+          .expect(200);
+
+        const bodyToCompare = removeListServerGeneratedProperties(body);
+        expect(bodyToCompare).to.eql(getListResponseMockWithoutAutoGeneratedValues());


Nice! Thanks for the e2e test here.

FrankHassanabad

LGTM

… in KQL query (elastic#93176) * Escape quotes in list ids and quote the id in KQL query * Remove decodeURIComponent because too many KQL queries don't handle quotes * Add quotes to user supplied IDs for other KQL queries Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

… in KQL query (#93176) (#93502) * Escape quotes in list ids and quote the id in KQL query * Remove decodeURIComponent because too many KQL queries don't handle quotes * Add quotes to user supplied IDs for other KQL queries Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

… in KQL query (#93176) (#93503) * Escape quotes in list ids and quote the id in KQL query * Remove decodeURIComponent because too many KQL queries don't handle quotes * Add quotes to user supplied IDs for other KQL queries Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (48 commits) Fix wrong import in data plugin causing 100kB bundle increase (elastic#93448) [Fleet] Correctly track install status of an integration (elastic#93464) Reviews data frame analytics UI text (elastic#93033) Display multiple copyable fields for process.args in resolver node detail panel (elastic#93280) [Security Solution][Detections] ML Popover overflow fix (elastic#93525) chore(NA): do not use execa on bazel workspace status update script (elastic#93532) Bump dependencies (elastic#93511) [dev/build_ts_refs] support disabling the ts-refs build completely (elastic#93529) [Security Solution] fix data provider cypress test (elastic#93465) Fix service map for All environment single service (elastic#93517) [Fleet] Fix package version comparaison in the UI (elastic#93498) [alerting] adds doc on JSON-expanded action variables and task manager max_workers (elastic#92720) [dev/build_ts_refs] ignore type checking failures when building ts refs (elastic#93473) [core-new-docs] Adds a dev-doc for core documentation (elastic#92976) remove opacity from maps legacy style (elastic#93456) [Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query (elastic#93176) Revert "Make tests deterministic by providing unique timestamps (elastic#93350)" [Discover] Fix link from dashboard saved search to Discover (elastic#92937) update public api docs App Search - Polishing Analytics Views (elastic#92939) ...

marshallmain added 3 commits March 2, 2021 01:23

Escape quotes in list ids and quote the id in KQL query

51d71ae

Remove decodeURIComponent because too many KQL queries don't handle q…

b064b7a

…uotes

Add quotes to user supplied IDs for other KQL queries

d926295

marshallmain marked this pull request as ready for review March 2, 2021 07:39

marshallmain requested review from a team as code owners March 2, 2021 07:39

marshallmain added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.12.0 v8.0.0 labels Mar 2, 2021

marshallmain added the release_note:skip Skip the PR/issue when compiling release notes label Mar 2, 2021

Merge branch 'master' into escape-list-ids

4621d36

marshallmain mentioned this pull request Mar 2, 2021

[Security Solution] Filename on value list import is stored in URI encoded form #93346

Closed

FrankHassanabad reviewed Mar 3, 2021

View reviewed changes

FrankHassanabad approved these changes Mar 3, 2021

View reviewed changes

marshallmain added the v7.13.0 label Mar 3, 2021

marshallmain merged commit 4c89398 into elastic:master Mar 3, 2021

This was referenced Mar 3, 2021

[7.x] [Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query (#93176) #93502

Merged

[7.12] [Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query (#93176) #93503

Merged

marshallmain deleted the escape-list-ids branch March 4, 2021 01:02

nkhristinin mentioned this pull request Sep 10, 2021

Decode file name on upload value lists and fix bug with removing value list #111838

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query #93176

[Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query #93176

marshallmain commented Mar 2, 2021 •

edited

Loading

elasticmachine commented Mar 2, 2021

elasticmachine commented Mar 2, 2021

rw-access commented Mar 2, 2021

marshallmain commented Mar 2, 2021

marshallmain commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

async chunk count

FrankHassanabad Mar 3, 2021 •

edited

Loading

marshallmain Mar 3, 2021

FrankHassanabad Mar 3, 2021

FrankHassanabad left a comment

[Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query #93176

[Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query #93176

Conversation

marshallmain commented Mar 2, 2021 • edited Loading

Checklist

For maintainers

elasticmachine commented Mar 2, 2021

elasticmachine commented Mar 2, 2021

rw-access commented Mar 2, 2021

marshallmain commented Mar 2, 2021

marshallmain commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

FrankHassanabad Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

marshallmain Mar 3, 2021

Choose a reason for hiding this comment

FrankHassanabad Mar 3, 2021

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

marshallmain commented Mar 2, 2021 •

edited

Loading

FrankHassanabad Mar 3, 2021 •

edited

Loading