Skip to content

Secure json parsing#1110

Merged
delvedor merged 2 commits intomasterfrom
fix-json-parsing
Mar 12, 2020
Merged

Secure json parsing#1110
delvedor merged 2 commits intomasterfrom
fix-json-parsing

Conversation

@delvedor
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member Author

delvedor commented Mar 8, 2021

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

@matAtWork
Copy link
Copy Markdown

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