From f38b65e82bdc7bdb30c6d6cca190b23ad3ec460d Mon Sep 17 00:00:00 2001 From: Bandini <63824432+bandinib-amzn@users.noreply.github.com> Date: Thu, 28 Jul 2022 14:47:03 -0700 Subject: [PATCH] [CVE] Handle invalid query, index and date in vega charts filter handlers (#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 * new license header for new files Signed-off-by: Bandini Bhopi Co-authored-by: Kawika Avilla --- .../validate_object.test.ts.snap | 13 +++ packages/osd-std/src/index.ts | 1 + packages/osd-std/src/validate_object.test.ts | 65 +++++++++++++ packages/osd-std/src/validate_object.ts | 66 +++++++++++++ .../public/data_model/utils.test.js | 94 +++++++++++++++++++ .../vis_type_vega/public/data_model/utils.ts | 38 ++++++++ .../public/vega_view/vega_base_view.js | 21 +++-- 7 files changed, 290 insertions(+), 8 deletions(-) create mode 100644 packages/osd-std/src/__snapshots__/validate_object.test.ts.snap create mode 100644 packages/osd-std/src/validate_object.test.ts create mode 100644 packages/osd-std/src/validate_object.ts create mode 100644 src/plugins/vis_type_vega/public/data_model/utils.test.js diff --git a/packages/osd-std/src/__snapshots__/validate_object.test.ts.snap b/packages/osd-std/src/__snapshots__/validate_object.test.ts.snap new file mode 100644 index 000000000000..937e040c771e --- /dev/null +++ b/packages/osd-std/src/__snapshots__/validate_object.test.ts.snap @@ -0,0 +1,13 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`can't submit {"__proto__":null} 1`] = `"'__proto__' is an invalid key"`; + +exports[`can't submit {"constructor":{"prototype":null}} 1`] = `"'constructor.prototype' is an invalid key"`; + +exports[`can't submit {"foo":{"__proto__":true}} 1`] = `"'__proto__' is an invalid key"`; + +exports[`can't submit {"foo":{"bar":{"__proto__":{}}}} 1`] = `"'__proto__' is an invalid key"`; + +exports[`can't submit {"foo":{"bar":{"constructor":{"prototype":null}}}} 1`] = `"'constructor.prototype' is an invalid key"`; + +exports[`can't submit {"foo":{"constructor":{"prototype":null}}} 1`] = `"'constructor.prototype' is an invalid key"`; diff --git a/packages/osd-std/src/index.ts b/packages/osd-std/src/index.ts index 3f2db99efd5d..49902a2c2479 100644 --- 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 new file mode 100644 index 000000000000..4bf3d04302e4 --- /dev/null +++ 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 = {}; + 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 new file mode 100644 index 000000000000..96b42050a361 --- /dev/null +++ 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 new file mode 100644 index 000000000000..9fe648c71139 --- /dev/null +++ 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 index f363740f536d..4e7a85062354 100644 --- 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 index 6040c8ffb2d3..824119110463 100644 --- 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: '*',