-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Default payload validation #48753
Default payload validation #48753
Changes from 19 commits
9275412
a0ef9c8
dba58c8
d798473
069678b
4e8b8ee
111271d
999c545
2136caa
8b7389a
d6ae8bb
b3a1bc8
477ae48
4718cf7
fce44bb
8eb156e
387ed6c
a6486f4
5b362af
e8324c7
ea33019
4bdb0da
c0d8834
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about query validation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are mostly concerned with payload validation, as that's the most common offender for nested data structures. We may add similar validations for query, path, and header in the future |
||
}, | ||
}, | ||
state: { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* 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 => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Ensuring |
||
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 => { | ||
['headers', 'payload', 'query', 'params'].forEach(property => { | ||
const obj = { | ||
[property]: value, | ||
}; | ||
test(`can't submit ${JSON.stringify(obj)}`, () => { | ||
expect(() => validateObject(obj)).toThrowErrorMatchingSnapshot(); | ||
}); | ||
}); | ||
}); |
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @legrego why did you add a throw here instead of simply a |
||
} | ||
|
||
seen.add(childValue); | ||
} | ||
|
||
stack.push({ | ||
value: childValue, | ||
previousKey: key, | ||
}); | ||
} | ||
} | ||
} |
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: [] }, | ||
}); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that we can remove it. NP plugins might disable validation https://github.com/restrry/kibana/blob/841abd1162f1eb6aaebf27d004c92e8344636d91/src/core/server/http/router/route.ts#L78-L80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks for the heads-up! Are we sure we want to allow this behavior? Do we have concrete use cases for disabling the
config-schema
validation at this time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some cases when we don't know the shape of an object upfront.
https://github.com/restrry/kibana/blob/841abd1162f1eb6aaebf27d004c92e8344636d91/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js#L204
https://github.com/elastic/kibana/pull/48413/files#diff-fbd7aa2d4390350fcd1ffd047c33af25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point -- we do know that payloads will at least be objects or arrays though, right? Could we at least enforce something like
schema.object({}, { allowUnknowns: true })
at route handlers?We are proposing a followup PR to this one which will incorporate these default validations into the
schema.object
call itself, so that all NP routes will have these protections in place, unless they explicitly opt-out of them viaschema.unsafeObject({})
or similar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already enforce. Don't we? Plugins cannot get access to a body without declaring validation
https://github.com/elastic/kibana/blob/master/src/core/server/http/router/request.ts#L106-L109
Although they can lax the validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I misunderstood. I thought NP provided a way to prevent any validation (not even
schema.any()
, and still get access to the underlying payload. I think we're OK as-is then. Our followup PR should protect these routes as well.