[CVE] Handle invalid query, index and date in vega charts filter hand…

…lers (#1932) * [CVE] Handle invalid query, index and date in vega charts filter handlers Potential way to prevent XSS vulnerability discovered in the Vega charts OSD integration. CVE link: https://nvd.nist.gov/vuln/detail/CVE-2022-23713 Signed-off-by: Bandini Bhopi <[email protected]> * new license header for new files Signed-off-by: Bandini Bhopi <[email protected]> Co-authored-by: Kawika Avilla <[email protected]>
opensearch-project · Aug 3, 2022 · f38b65e · f38b65e
1 parent 52ce386
commit f38b65e
Show file tree

Hide file tree

Showing 7 changed files with 290 additions and 8 deletions.
diff --git a/packages/osd-std/src/__snapshots__/validate_object.test.ts.snap b/packages/osd-std/src/__snapshots__/validate_object.test.ts.snap
diff --git a/packages/osd-std/src/index.ts b/packages/osd-std/src/index.ts
@@ -38,4 +38,5 @@ export { withTimeout } from './promise';
 export { isRelativeUrl, modifyUrl, getUrlOrigin, URLMeaningfulParts } from './url';
 export { unset } from './unset';
 export { getFlattenedObject } from './get_flattened_object';
+export { validateObject } from './validate_object';
 export * from './rxjs_7';
diff --git a/packages/osd-std/src/validate_object.test.ts b/packages/osd-std/src/validate_object.test.ts
@@ -0,0 +1,65 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { validateObject } from './validate_object';
+
+test(`fails on circular references`, () => {
+  const foo: Record<string, any> = {};
+  foo.myself = foo;
+
+  expect(() =>
+    validateObject({
+      payload: foo,
+    })
+  ).toThrowErrorMatchingInlineSnapshot(`"circular reference detected"`);
+});
+
+[
+  {
+    foo: true,
+    bar: '__proto__',
+    baz: 1.1,
+    qux: undefined,
+    quux: () => null,
+    quuz: Object.create(null),
+  },
+  {
+    foo: {
+      foo: true,
+      bar: '__proto__',
+      baz: 1.1,
+      qux: undefined,
+      quux: () => null,
+      quuz: Object.create(null),
+    },
+  },
+  { constructor: { foo: { prototype: null } } },
+  { prototype: { foo: { constructor: null } } },
+].forEach((value) => {
+  ['headers', 'payload', 'query', 'params'].forEach((property) => {
+    const obj = {
+      [property]: value,
+    };
+    test(`can submit ${JSON.stringify(obj)}`, () => {
+      expect(() => validateObject(obj)).not.toThrowError();
+    });
+  });
+});
+
+// if we use the object literal syntax to create the following values, we end up
+// actually reassigning the __proto__ which makes it be a non-enumerable not-own property
+// which isn't what we want to test here
+[
+  JSON.parse(`{ "__proto__": null }`),
+  JSON.parse(`{ "foo": { "__proto__": true } }`),
+  JSON.parse(`{ "foo": { "bar": { "__proto__": {} } } }`),
+  JSON.parse(`{ "constructor": { "prototype" : null } }`),
+  JSON.parse(`{ "foo": { "constructor": { "prototype" : null } } }`),
+  JSON.parse(`{ "foo": { "bar": { "constructor": { "prototype" : null } } } }`),
+].forEach((value) => {
+  test(`can't submit ${JSON.stringify(value)}`, () => {
+    expect(() => validateObject(value)).toThrowErrorMatchingSnapshot();
+  });
+});
diff --git a/packages/osd-std/src/validate_object.ts b/packages/osd-std/src/validate_object.ts
@@ -0,0 +1,66 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+interface StackItem {
+  value: any;
+  previousKey: string | null;
+}
+
+// we have to do Object.prototype.hasOwnProperty because when you create an object using
+// Object.create(null), and I assume other methods, you get an object without a prototype,
+// so you can't use current.hasOwnProperty
+const hasOwnProperty = (obj: any, property: string) =>
+  Object.prototype.hasOwnProperty.call(obj, property);
+
+const isObject = (obj: any) => typeof obj === 'object' && obj !== null;
+
+// we're using a stack instead of recursion so we aren't limited by the call stack
+export function validateObject(obj: any) {
+  if (!isObject(obj)) {
+    return;
+  }
+
+  const stack: StackItem[] = [
+    {
+      value: obj,
+      previousKey: null,
+    },
+  ];
+  const seen = new WeakSet([obj]);
+
+  while (stack.length > 0) {
+    const { value, previousKey } = stack.pop()!;
+
+    if (!isObject(value)) {
+      continue;
+    }
+
+    if (hasOwnProperty(value, '__proto__')) {
+      throw new Error(`'__proto__' is an invalid key`);
+    }
+
+    if (hasOwnProperty(value, 'prototype') && previousKey === 'constructor') {
+      throw new Error(`'constructor.prototype' is an invalid key`);
+    }
+
+    // iterating backwards through an array is reportedly more performant
+    const entries = Object.entries(value);
+    for (let i = entries.length - 1; i >= 0; --i) {
+      const [key, childValue] = entries[i];
+      if (isObject(childValue)) {
+        if (seen.has(childValue)) {
+          throw new Error('circular reference detected');
+        }
+
+        seen.add(childValue);
+      }
+
+      stack.push({
+        value: childValue,
+        previousKey: key,
+      });
+    }
+  }
+}
diff --git a/src/plugins/vis_type_vega/public/data_model/utils.test.js b/src/plugins/vis_type_vega/public/data_model/utils.test.js
@@ -0,0 +1,94 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { Utils } from './utils';
+
+describe('Utils.handleNonStringIndex', () => {
+  test('should return same string on string input', async () => {
+    expect(Utils.handleNonStringIndex('*')).toBe('*');
+  });
+
+  test('should return empty string on empty string input', async () => {
+    expect(Utils.handleNonStringIndex('')).toBe('');
+  });
+
+  test('should return undefined on non-string input', async () => {
+    expect(Utils.handleNonStringIndex(123)).toBe(undefined);
+    expect(Utils.handleNonStringIndex(null)).toBe(undefined);
+    expect(Utils.handleNonStringIndex(undefined)).toBe(undefined);
+  });
+});
+
+describe('Utils.handleInvalidDate', () => {
+  test('should return null if passed timestamp is Not A Number', async () => {
+    expect(Utils.handleInvalidDate(Number.NaN)).toBe(null);
+  });
+
+  test('should return timestamp if passed timestamp is valid Number', async () => {
+    expect(Utils.handleInvalidDate(1658189958487)).toBe(1658189958487);
+  });
+
+  test('should return string on string input', async () => {
+    expect(Utils.handleInvalidDate('Sat Jul 16 2022 22:59:42')).toBe('Sat Jul 16 2022 22:59:42');
+  });
+
+  test('should return date on date input', async () => {
+    const date = Date.now();
+    expect(Utils.handleInvalidDate(date)).toBe(date);
+  });
+
+  test('should return null if input neigther timestamp, nor date, nor string', async () => {
+    expect(Utils.handleInvalidDate(undefined)).toBe(null);
+    expect(Utils.handleInvalidDate({ key: 'value' })).toBe(null);
+  });
+});
+
+describe('Utils.handleInvalidQuery', () => {
+  test('should return valid object on valid DSL query', async () => {
+    const testQuery = { match_phrase: { customer_gender: 'MALE' } };
+    expect(Utils.handleInvalidQuery(testQuery)).toStrictEqual(testQuery);
+  });
+
+  test('should return null on null or undefined input', async () => {
+    expect(Utils.handleInvalidQuery(null)).toBe(null);
+    expect(Utils.handleInvalidQuery(undefined)).toBe(null);
+  });
+
+  test('should return null if input object has function as property', async () => {
+    const input = {
+      key1: 'value1',
+      key2: () => {
+        alert('Hello!');
+      },
+    };
+
+    expect(Utils.handleInvalidQuery(input)).toBe(null);
+  });
+
+  test('should return null if nested object has function as property', async () => {
+    const input = {
+      key1: 'value1',
+      key2: {
+        func: () => {
+          alert('Hello!');
+        },
+      },
+    };
+    expect(Utils.handleInvalidQuery(input)).toBe(null);
+  });
+
+  test('should throw error on polluted query', async () => {
+    const maliciousQueries = [
+      JSON.parse(`{ "__proto__": null }`),
+      JSON.parse(`{ "constructor": { "prototype" : null } }`),
+    ];
+
+    maliciousQueries.forEach((value) => {
+      expect(() => {
+        Utils.handleInvalidQuery(value);
+      }).toThrowError();
+    });
+  });
+});
diff --git a/src/plugins/vis_type_vega/public/data_model/utils.ts b/src/plugins/vis_type_vega/public/data_model/utils.ts
@@ -29,6 +29,7 @@
  */
 
 import compactStringify from 'json-stringify-pretty-compact';
+import { validateObject } from '@osd/std';
 
 export class Utils {
   /**
@@ -59,4 +60,41 @@ export class Utils {
     }
     return Utils.formatWarningToStr(error, ...Array.from(args).slice(1));
   }
+
+  static handleNonStringIndex(index: unknown): string | undefined {
+    return typeof index === 'string' ? index : undefined;
+  }
+
+  static handleInvalidQuery(query: unknown): object | null {
+    if (Utils.isObject(query) && !Utils.checkForFunctionProperty(query as object)) {
+      // Validating object against prototype pollution
+      validateObject(query);
+      return JSON.parse(JSON.stringify(query));
+    }
+    return null;
+  }
+
+  static isObject(object: unknown): boolean {
+    return !!(object && typeof object === 'object' && !Array.isArray(object));
+  }
+
+  static checkForFunctionProperty(object: object): boolean {
+    let result = false;
+    Object.values(object).forEach((value) => {
+      result =
+        typeof value === 'function'
+          ? true
+          : Utils.isObject(value) && Utils.checkForFunctionProperty(value);
+    });
+    return result;
+  }
+
+  static handleInvalidDate(date: unknown): number | string | Date | null {
+    if (typeof date === 'number') {
+      return !isNaN(date) ? date : null;
+    } else if (date instanceof Date || typeof date === 'string') {
+      return date;
+    }
+    return null;
+  }
 }
diff --git a/src/plugins/vis_type_vega/public/vega_view/vega_base_view.js b/src/plugins/vis_type_vega/public/vega_view/vega_base_view.js
@@ -310,23 +310,25 @@ export class VegaBaseView {
   }
 
   /**
-   * @param {object} query Elastic Query DSL snippet, as used in the query DSL editor
+   * @param {object} query Query DSL snippet, as used in the query DSL editor
    * @param {string} [index] as defined in OpenSearch Dashboards, or default if missing
    */
   async addFilterHandler(query, index) {
-    const indexId = await this.findIndex(index);
-    const filter = opensearchFilters.buildQueryFilter(query, indexId);
-
+    const indexId = await this.findIndex(Utils.handleNonStringIndex(index));
+    const filter = opensearchFilters.buildQueryFilter(Utils.handleInvalidQuery(query), indexId);
     this._applyFilter({ filters: [filter] });
   }
 
   /**
-   * @param {object} query Elastic Query DSL snippet, as used in the query DSL editor
+   * @param {object} query Query DSL snippet, as used in the query DSL editor
    * @param {string} [index] as defined in OpenSearch Dashboards, or default if missing
    */
   async removeFilterHandler(query, index) {
-    const indexId = await this.findIndex(index);
-    const filterToRemove = opensearchFilters.buildQueryFilter(query, indexId);
+    const indexId = await this.findIndex(Utils.handleNonStringIndex(index));
+    const filterToRemove = opensearchFilters.buildQueryFilter(
+      Utils.handleInvalidQuery(query),
+      indexId
+    );
 
     const currentFilters = this._filterManager.getFilters();
     const existingFilter = currentFilters.find((filter) =>
@@ -352,7 +354,10 @@ export class VegaBaseView {
    * @param {number|string|Date} end
    */
   setTimeFilterHandler(start, end) {
-    const { from, to, mode } = VegaBaseView._parseTimeRange(start, end);
+    const { from, to, mode } = VegaBaseView._parseTimeRange(
+      Utils.handleInvalidDate(start),
+      Utils.handleInvalidDate(end)
+    );
 
     this._applyFilter({
       timeFieldName: '*',