Skip to content

Commit 62e5759

Browse files
[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 <[email protected]>
1 parent ec70565 commit 62e5759

File tree

12 files changed

+294
-2
lines changed

12 files changed

+294
-2
lines changed

src/core/server/http/base_path_proxy_server.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ export class BasePathProxyServer {
143143
return responseToolkit.continue;
144144
},
145145
],
146+
validate: { payload: true },
146147
},
147148
path: `${this.httpConfig.basePath}/{kbnPath*}`,
148149
});
@@ -175,6 +176,7 @@ export class BasePathProxyServer {
175176
return responseToolkit.continue;
176177
},
177178
],
179+
validate: { payload: true },
178180
},
179181
path: `/__UNSAFE_bypassBasePath/{kbnPath*}`,
180182
});

src/core/server/http/http_server.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,20 @@ export class HttpServer {
128128
for (const route of router.getRoutes()) {
129129
this.log.debug(`registering route handler for [${route.path}]`);
130130
const { authRequired = true, tags } = route.options;
131+
// Hapi does not allow payload validation to be specified for 'head' or 'get' requests
132+
const validate = ['head', 'get'].includes(route.method) ? undefined : { payload: true };
131133
this.server.route({
132134
handler: route.handler,
133135
method: route.method,
134136
path: route.path,
135137
options: {
136138
auth: authRequired ? undefined : false,
137139
tags: tags ? Array.from(tags) : undefined,
140+
// TODO: This 'validate' section can be removed once the legacy platform is completely removed.
141+
// We are telling Hapi that NP routes can accept any payload, so that it can bypass the default
142+
// validation applied in ./http_tools#getServerOptions
143+
// (All NP routes are already required to specify their own validation in order to access the payload)
144+
validate,
138145
},
139146
});
140147
}

src/core/server/http/http_tools.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import Hoek from 'hoek';
2323
import { ServerOptions as TLSOptions } from 'https';
2424
import { ValidationError } from 'joi';
2525
import { HttpConfig } from './http_config';
26+
import { validateObject } from './prototype_pollution';
2627

2728
/**
2829
* Converts Kibana `HttpConfig` into `ServerOptions` that are accepted by the Hapi server.
@@ -45,6 +46,11 @@ export function getServerOptions(config: HttpConfig, { configureTLS = true } = {
4546
options: {
4647
abortEarly: false,
4748
},
49+
// TODO: This payload validation can be removed once the legacy platform is completely removed.
50+
// This is a default payload validation which applies to all LP routes which do not specify their own
51+
// `validate.payload` handler, in order to reduce the likelyhood of prototype pollution vulnerabilities.
52+
// (All NP routes are already required to specify their own validation in order to access the payload)
53+
payload: value => Promise.resolve(validateObject(value)),
4854
},
4955
},
5056
state: {

src/core/server/http/prototype_pollution/__snapshots__/validate_object.test.ts.snap

Lines changed: 13 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
export { validateObject } from './validate_object';
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
import { validateObject } from './validate_object';
21+
22+
test(`fails on circular references`, () => {
23+
const foo: Record<string, any> = {};
24+
foo.myself = foo;
25+
26+
expect(() =>
27+
validateObject({
28+
payload: foo,
29+
})
30+
).toThrowErrorMatchingInlineSnapshot(`"circular reference detected"`);
31+
});
32+
33+
[
34+
{
35+
foo: true,
36+
bar: '__proto__',
37+
baz: 1.1,
38+
qux: undefined,
39+
quux: () => null,
40+
quuz: Object.create(null),
41+
},
42+
{
43+
foo: {
44+
foo: true,
45+
bar: '__proto__',
46+
baz: 1.1,
47+
qux: undefined,
48+
quux: () => null,
49+
quuz: Object.create(null),
50+
},
51+
},
52+
{ constructor: { foo: { prototype: null } } },
53+
{ prototype: { foo: { constructor: null } } },
54+
].forEach(value => {
55+
['headers', 'payload', 'query', 'params'].forEach(property => {
56+
const obj = {
57+
[property]: value,
58+
};
59+
test(`can submit ${JSON.stringify(obj)}`, () => {
60+
expect(() => validateObject(obj)).not.toThrowError();
61+
});
62+
});
63+
});
64+
65+
// if we use the object literal syntax to create the following values, we end up
66+
// actually reassigning the __proto__ which makes it be a non-enumerable not-own property
67+
// which isn't what we want to test here
68+
[
69+
JSON.parse(`{ "__proto__": null }`),
70+
JSON.parse(`{ "foo": { "__proto__": true } }`),
71+
JSON.parse(`{ "foo": { "bar": { "__proto__": {} } } }`),
72+
JSON.parse(`{ "constructor": { "prototype" : null } }`),
73+
JSON.parse(`{ "foo": { "constructor": { "prototype" : null } } }`),
74+
JSON.parse(`{ "foo": { "bar": { "constructor": { "prototype" : null } } } }`),
75+
].forEach(value => {
76+
test(`can't submit ${JSON.stringify(value)}`, () => {
77+
expect(() => validateObject(value)).toThrowErrorMatchingSnapshot();
78+
});
79+
});
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
interface StackItem {
21+
value: any;
22+
previousKey: string | null;
23+
}
24+
25+
// we have to do Object.prototype.hasOwnProperty because when you create an object using
26+
// Object.create(null), and I assume other methods, you get an object without a prototype,
27+
// so you can't use current.hasOwnProperty
28+
const hasOwnProperty = (obj: any, property: string) =>
29+
Object.prototype.hasOwnProperty.call(obj, property);
30+
31+
const isObject = (obj: any) => typeof obj === 'object' && obj !== null;
32+
33+
// we're using a stack instead of recursion so we aren't limited by the call stack
34+
export function validateObject(obj: any) {
35+
if (!isObject(obj)) {
36+
return;
37+
}
38+
39+
const stack: StackItem[] = [
40+
{
41+
value: obj,
42+
previousKey: null,
43+
},
44+
];
45+
const seen = new WeakSet([obj]);
46+
47+
while (stack.length > 0) {
48+
const { value, previousKey } = stack.pop()!;
49+
50+
if (!isObject(value)) {
51+
continue;
52+
}
53+
54+
if (hasOwnProperty(value, '__proto__')) {
55+
throw new Error(`'__proto__' is an invalid key`);
56+
}
57+
58+
if (hasOwnProperty(value, 'prototype') && previousKey === 'constructor') {
59+
throw new Error(`'constructor.prototype' is an invalid key`);
60+
}
61+
62+
// iterating backwards through an array is reportedly more performant
63+
const entries = Object.entries(value);
64+
for (let i = entries.length - 1; i >= 0; --i) {
65+
const [key, childValue] = entries[i];
66+
if (isObject(childValue)) {
67+
if (seen.has(childValue)) {
68+
throw new Error('circular reference detected');
69+
}
70+
71+
seen.add(childValue);
72+
}
73+
74+
stack.push({
75+
value: childValue,
76+
previousKey: key,
77+
});
78+
}
79+
}
80+
}

src/legacy/core_plugins/console/server/proxy_route.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ export const createProxyRoute = ({
7171
parse: false,
7272
},
7373
validate: {
74+
payload: true,
7475
query: Joi.object()
7576
.keys({
7677
method: Joi.string()

src/legacy/server/http/integration_tests/xsrf.test.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ describe('xsrf request filter', () => {
5757
// Disable payload parsing to make HapiJS server accept any content-type header.
5858
payload: {
5959
parse: false
60-
}
60+
},
61+
validate: { payload: null }
6162
},
6263
handler: async function () {
6364
return 'ok';
@@ -71,7 +72,8 @@ describe('xsrf request filter', () => {
7172
// Disable payload parsing to make HapiJS server accept any content-type header.
7273
payload: {
7374
parse: false
74-
}
75+
},
76+
validate: { payload: null }
7577
},
7678
handler: async function () {
7779
return 'ok';

test/api_integration/apis/general/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,5 +21,6 @@ export default function ({ loadTestFile }) {
2121
describe('general', () => {
2222
loadTestFile(require.resolve('./cookies'));
2323
loadTestFile(require.resolve('./csp'));
24+
loadTestFile(require.resolve('./prototype_pollution'));
2425
});
2526
}

0 commit comments

Comments
 (0)