From 62e57599b120fedba331973b670d8961fa1076ed Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 15 Nov 2019 12:18:36 -0500 Subject: [PATCH] [7.x] Default payload validation (#48753) (#50784) * trial for default payload validation * relaxing default validation * some cleanup and testing * update xsrf integration test * adding API smoke tests * fixing types * removing Joi extensions * updating tests * documenting changes * fixing NP validation bypass * fix lint problems * Update src/legacy/server/http/integration_tests/xsrf.test.js * Update src/legacy/server/http/integration_tests/xsrf.test.js * revert test changes * simplifying tests Co-authored-by: Elastic Machine --- .../server/http/base_path_proxy_server.ts | 2 + src/core/server/http/http_server.ts | 7 ++ src/core/server/http/http_tools.ts | 6 ++ .../validate_object.test.ts.snap | 13 +++ .../server/http/prototype_pollution/index.ts | 20 +++++ .../validate_object.test.ts | 79 ++++++++++++++++++ .../prototype_pollution/validate_object.ts | 80 +++++++++++++++++++ .../console/server/proxy_route.js | 1 + .../http/integration_tests/xsrf.test.js | 6 +- test/api_integration/apis/general/index.js | 1 + .../apis/general/prototype_pollution.ts | 57 +++++++++++++ .../api_integration/ftr_provider_context.d.ts | 24 ++++++ 12 files changed, 294 insertions(+), 2 deletions(-) create mode 100644 src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap create mode 100644 src/core/server/http/prototype_pollution/index.ts create mode 100644 src/core/server/http/prototype_pollution/validate_object.test.ts create mode 100644 src/core/server/http/prototype_pollution/validate_object.ts create mode 100644 test/api_integration/apis/general/prototype_pollution.ts create mode 100644 test/api_integration/ftr_provider_context.d.ts diff --git a/src/core/server/http/base_path_proxy_server.ts b/src/core/server/http/base_path_proxy_server.ts index ff7fee0198f681..cde35f3cbe995a 100644 --- a/src/core/server/http/base_path_proxy_server.ts +++ b/src/core/server/http/base_path_proxy_server.ts @@ -143,6 +143,7 @@ export class BasePathProxyServer { return responseToolkit.continue; }, ], + validate: { payload: true }, }, path: `${this.httpConfig.basePath}/{kbnPath*}`, }); @@ -175,6 +176,7 @@ export class BasePathProxyServer { return responseToolkit.continue; }, ], + validate: { payload: true }, }, path: `/__UNSAFE_bypassBasePath/{kbnPath*}`, }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 3354324c12407d..da97ab535516c8 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -128,6 +128,8 @@ export class HttpServer { for (const route of router.getRoutes()) { this.log.debug(`registering route handler for [${route.path}]`); const { authRequired = true, tags } = route.options; + // Hapi does not allow payload validation to be specified for 'head' or 'get' requests + const validate = ['head', 'get'].includes(route.method) ? undefined : { payload: true }; this.server.route({ handler: route.handler, method: route.method, @@ -135,6 +137,11 @@ export class HttpServer { options: { auth: authRequired ? undefined : false, tags: tags ? Array.from(tags) : undefined, + // TODO: This 'validate' section can be removed once the legacy platform is completely removed. + // We are telling Hapi that NP routes can accept any payload, so that it can bypass the default + // validation applied in ./http_tools#getServerOptions + // (All NP routes are already required to specify their own validation in order to access the payload) + validate, }, }); } diff --git a/src/core/server/http/http_tools.ts b/src/core/server/http/http_tools.ts index 88164a76c66f07..22468a5b252f42 100644 --- a/src/core/server/http/http_tools.ts +++ b/src/core/server/http/http_tools.ts @@ -23,6 +23,7 @@ import Hoek from 'hoek'; import { ServerOptions as TLSOptions } from 'https'; import { ValidationError } from 'joi'; import { HttpConfig } from './http_config'; +import { validateObject } from './prototype_pollution'; /** * Converts Kibana `HttpConfig` into `ServerOptions` that are accepted by the Hapi server. @@ -45,6 +46,11 @@ export function getServerOptions(config: HttpConfig, { configureTLS = true } = { options: { abortEarly: false, }, + // TODO: This payload validation can be removed once the legacy platform is completely removed. + // 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)), }, }, state: { diff --git a/src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap b/src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap new file mode 100644 index 00000000000000..937e040c771ee1 --- /dev/null +++ b/src/core/server/http/prototype_pollution/__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/src/core/server/http/prototype_pollution/index.ts b/src/core/server/http/prototype_pollution/index.ts new file mode 100644 index 00000000000000..e1a33ffba155e8 --- /dev/null +++ b/src/core/server/http/prototype_pollution/index.ts @@ -0,0 +1,20 @@ +/* + * 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/http/prototype_pollution/validate_object.test.ts b/src/core/server/http/prototype_pollution/validate_object.test.ts new file mode 100644 index 00000000000000..9e23d6cec64445 --- /dev/null +++ b/src/core/server/http/prototype_pollution/validate_object.test.ts @@ -0,0 +1,79 @@ +/* + * 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. + */ + +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/src/core/server/http/prototype_pollution/validate_object.ts b/src/core/server/http/prototype_pollution/validate_object.ts new file mode 100644 index 00000000000000..cab6ce295ce92a --- /dev/null +++ b/src/core/server/http/prototype_pollution/validate_object.ts @@ -0,0 +1,80 @@ +/* + * 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. + */ + +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/legacy/core_plugins/console/server/proxy_route.js b/src/legacy/core_plugins/console/server/proxy_route.js index 8ce828879a677a..856128f3d4c031 100644 --- a/src/legacy/core_plugins/console/server/proxy_route.js +++ b/src/legacy/core_plugins/console/server/proxy_route.js @@ -71,6 +71,7 @@ export const createProxyRoute = ({ parse: false, }, validate: { + payload: true, query: Joi.object() .keys({ method: Joi.string() diff --git a/src/legacy/server/http/integration_tests/xsrf.test.js b/src/legacy/server/http/integration_tests/xsrf.test.js index 562a94e198631b..baeb61bff6113c 100644 --- a/src/legacy/server/http/integration_tests/xsrf.test.js +++ b/src/legacy/server/http/integration_tests/xsrf.test.js @@ -57,7 +57,8 @@ describe('xsrf request filter', () => { // Disable payload parsing to make HapiJS server accept any content-type header. payload: { parse: false - } + }, + validate: { payload: null } }, handler: async function () { return 'ok'; @@ -71,7 +72,8 @@ describe('xsrf request filter', () => { // Disable payload parsing to make HapiJS server accept any content-type header. payload: { parse: false - } + }, + validate: { payload: null } }, handler: async function () { return 'ok'; diff --git a/test/api_integration/apis/general/index.js b/test/api_integration/apis/general/index.js index 86b7565cba6de0..f8daff1a6e8a89 100644 --- a/test/api_integration/apis/general/index.js +++ b/test/api_integration/apis/general/index.js @@ -21,5 +21,6 @@ export default function ({ loadTestFile }) { describe('general', () => { loadTestFile(require.resolve('./cookies')); loadTestFile(require.resolve('./csp')); + loadTestFile(require.resolve('./prototype_pollution')); }); } diff --git a/test/api_integration/apis/general/prototype_pollution.ts b/test/api_integration/apis/general/prototype_pollution.ts new file mode 100644 index 00000000000000..1b732dc81afa92 --- /dev/null +++ b/test/api_integration/apis/general/prototype_pollution.ts @@ -0,0 +1,57 @@ +/* + * 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. + */ + +import { FtrProviderContext } from 'test/api_integration/ftr_provider_context'; + +// eslint-disable-next-line import/no-default-export +export default function({ getService }: FtrProviderContext) { + const supertest = getService('supertest'); + + describe('prototype pollution smoke test', () => { + it('prevents payloads with the "constructor.prototype" pollution vector from being accepted', async () => { + await supertest + .post('/api/sample_data/some_data_id') + .send([ + { + constructor: { + prototype: 'foo', + }, + }, + ]) + .expect(400, { + statusCode: 400, + error: 'Bad Request', + message: "'constructor.prototype' is an invalid key", + validation: { source: 'payload', keys: [] }, + }); + }); + + it('prevents payloads with the "__proto__" pollution vector from being accepted', async () => { + await supertest + .post('/api/sample_data/some_data_id') + .send(JSON.parse(`{"foo": { "__proto__": {} } }`)) + .expect(400, { + statusCode: 400, + error: 'Bad Request', + message: "'__proto__' is an invalid key", + validation: { source: 'payload', keys: [] }, + }); + }); + }); +} diff --git a/test/api_integration/ftr_provider_context.d.ts b/test/api_integration/ftr_provider_context.d.ts new file mode 100644 index 00000000000000..60f4914a1d27e1 --- /dev/null +++ b/test/api_integration/ftr_provider_context.d.ts @@ -0,0 +1,24 @@ +/* + * 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. + */ + +import { GenericFtrProviderContext } from '@kbn/test/types/ftr'; + +import { services } from './services'; + +export type FtrProviderContext = GenericFtrProviderContext;