Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow user data contain special JS words #109425

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Allow user data contain special JS words #109425

merged 5 commits into from
Aug 31, 2021

Conversation

mshustov
Copy link
Contributor

Summary

Closes #101944
Disables disablePrototypePoisoningProtection protection on ES client not to make an assumption on user-supplied data.

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 20, 2021
@mshustov mshustov requested a review from a team as a code owner August 20, 2021 11:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

pgayvallet
pgayvallet approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically LGTM, I'll let security confirm that they're fine with that.

mattkime
mattkime approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works, I checked the field caps api. That said, __proto__ starts with an underscore so it won't appear in the field list - #2551 - however, you do get a field list with this PR which is a considerable improvement.

@jportner
Copy link
Contributor

Thanks for the PR, I want to get a few more peoples feedback on this before we push this through. I'll plan to revisit this next week 👍

@jportner
Copy link
Contributor

Thanks for the PR, I want to get a few more peoples feedback on this before we push this through. I'll plan to revisit this next week 👍

I started another discussion with an alternative proposal in #109544.

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

jportner
jportner approved these changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (see #109544 (comment))

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@kobelb kobelb self-requested a review August 31, 2021 15:34
kobelb
kobelb approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm alright with us doing this for the short-term. The protection that is offered by the Elasticsearch client is insufficient as it prevents users from viewing valid data that they've ingested into Elasticsearch.

However, as I've stated a few times, I really think we need a plan to prevent prototype-pollution. Almost all Elasticsearch documents should be treated as containing user-specified input and handled very carefully.

@legrego
Copy link
Member

legrego commented Aug 31, 2021

However, as I've stated a few times, I really think we need a plan to prevent prototype-pollution. Almost all Elasticsearch documents should be treated as containing user-specified input and handled very carefully.

++ agreed, we need to prioritize a cohesive plan for this. The issue is light right now, but we can use #58040 to track this work.

@mshustov mshustov added v7.16.0 and removed v7.15.0 labels Aug 31, 2021
@mshustov mshustov added v7.15.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 31, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mshustov mshustov merged commit a3fd138 into elastic:master Aug 31, 2021
@mshustov mshustov deleted the issue-101944-disable-proto-protection branch August 31, 2021 18:06
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 31, 2021
@mshustov @kibanamachine
do not make an assumption on user-supplied data content (elastic#109425)
9496a1d 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Aug 31, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 31, 2021
@mshustov @kibanamachine
do not make an assumption on user-supplied data content (elastic#109425)
05b3273 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Aug 31, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.15
✅ 7.x

The backport PRs will be merged automatically after passing CI.

@jportner jportner mentioned this pull request Aug 31, 2021
Closed
kibanamachine added a commit that referenced this pull request Aug 31, 2021
@kibanamachine @mshustov
do not make an assumption on user-supplied data content (#109425) (#1…
5a89b4b 
…10688)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Mikhail Shustov <restrry@gmail.com>
kibanamachine added a commit that referenced this pull request Aug 31, 2021
@kibanamachine @mshustov
do not make an assumption on user-supplied data content (#109425) (#1…
4689cef 
…10686)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Mikhail Shustov <restrry@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattkime mattkime mattkime approved these changes

@kobelb kobelb kobelb approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@jportner jportner jportner approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.15.0 v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to use index patterns against indices with a __proto__ field
8 participants
@mshustov @elasticmachine @jportner @legrego @kibanamachine @mattkime @kobelb @pgayvallet