Skip to content

Commit

Permalink
Default payload validation (#48753)
Browse files Browse the repository at this point in the history
* 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 <elasticmachine@users.noreply.github.com>
  • Loading branch information
legrego and elasticmachine authored Nov 15, 2019
1 parent 1dfc70f commit 1c415e0
Show file tree
Hide file tree
Showing 12 changed files with 294 additions and 2 deletions.
2 changes: 2 additions & 0 deletions src/core/server/http/base_path_proxy_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export class BasePathProxyServer {
return responseToolkit.continue;
},
],
validate: { payload: true },
},
path: `${this.httpConfig.basePath}/{kbnPath*}`,
});
Expand Down Expand Up @@ -175,6 +176,7 @@ export class BasePathProxyServer {
return responseToolkit.continue;
},
],
validate: { payload: true },
},
path: `/__UNSAFE_bypassBasePath/{kbnPath*}`,
});
Expand Down
7 changes: 7 additions & 0 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,20 @@ 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,
path: route.path,
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,
},
});
}
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/http/http_tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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: {
Expand Down

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

20 changes: 20 additions & 0 deletions src/core/server/http/prototype_pollution/index.ts
Original file line number Diff line number Diff line change
@@ -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';
79 changes: 79 additions & 0 deletions src/core/server/http/prototype_pollution/validate_object.test.ts
Original file line number Diff line number Diff line change
@@ -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<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();
});
});
80 changes: 80 additions & 0 deletions src/core/server/http/prototype_pollution/validate_object.ts
Original file line number Diff line number Diff line change
@@ -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,
});
}
}
}
1 change: 1 addition & 0 deletions src/legacy/core_plugins/console/server/proxy_route.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export const createProxyRoute = ({
parse: false,
},
validate: {
payload: true,
query: Joi.object()
.keys({
method: Joi.string()
Expand Down
6 changes: 4 additions & 2 deletions src/legacy/server/http/integration_tests/xsrf.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down
1 change: 1 addition & 0 deletions test/api_integration/apis/general/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ export default function ({ loadTestFile }) {
describe('general', () => {
loadTestFile(require.resolve('./cookies'));
loadTestFile(require.resolve('./csp'));
loadTestFile(require.resolve('./prototype_pollution'));
});
}
57 changes: 57 additions & 0 deletions test/api_integration/apis/general/prototype_pollution.ts
Original file line number Diff line number Diff line change
@@ -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: [] },
});
});
});
}
24 changes: 24 additions & 0 deletions test/api_integration/ftr_provider_context.d.ts
Original file line number Diff line number Diff line change
@@ -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<typeof services, {}>;

0 comments on commit 1c415e0

Please sign in to comment.