-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Enable prototype pollution protection in TSVB #85952
Conversation
@elasticmachine merge upstream |
@@ -41,6 +42,7 @@ export const visDataRoutes = ( | |||
}, | |||
async (requestContext, request, response) => { | |||
try { | |||
validateObject(request.body); |
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.
@flash1293 could you please confirm that it's exactly is what you described in #78908.
Honestly I don't fully understand why we need it cause I've tried to add __proto__, prototype
properties into payload and got the following error: "Invalid request payload JSON format"
Just want to be double sure that we really need it
@elasticmachine merge upstream |
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.
I tested this and it's not doing what it should. I'm not sure how validateObject
should be used (some documentation would be great), but the request should fail if sensitive keys are used in the request body. It seems like something in the stack is already doing this for the __proto__
key, but it didn't seem to catch constructor
.
If the validation fails, it shouldn't just be logged but fail the request with a 400 status code.
I just realized I'm maybe misunderstanding how |
{
"constructor": { "prototype": "any" }
}
Our newer version of Hapi may be protecting against some cases of prototype pollution, but it doesn't catch everything. It appears to be trying to catch |
Thanks @legrego ! @dziyanadzeraviankina , @alexwizp #85952 (review) still holds :) |
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
KibanaApp changes LGTM - tested in chrome and __proto__
and constructor.prototype
is caught correctly.
@legrego Just to be sure could you take a look as well?
|
||
[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [validateObject](./kibana-plugin-core-server.validateobject.md) | ||
|
||
## validateObject() function |
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.
It might make sense to add an explanatory sentence to the documentation of this helper
I'm not sure whether we have an issue for this, but we discussed to remove the soft validation which is still in there. I guess we will tackle that on a separate PR? |
src/core/server/http/index.ts
Outdated
@@ -94,3 +94,4 @@ export { | |||
} from './cookie_session_storage'; | |||
export * from './types'; | |||
export { BasePath, IBasePath } from './base_path_service'; | |||
export { validateObject } from './prototype_pollution'; |
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.
I would really like to avoid static functions leaking outside of core. If this needs to be used in plugins, maybe moving it to a package (existing or new) would make sense. wdyt @legrego?
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.
I'm not opposed to moving this into a package. That'd certainly make it easier to consume
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.
@dziyanadzeraviankina we are currently discussing the details with @legrego. Note that if it's mandatory to have this merged for 7.11
, we could do the moving/refactoring at a later time. So considerer this approved on my side if this is urgent.
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.
I'm not sure if there's a suitable package for that, maybe it could be moved to kbn-config-schema
package, or is it better to create a new one?
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.
Would @kbn/std
work for this?
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.
Naming aside, LGTM!
Exporting a function named validateObject
from a package named @kbn/std
is really vague. It's not clear from reading this what the purpose of validateObject
is. That said, naming is hard so don't consider this a blocker. One idea: ensureNoUnsafeProperties
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.
LGTM
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.
LGTM 👍
@pgayvallet could you review the changes, please? |
Still LGTM from the KibanaApp side :) |
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.
ok for the core team changes
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Enable prototype pollution protection in TSVB Closes elastic#78908 * Update Dock API Changes * Replace logging failed in validateObject validation with 400 error * Move validateObject to kbn-std package and add a description * Update Doc API Changes * Rename validateObject function to ensureNoUnsafeProperties * Rename other validateObject occurrences Co-authored-by: Kibana Machine <[email protected]>
* Enable prototype pollution protection in TSVB Closes #78908 * Update Dock API Changes * Replace logging failed in validateObject validation with 400 error * Move validateObject to kbn-std package and add a description * Update Doc API Changes * Rename validateObject function to ensureNoUnsafeProperties * Rename other validateObject occurrences Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Diana Derevyankina <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…meline-component * 'master' of github.com:elastic/kibana: (955 commits) remove SameSite:None workaround (elastic#86994) URL encoding for URL drilldown (elastic#86902) [Security Solution] Fix few styling issues (elastic#87045) [APM] Custom links can still be created with a read only user. (elastic#87089) prevent double update (elastic#86794) Upgrade @hapi/hoek to revert hack introduced in hapi v20 upgrade (elastic#87113) [APM] Every time the new Header Icon is clicked it fetches data (elastic#87093) [APM] Add range query to service map trace walk (elastic#86631) [Discover] Deangularize navbar in context app (elastic#86353) skip "should schedule actions on legacy alerts" elastic#87010 🍾 update notice text for 2021 [logstash] remove "upgrade" functionality now that .logstash is a system index (elastic#87056) Enable prototype pollution protection in TSVB (elastic#85952) [Security Solution] add a consistent spelling of ES in Policy Response (elastic#87073) [SECURITY_SOLUTION][ENDPOINT] Delete Endpoint Policy List code (elastic#87063) Adds more URLs to the docs links service (elastic#86972) Add missing backticks in reporting-settings.asciidoc (elastic#77979) [test/functional_cors] 9000 is sometimes in use, make getPort random (elastic#87050) [Security Solution] Fix Timeline filter EuiSuperSelect styling (elastic#87033) [Lens] Fix duplicate suggestions on single-bucket charts (elastic#86996) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
Closes #78908
Summary
validateObject
function toensureNoUnsafeProperties
ensureNoUnsafeProperties
to@kbn/std
packageensureNoUnsafeProperties
in TSVB endpoint before the schema validationChecklist
For maintainers