Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CVE] Handle invalid query, index and date in vega charts filter handlers #1932

Merged
merged 3 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/osd-std/src/__snapshots__/validate_object.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/osd-std/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
65 changes: 65 additions & 0 deletions packages/osd-std/src/validate_object.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
bandinib-amzn marked this conversation as resolved.
Show resolved Hide resolved
* 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();
});
});
66 changes: 66 additions & 0 deletions packages/osd-std/src/validate_object.ts
Original file line number Diff line number Diff line change
@@ -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,
});
}
}
}
94 changes: 94 additions & 0 deletions src/plugins/vis_type_vega/public/data_model/utils.test.js
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
38 changes: 38 additions & 0 deletions src/plugins/vis_type_vega/public/data_model/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
*/

import compactStringify from 'json-stringify-pretty-compact';
import { validateObject } from '@osd/std';

export class Utils {
/**
Expand Down Expand Up @@ -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;
}
}
21 changes: 13 additions & 8 deletions src/plugins/vis_type_vega/public/vega_view/vega_base_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand All @@ -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: '*',
Expand Down