[Security Solution][Lists] Escape quotes in list ids and quote the id…

… 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>
marshallmain · Mar 3, 2021 · 477a30c · 477a30c
1 parent ad75625
commit 477a30c
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 13 deletions.
diff --git a/x-pack/plugins/lists/server/routes/delete_list_route.ts b/x-pack/plugins/lists/server/routes/delete_list_route.ts
@@ -19,6 +19,7 @@ import {
 } from '../../common/schemas';
 import { getSavedObjectType } from '../services/exception_lists/utils';
 import { ExceptionListClient } from '../services/exception_lists/exception_list_client';
+import { escapeQuotes } from '../services/utils/escape_query';
 
 import { getExceptionListClient, getListClient } from '.';
 
@@ -142,7 +143,7 @@ const getReferencedExceptionLists = async (
       (item) =>
         `${getSavedObjectType({
           namespaceType: item.namespace_type,
-        })}.attributes.list_id: ${item.list_id}`
+        })}.attributes.list_id: "${escapeQuotes(item.list_id)}"`
     )
     .join(' OR ');
   return exceptionLists.findExceptionList({

diff --git a/x-pack/plugins/lists/server/services/exception_lists/find_exception_list_items.test.ts b/x-pack/plugins/lists/server/services/exception_lists/find_exception_list_items.test.ts
@@ -18,7 +18,18 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list'],
       });
       expect(filter).toEqual(
-        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: some-list-id)'
+        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "some-list-id")'
+      );
+    });
+
+    test('It should create a filter escaping quotes in list ids', () => {
+      const filter = getExceptionListsItemFilter({
+        filter: [],
+        listId: ['list-id-"-with-quote'],
+        savedObjectType: ['exception-list'],
+      });
+      expect(filter).toEqual(
+        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-id-\\"-with-quote")'
       );
     });
 
@@ -29,7 +40,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list'],
       });
       expect(filter).toEqual(
-        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: some-list-id) AND exception-list.attributes.name: "Sample Endpoint Exception List")'
+        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "some-list-id") AND exception-list.attributes.name: "Sample Endpoint Exception List")'
       );
     });
 
@@ -40,7 +51,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list', 'exception-list-agnostic'],
       });
       expect(filter).toEqual(
-        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: list-1) OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-2)'
+        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-1") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-2")'
       );
     });
 
@@ -51,7 +62,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list', 'exception-list-agnostic'],
       });
       expect(filter).toEqual(
-        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: list-1) AND exception-list.attributes.name: "Sample Endpoint Exception List") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-2)'
+        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-1") AND exception-list.attributes.name: "Sample Endpoint Exception List") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-2")'
       );
     });
 
@@ -62,7 +73,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list', 'exception-list-agnostic', 'exception-list-agnostic'],
       });
       expect(filter).toEqual(
-        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: list-1) OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-2) OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-3)'
+        '(exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-1") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-2") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-3")'
       );
     });
 
@@ -73,7 +84,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list', 'exception-list-agnostic', 'exception-list-agnostic'],
       });
       expect(filter).toEqual(
-        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: list-1) AND exception-list.attributes.name: "Sample Endpoint Exception List") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-2) OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-3)'
+        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-1") AND exception-list.attributes.name: "Sample Endpoint Exception List") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-2") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-3")'
       );
     });
 
@@ -88,7 +99,7 @@ describe('find_exception_list_items', () => {
         savedObjectType: ['exception-list', 'exception-list-agnostic', 'exception-list-agnostic'],
       });
       expect(filter).toEqual(
-        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: list-1) AND exception-list.attributes.name: "Sample Endpoint Exception List 1") OR ((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-2) AND exception-list.attributes.name: "Sample Endpoint Exception List 2") OR ((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: list-3) AND exception-list.attributes.name: "Sample Endpoint Exception List 3")'
+        '((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "list-1") AND exception-list.attributes.name: "Sample Endpoint Exception List 1") OR ((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-2") AND exception-list.attributes.name: "Sample Endpoint Exception List 2") OR ((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "list-3") AND exception-list.attributes.name: "Sample Endpoint Exception List 3")'
       );
     });
   });

diff --git a/x-pack/plugins/lists/server/services/exception_lists/find_exception_list_items.ts b/x-pack/plugins/lists/server/services/exception_lists/find_exception_list_items.ts
@@ -24,6 +24,7 @@ import {
   SortFieldOrUndefined,
   SortOrderOrUndefined,
 } from '../../../common/schemas';
+import { escapeQuotes } from '../utils/escape_query';
 
 import { getSavedObjectTypes, transformSavedObjectsToFoundExceptionListItem } from './utils';
 import { getExceptionList } from './get_exception_list';
@@ -89,7 +90,8 @@ export const getExceptionListsItemFilter = ({
   savedObjectType: SavedObjectType[];
 }): string => {
   return listId.reduce((accum, singleListId, index) => {
-    const listItemAppend = `(${savedObjectType[index]}.attributes.list_type: item AND ${savedObjectType[index]}.attributes.list_id: ${singleListId})`;
+    const escapedListId = escapeQuotes(singleListId);
+    const listItemAppend = `(${savedObjectType[index]}.attributes.list_type: item AND ${savedObjectType[index]}.attributes.list_id: "${escapedListId}")`;
     const listItemAppendWithFilter =
       filter[index] != null ? `(${listItemAppend} AND ${filter[index]})` : listItemAppend;
     if (accum === '') {
@@ -117,8 +119,9 @@ export const findValueListExceptionListItems = async ({
   sortField,
   sortOrder,
 }: FindValueListExceptionListsItems): Promise<FoundExceptionListItemSchema | null> => {
+  const escapedValueListId = escapeQuotes(valueListId);
   const savedObjectsFindResponse = await savedObjectsClient.find<ExceptionListSoSchema>({
-    filter: `(exception-list.attributes.list_type: item AND exception-list.attributes.entries.list.id:${valueListId}) OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.entries.list.id:${valueListId}) `,
+    filter: `(exception-list.attributes.list_type: item AND exception-list.attributes.entries.list.id:"${escapedValueListId}") OR (exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.entries.list.id:"${escapedValueListId}") `,
     page,
     perPage,
     sortField,

diff --git a/x-pack/plugins/lists/server/services/utils/escape_query.ts b/x-pack/plugins/lists/server/services/utils/escape_query.ts
@@ -0,0 +1,10 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+export const escapeQuotes = (str: string): string => {
+  return str.replace(/[\\"]/g, '\\$&');
+};
diff --git a/x-pack/plugins/lists/server/services/utils/get_query_filter.test.ts b/x-pack/plugins/lists/server/services/utils/get_query_filter.test.ts
@@ -46,7 +46,7 @@ describe('get_query_filter', () => {
                 minimum_should_match: 1,
                 should: [
                   {
-                    match: {
+                    match_phrase: {
                       list_id: 'list-123',
                     },
                   },
@@ -74,7 +74,7 @@ describe('get_query_filter', () => {
                       minimum_should_match: 1,
                       should: [
                         {
-                          match: {
+                          match_phrase: {
                             list_id: 'list-123',
                           },
                         },

diff --git a/x-pack/plugins/lists/server/services/utils/get_query_filter.ts b/x-pack/plugins/lists/server/services/utils/get_query_filter.ts
@@ -9,6 +9,8 @@ import { DslQuery, EsQueryConfig } from 'src/plugins/data/common';
 
 import { Filter, Query, esQuery } from '../../../../../../src/plugins/data/server';
 
+import { escapeQuotes } from './escape_query';
+
 export interface GetQueryFilterOptions {
   filter: string;
 }
@@ -41,7 +43,10 @@ export const getQueryFilterWithListId = ({
   filter,
   listId,
 }: GetQueryFilterWithListIdOptions): GetQueryFilterReturn => {
+  const escapedListId = escapeQuotes(listId);
   const filterWithListId =
-    filter.trim() !== '' ? `list_id: ${listId} AND (${filter})` : `list_id: ${listId}`;
+    filter.trim() !== ''
+      ? `list_id: "${escapedListId}" AND (${filter})`
+      : `list_id: "${escapedListId}"`;
   return getQueryFilter({ filter: filterWithListId });
 };
diff --git a/x-pack/test/lists_api_integration/security_and_spaces/tests/delete_lists.ts b/x-pack/test/lists_api_integration/security_and_spaces/tests/delete_lists.ts
@@ -63,6 +63,28 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare).to.eql(getListResponseMockWithoutAutoGeneratedValues());
       });
 
+      it('should delete a single list with a list id containing non-alphanumeric characters', async () => {
+        // create a list
+        const id = `some""-list-id"(1)`;
+        await supertest
+          .post(LIST_URL)
+          .set('kbn-xsrf', 'true')
+          .send({
+            ...getCreateMinimalListSchemaMock(),
+            id,
+          })
+          .expect(200);
+
+        // delete the list by its list id
+        const { body } = await supertest
+          .delete(`${LIST_URL}?id=${id}`)
+          .set('kbn-xsrf', 'true')
+          .expect(200);
+
+        const bodyToCompare = removeListServerGeneratedProperties(body);
+        expect(bodyToCompare).to.eql(getListResponseMockWithoutAutoGeneratedValues());
+      });
+
       it('should delete a single list using an auto generated id', async () => {
         // add a list
         const { body: bodyWithCreatedList } = await supertest