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.

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

Secure json parsing #1110

Merged
merged 2 commits into from
Mar 12, 2020
Merged

Secure json parsing #1110

merged 2 commits into from
Mar 12, 2020

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Mar 12, 2020

This pull request adds a protection mechanism against malicious JSON documents, avoiding prototype pollution attacks.

Further reading: Square Brackets are the Enemy

@delvedor delvedor merged commit 6bf0447 into master Mar 12, 2020
@delvedor delvedor deleted the fix-json-parsing branch March 12, 2020 15:35
github-actions bot pushed a commit that referenced this pull request Mar 12, 2020
* Safe json parsing

* Updated test
github-actions bot pushed a commit that referenced this pull request Mar 12, 2020
* Safe json parsing

* Updated test
github-actions bot pushed a commit that referenced this pull request Mar 12, 2020
* Safe json parsing

* Updated test
@matAtWork
Copy link

matAtWork commented Mar 4, 2021

This breaks perfectly valid existing code:

Consider the request:

POST /myindex/_search
{
  "size": 0,
  "aggs": {
    "constructor": {
      "terms": {
        "field": "anything",
      }
    }
  }
}

The response is:

{
  "took": 33,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 1,
      "relation": "eq"
    },
    "max_score": null,
    "hits": []
  },
  "aggregations": {
    "constructor": { /*** CAUSES AN ERROR FOR VALID REASON ***/
      "doc_count_error_upper_bound": 0,
      "sum_other_doc_count": 0,
      "buckets": []
    }
  }
}

This will now throw a DeserializtionError on a perfectly valid response. It also occurs in many other contexts which depend on the data (such as mtermvectors) and not the request, making it impossible to avoid defensively.

@delvedor
Copy link
Member Author

delvedor commented Mar 8, 2021

Hi @matAtWork, please see #1408 (comment).

@matAtWork
Copy link

Warning to other users: this PR introduces a breaking change that is dependent on the data in your ES index.

More details are in the above issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants