From 272410539bd3bbceed6e1f4d9a27485e1d3cfd36 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Tue, 15 Dec 2020 15:59:47 +0300 Subject: [PATCH 1/7] Enable prototype pollution protection in TSVB Closes #78908 --- src/core/server/http/index.ts | 1 + src/core/server/index.ts | 1 + src/plugins/vis_type_timeseries/server/routes/vis.ts | 2 ++ 3 files changed, 4 insertions(+) diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index cb842b2f602685..06ea15e89ce0b3 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -94,3 +94,4 @@ export { } from './cookie_session_storage'; export * from './types'; export { BasePath, IBasePath } from './base_path_service'; +export { validateObject } from './prototype_pollution'; diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 0f2761b67437db..b007def4e81363 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -201,6 +201,7 @@ export { RouteConfigOptionsBody, RouteContentType, validBodyOutput, + validateObject, RouteValidatorConfig, RouteValidationSpec, RouteValidationFunction, diff --git a/src/plugins/vis_type_timeseries/server/routes/vis.ts b/src/plugins/vis_type_timeseries/server/routes/vis.ts index bba086720da0a1..8eede308d2cd7e 100644 --- a/src/plugins/vis_type_timeseries/server/routes/vis.ts +++ b/src/plugins/vis_type_timeseries/server/routes/vis.ts @@ -24,6 +24,7 @@ import { visPayloadSchema } from '../../common/vis_schema'; import { ROUTES } from '../../common/constants'; import { ValidationTelemetryServiceSetup } from '../index'; import { Framework } from '../plugin'; +import { validateObject } from '../../../../core/server'; const escapeHatch = schema.object({}, { unknowns: 'allow' }); @@ -41,6 +42,7 @@ export const visDataRoutes = ( }, async (requestContext, request, response) => { try { + validateObject(request.body); visPayloadSchema.validate(request.body); } catch (error) { logFailedValidation(); From 631bd5726dea0da4ebf4ac214c6369ca99291354 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Wed, 16 Dec 2020 10:12:17 +0300 Subject: [PATCH 2/7] Update Dock API Changes --- ...ibana-plugin-core-server.validateobject.md | 22 +++++++++++++++++++ src/core/server/server.api.md | 5 +++++ 2 files changed, 27 insertions(+) create mode 100644 docs/development/core/server/kibana-plugin-core-server.validateobject.md diff --git a/docs/development/core/server/kibana-plugin-core-server.validateobject.md b/docs/development/core/server/kibana-plugin-core-server.validateobject.md new file mode 100644 index 00000000000000..1772c21c873252 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.validateobject.md @@ -0,0 +1,22 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [validateObject](./kibana-plugin-core-server.validateobject.md) + +## validateObject() function + +Signature: + +```typescript +export declare function validateObject(obj: any): void; +``` + +## Parameters + +| Parameter | Type | Description | +| --- | --- | --- | +| obj | any | | + +Returns: + +`void` + diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 5f07a4b5230560..eed702158e9bdd 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2985,6 +2985,11 @@ export interface UserProvidedValues { userValue?: T; } +// Warning: (ae-missing-release-tag) "validateObject" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export function validateObject(obj: any): void; + // @public export const validBodyOutput: readonly ["data", "stream"]; From 679417c688626f5e0f6a81850d74b6a00e0c4849 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Thu, 17 Dec 2020 16:11:04 +0300 Subject: [PATCH 3/7] Replace logging failed in validateObject validation with 400 error --- src/plugins/vis_type_timeseries/server/routes/vis.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/plugins/vis_type_timeseries/server/routes/vis.ts b/src/plugins/vis_type_timeseries/server/routes/vis.ts index 8eede308d2cd7e..1ae6aa025eb6a1 100644 --- a/src/plugins/vis_type_timeseries/server/routes/vis.ts +++ b/src/plugins/vis_type_timeseries/server/routes/vis.ts @@ -43,6 +43,13 @@ export const visDataRoutes = ( async (requestContext, request, response) => { try { validateObject(request.body); + } catch (error) { + return response.badRequest({ + body: error.message, + }); + } + + try { visPayloadSchema.validate(request.body); } catch (error) { logFailedValidation(); From 0555f023c1e1b2757d555a7a1678f48399017ee3 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Fri, 18 Dec 2020 17:52:04 +0300 Subject: [PATCH 4/7] Move validateObject to kbn-std package and add a description --- ...ibana-plugin-core-server.validateobject.md | 2 +- .../validate_object.test.ts.snap | 0 packages/kbn-std/src/index.ts | 1 + .../kbn-std/src}/validate_object.test.ts | 0 .../kbn-std/src}/validate_object.ts | 0 src/core/server/http/http_tools.ts | 2 +- src/core/server/http/index.ts | 1 - .../server/http/prototype_pollution/index.ts | 20 ------------------- src/core/server/index.ts | 1 - .../vis_type_timeseries/server/routes/vis.ts | 2 +- 10 files changed, 4 insertions(+), 25 deletions(-) rename {src/core/server/http/prototype_pollution => packages/kbn-std/src}/__snapshots__/validate_object.test.ts.snap (100%) rename {src/core/server/http/prototype_pollution => packages/kbn-std/src}/validate_object.test.ts (100%) rename {src/core/server/http/prototype_pollution => packages/kbn-std/src}/validate_object.ts (100%) delete mode 100644 src/core/server/http/prototype_pollution/index.ts diff --git a/docs/development/core/server/kibana-plugin-core-server.validateobject.md b/docs/development/core/server/kibana-plugin-core-server.validateobject.md index 1772c21c873252..638fe6a05d3e15 100644 --- a/docs/development/core/server/kibana-plugin-core-server.validateobject.md +++ b/docs/development/core/server/kibana-plugin-core-server.validateobject.md @@ -14,7 +14,7 @@ export declare function validateObject(obj: any): void; | Parameter | Type | Description | | --- | --- | --- | -| obj | any | | +| obj | any | Is used to validate object properties for containing constructor and prototype with detecting circular references Returns: diff --git a/src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap b/packages/kbn-std/src/__snapshots__/validate_object.test.ts.snap similarity index 100% rename from src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap rename to packages/kbn-std/src/__snapshots__/validate_object.test.ts.snap diff --git a/packages/kbn-std/src/index.ts b/packages/kbn-std/src/index.ts index c111428017539e..fb0d6e5db2bba6 100644 --- a/packages/kbn-std/src/index.ts +++ b/packages/kbn-std/src/index.ts @@ -27,4 +27,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/src/core/server/http/prototype_pollution/validate_object.test.ts b/packages/kbn-std/src/validate_object.test.ts similarity index 100% rename from src/core/server/http/prototype_pollution/validate_object.test.ts rename to packages/kbn-std/src/validate_object.test.ts diff --git a/src/core/server/http/prototype_pollution/validate_object.ts b/packages/kbn-std/src/validate_object.ts similarity index 100% rename from src/core/server/http/prototype_pollution/validate_object.ts rename to packages/kbn-std/src/validate_object.ts diff --git a/src/core/server/http/http_tools.ts b/src/core/server/http/http_tools.ts index 8bec26f31fa26b..a215dd8e073751 100644 --- a/src/core/server/http/http_tools.ts +++ b/src/core/server/http/http_tools.ts @@ -29,8 +29,8 @@ import Hoek from '@hapi/hoek'; import type { ServerOptions as TLSOptions } from 'https'; import type { ValidationError } from 'joi'; import uuid from 'uuid'; +import { validateObject } from '@kbn/std'; import { HttpConfig } from './http_config'; -import { validateObject } from './prototype_pollution'; const corsAllowedHeaders = ['Accept', 'Authorization', 'Content-Type', 'If-None-Match', 'kbn-xsrf']; /** diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index 06ea15e89ce0b3..cb842b2f602685 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -94,4 +94,3 @@ export { } from './cookie_session_storage'; export * from './types'; export { BasePath, IBasePath } from './base_path_service'; -export { validateObject } from './prototype_pollution'; diff --git a/src/core/server/http/prototype_pollution/index.ts b/src/core/server/http/prototype_pollution/index.ts deleted file mode 100644 index e1a33ffba155e8..00000000000000 --- a/src/core/server/http/prototype_pollution/index.ts +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -export { validateObject } from './validate_object'; diff --git a/src/core/server/index.ts b/src/core/server/index.ts index b007def4e81363..0f2761b67437db 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -201,7 +201,6 @@ export { RouteConfigOptionsBody, RouteContentType, validBodyOutput, - validateObject, RouteValidatorConfig, RouteValidationSpec, RouteValidationFunction, diff --git a/src/plugins/vis_type_timeseries/server/routes/vis.ts b/src/plugins/vis_type_timeseries/server/routes/vis.ts index 1ae6aa025eb6a1..ee23e5439511b4 100644 --- a/src/plugins/vis_type_timeseries/server/routes/vis.ts +++ b/src/plugins/vis_type_timeseries/server/routes/vis.ts @@ -19,12 +19,12 @@ import { IRouter, KibanaRequest } from 'kibana/server'; import { schema } from '@kbn/config-schema'; +import { validateObject } from '@kbn/std'; import { getVisData, GetVisDataOptions } from '../lib/get_vis_data'; import { visPayloadSchema } from '../../common/vis_schema'; import { ROUTES } from '../../common/constants'; import { ValidationTelemetryServiceSetup } from '../index'; import { Framework } from '../plugin'; -import { validateObject } from '../../../../core/server'; const escapeHatch = schema.object({}, { unknowns: 'allow' }); From 7b9aff076dd2f65cdfdaa4f6d4d1b8f7477dc088 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Fri, 18 Dec 2020 18:50:17 +0300 Subject: [PATCH 5/7] Update Doc API Changes --- ...ibana-plugin-core-server.validateobject.md | 22 ------------------- src/core/server/server.api.md | 5 ----- 2 files changed, 27 deletions(-) delete mode 100644 docs/development/core/server/kibana-plugin-core-server.validateobject.md diff --git a/docs/development/core/server/kibana-plugin-core-server.validateobject.md b/docs/development/core/server/kibana-plugin-core-server.validateobject.md deleted file mode 100644 index 638fe6a05d3e15..00000000000000 --- a/docs/development/core/server/kibana-plugin-core-server.validateobject.md +++ /dev/null @@ -1,22 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [validateObject](./kibana-plugin-core-server.validateobject.md) - -## validateObject() function - -Signature: - -```typescript -export declare function validateObject(obj: any): void; -``` - -## Parameters - -| Parameter | Type | Description | -| --- | --- | --- | -| obj | any | Is used to validate object properties for containing constructor and prototype with detecting circular references - -Returns: - -`void` - diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index eed702158e9bdd..5f07a4b5230560 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2985,11 +2985,6 @@ export interface UserProvidedValues { userValue?: T; } -// Warning: (ae-missing-release-tag) "validateObject" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) -// -// @public (undocumented) -export function validateObject(obj: any): void; - // @public export const validBodyOutput: readonly ["data", "stream"]; From 4261fbeea99fa3a0d34e0df4a87512e7a9061612 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Mon, 21 Dec 2020 11:10:03 +0300 Subject: [PATCH 6/7] Rename validateObject function to ensureNoUnsafeProperties --- ...t.ts.snap => ensure_no_unsafe_properties.test.ts.snap} | 0 ...object.test.ts => ensure_no_unsafe_properties.test.ts} | 8 ++++---- ...{validate_object.ts => ensure_no_unsafe_properties.ts} | 2 +- packages/kbn-std/src/index.ts | 2 +- src/core/server/http/http_tools.ts | 4 ++-- 5 files changed, 8 insertions(+), 8 deletions(-) rename packages/kbn-std/src/__snapshots__/{validate_object.test.ts.snap => ensure_no_unsafe_properties.test.ts.snap} (100%) rename packages/kbn-std/src/{validate_object.test.ts => ensure_no_unsafe_properties.test.ts} (89%) rename packages/kbn-std/src/{validate_object.ts => ensure_no_unsafe_properties.ts} (97%) diff --git a/packages/kbn-std/src/__snapshots__/validate_object.test.ts.snap b/packages/kbn-std/src/__snapshots__/ensure_no_unsafe_properties.test.ts.snap similarity index 100% rename from packages/kbn-std/src/__snapshots__/validate_object.test.ts.snap rename to packages/kbn-std/src/__snapshots__/ensure_no_unsafe_properties.test.ts.snap diff --git a/packages/kbn-std/src/validate_object.test.ts b/packages/kbn-std/src/ensure_no_unsafe_properties.test.ts similarity index 89% rename from packages/kbn-std/src/validate_object.test.ts rename to packages/kbn-std/src/ensure_no_unsafe_properties.test.ts index 23d6c4ae3b49f9..c12626b8d777ef 100644 --- a/packages/kbn-std/src/validate_object.test.ts +++ b/packages/kbn-std/src/ensure_no_unsafe_properties.test.ts @@ -17,14 +17,14 @@ * under the License. */ -import { validateObject } from './validate_object'; +import { ensureNoUnsafeProperties } from './ensure_no_unsafe_properties'; test(`fails on circular references`, () => { const foo: Record = {}; foo.myself = foo; expect(() => - validateObject({ + ensureNoUnsafeProperties({ payload: foo, }) ).toThrowErrorMatchingInlineSnapshot(`"circular reference detected"`); @@ -57,7 +57,7 @@ test(`fails on circular references`, () => { [property]: value, }; test(`can submit ${JSON.stringify(obj)}`, () => { - expect(() => validateObject(obj)).not.toThrowError(); + expect(() => ensureNoUnsafeProperties(obj)).not.toThrowError(); }); }); }); @@ -74,6 +74,6 @@ test(`fails on circular references`, () => { JSON.parse(`{ "foo": { "bar": { "constructor": { "prototype" : null } } } }`), ].forEach((value) => { test(`can't submit ${JSON.stringify(value)}`, () => { - expect(() => validateObject(value)).toThrowErrorMatchingSnapshot(); + expect(() => ensureNoUnsafeProperties(value)).toThrowErrorMatchingSnapshot(); }); }); diff --git a/packages/kbn-std/src/validate_object.ts b/packages/kbn-std/src/ensure_no_unsafe_properties.ts similarity index 97% rename from packages/kbn-std/src/validate_object.ts rename to packages/kbn-std/src/ensure_no_unsafe_properties.ts index cab6ce295ce92a..47cbea5ecf3ee7 100644 --- a/packages/kbn-std/src/validate_object.ts +++ b/packages/kbn-std/src/ensure_no_unsafe_properties.ts @@ -31,7 +31,7 @@ const hasOwnProperty = (obj: any, property: string) => 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) { +export function ensureNoUnsafeProperties(obj: any) { if (!isObject(obj)) { return; } diff --git a/packages/kbn-std/src/index.ts b/packages/kbn-std/src/index.ts index fb0d6e5db2bba6..a5b5088f9105fa 100644 --- a/packages/kbn-std/src/index.ts +++ b/packages/kbn-std/src/index.ts @@ -27,5 +27,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 { ensureNoUnsafeProperties } from './ensure_no_unsafe_properties'; export * from './rxjs_7'; diff --git a/src/core/server/http/http_tools.ts b/src/core/server/http/http_tools.ts index a215dd8e073751..f09f3dc2730a17 100644 --- a/src/core/server/http/http_tools.ts +++ b/src/core/server/http/http_tools.ts @@ -29,7 +29,7 @@ import Hoek from '@hapi/hoek'; import type { ServerOptions as TLSOptions } from 'https'; import type { ValidationError } from 'joi'; import uuid from 'uuid'; -import { validateObject } from '@kbn/std'; +import { ensureNoUnsafeProperties } from '@kbn/std'; import { HttpConfig } from './http_config'; const corsAllowedHeaders = ['Accept', 'Authorization', 'Content-Type', 'If-None-Match', 'kbn-xsrf']; @@ -69,7 +69,7 @@ export function getServerOptions(config: HttpConfig, { configureTLS = true } = { // This is a default payload validation which applies to all LP routes which do not specify their own // `validate.payload` handler, in order to reduce the likelyhood of prototype pollution vulnerabilities. // (All NP routes are already required to specify their own validation in order to access the payload) - payload: (value) => Promise.resolve(validateObject(value)), + payload: (value) => Promise.resolve(ensureNoUnsafeProperties(value)), }, }, state: { From 8f1f8a90a90e031a22ce463a96eb3ffa81489518 Mon Sep 17 00:00:00 2001 From: Diana Derevyankina Date: Mon, 21 Dec 2020 11:18:48 +0300 Subject: [PATCH 7/7] Rename other validateObject occurrences --- src/plugins/vis_type_timeseries/server/routes/vis.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/vis_type_timeseries/server/routes/vis.ts b/src/plugins/vis_type_timeseries/server/routes/vis.ts index ee23e5439511b4..3ed9aaaaea2268 100644 --- a/src/plugins/vis_type_timeseries/server/routes/vis.ts +++ b/src/plugins/vis_type_timeseries/server/routes/vis.ts @@ -19,7 +19,7 @@ import { IRouter, KibanaRequest } from 'kibana/server'; import { schema } from '@kbn/config-schema'; -import { validateObject } from '@kbn/std'; +import { ensureNoUnsafeProperties } from '@kbn/std'; import { getVisData, GetVisDataOptions } from '../lib/get_vis_data'; import { visPayloadSchema } from '../../common/vis_schema'; import { ROUTES } from '../../common/constants'; @@ -42,7 +42,7 @@ export const visDataRoutes = ( }, async (requestContext, request, response) => { try { - validateObject(request.body); + ensureNoUnsafeProperties(request.body); } catch (error) { return response.badRequest({ body: error.message,