Fixed incomplete JSON body on count request making org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder go into endless loop #26680

original-brownbear · 2017-09-16T20:16:56Z

Took a shot at #26083 here :)

The problem is that parser.nextToken() will return null when hitting the end a JSON string. So if the String is partly valid like an empty string:

for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) {

will loop forever because nextToken() will keep returning null.
You can see this by one core going to 100% load while a request like:

curl -XGET  -H 'Content-Type: application/json' http://localhost:9200/\*/_count -d '""'

blocks forever.

In 5.2 this seems to have been caught in the logic guessing the content type of the request and the above example returned:

{"error":{"root_cause":[{"type":"parse_exception","reason":"Failed to derive xcontent"}],"type":"parse_exception","reason":"Failed to derive xcontent"},"status":400}%

Since this is no longer the cause of the problem, I took a shot at a more appropriate error and it now returns:

"error":{"root_cause":[{"type":"parsing_exception","reason":"Failed to parse","line":1,"col":1}],"type":"parsing_exception","reason":"Failed to parse","line":1,"col":1,"caused_by":{"type":"e_o_f_exception","reason":"Unexpected end of JSON request body."}},"status":400}%

Fixes #26083

cbuescher · 2017-09-18T11:02:43Z

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

@@ -244,6 +245,9 @@ private static QueryBuilder parseTopLevelQueryBuilder(XContentParser parser) {
        try {
            QueryBuilder queryBuilder = null;
            for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) {
+                if (token == null) {
+                    throw new EOFException("Unexpected end of JSON request body.");


I'd rather like to have a check outside the parsing loop that asserts that the first token the parser emmits is XContentParser.Token.START_OBJECT. I think its save to assume we are expecting full json objects. I'd also just throw a ParsingException in that case.

@cbuescher but if we just verify that the first token is XContentParser.Token.START_OBJECT, we'll still see this infinite loop for "{" for example. Do you mean we want to accept that?
Is it really a good idea to have the behavior of org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder be to loop forever on part of a valid JSON request? :)

we'll still see this infinite loop for "{" for example.

I'd expect the parser to throw an exception on this?

Is it really a good idea to have the behavior of org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder be to loop forever on part of a valid JSON request? :)

Of course not, and this is not what @cbuescher said.

@tlrx

I'd expect the parser to throw an exception on this?

Currently, it doesn't throw on half a valid JSON request that's where my confusion was coming from. Also just goes into the infinite loop then.

I just implemented the tests @cbuescher asked for below (good call obviously it started throwing on empty string now too :)).
The best fix I see that includes the empty string case as well would be to do this:

if (parser.nextToken() == null) { return null; } for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) { if (token == null) { throw new ParsingException(parser.getTokenLocation(), "Failed to parse"); }

if the first token isn't a XContentParser.Token.START_OBJECT the parser throws eventually anyways and we handle the empty string case without much code, how about it? :)

Wops my bad :) you're right, '{' dies just fine.
So basically we have return null if the first token is null (to cover the empty string case), throw if it's not null and not ContentParser.Token.START_OBJECT and leave the loop untouched? :)

And because this is really a bit confusing (I was suprised by some of this myself), maybe add some of the above cases in tests. Or wdyt @tlrx

@cbuescher I agree

@cbuescher @tlrx done :) Added all the discussed cases here https://github.com/elastic/elasticsearch/pull/26680/files#diff-2098a5df6345871e4c3fdb934d3f9f49R72 and moved checks before the loop.

cbuescher · 2017-09-18T11:07:12Z

core/src/test/java/org/elasticsearch/rest/action/RestActionsTests.java

@@ -66,6 +66,14 @@ public void testParseTopLevelBuilderEmptyObject() throws IOException {
        }
    }

+    public void testParseTopLevelBuilderMalformedJson() throws IOException {
+        String requestBody = "\"\"";


Can you add a test for the cases "\"someString\"" and maybe also make sure that we don't barf for the empty String, but simply return null like in the case of "{}"?

Extended tests to cover this :)

cbuescher · 2017-09-18T11:12:15Z

@original-brownbear thanks, I left two small comments. I was suprised to see that we allow creating XContentParser instances from input Strings like "foo", I would have expected the Jackson JsonFactory to barf on that. I talked to @tlrx about this and unfortunately there is some open debate about whether a single string value is considered as valid JSON, otherwise I would prefer to reject request bodies of that kind altogether but that doesn't seem to be an option atm.

cbuescher

Thanks for the change, I left two other small comments regarding the error message and the corresponding test. The rest looks good to me.

cbuescher · 2017-09-18T12:55:13Z

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

+            if (first == null) {
+                return null;
+            } else if (first != XContentParser.Token.START_OBJECT) {
+                throw new ParsingException(parser.getTokenLocation(), "Failed to parse");


Can you make the message a little bit more descriptive, since we know whats wrong in this case?

@cbuescher sure, how about "Invalid JSON object, first token must be '{' but was 'VALUE_STRING'" ? (or whatever other token type it may be) I liked this better than printing the string back, which is hard to interpret for accidental string literals like in the "'{'" example if that makes sense? :)

It doesn't have to be JSON, we support Yaml, Cbor and Smile, so better to say we expected the beginning of an object or something along those lines.

Maybe do sth. similar as this: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java#L962

yea that looks perfect, sec adjusting :)

cbuescher · 2017-09-18T12:57:40Z

core/src/test/java/org/elasticsearch/rest/action/RestActionsTests.java

+            try (XContentParser parser = createParser(JsonXContent.jsonXContent, requestBody)) {
+                ParsingException exception =
+                    expectThrows(ParsingException.class, () -> RestActions.getQueryContent(parser));
+                assertEquals("Failed to parse", exception.getMessage());


If the error message is different, maybe you can differentiate between the case where we detect a missing START_OBJECT and the case where the underlying Jackson parser throws.

@cbuescher sure already done locally (as well as the broken indentation in the test), just waiting for an answer on the above :)

javanna · 2017-09-18T14:06:37Z

Question: is this a problem with the count api only? Seems like the same could happen with any API, or do we have protection in other places already?

cbuescher · 2017-09-18T14:15:49Z

@javanna As far as I can see the code path in question here (RestActions.getQueryContent()) is used by RestValidateQueryAction, RestCountAction (incl. the cat variant) and the RestExplainAction. In other places, e.g. for parsing in SearchSourceBuilder we already seem to have similar checks: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java#L962

cbuescher · 2017-09-18T14:21:22Z

Seems like the same could happen with any API, or do we have protection in other places already?

And yes, I talked to @tlrx before and it seems we fixed some of these cases on a case by case basis, there might be other places left to check though, but this is outside of the scope of this PR I think. I would have prefered that XContent#createParser would reject these cases already but that seems not to be the case and more difficult to change.

original-brownbear · 2017-09-18T14:33:27Z

@cbuescher exception message adjusted :)

cbuescher

@original-brownbear thanks a lot, looks good to me now. Let's just wait for CI to pass, there were some failures earlier on that I think were unrelated but just to be sure.
Also, would you squash and shorten the commit title a little bit before merging? Since running into this bug can be quiet nasty, I think we should backport it to all active 6.x branches and the current 5.6 branch.

original-brownbear · 2017-09-18T15:52:47Z

Build all green here https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/4516/, squashing

original-brownbear · 2017-09-18T15:56:00Z

@cbuescher done, squashed with shorter commit message :)

cbuescher · 2017-09-19T09:22:21Z

@original-brownbear thanks, do you want to merge and do the backport or should I do it?

original-brownbear · 2017-09-19T09:48:50Z

@cbuescher I don't know how you guys are handling merging and backporting, probably safest for your repo if you do it this time around :)

cbuescher · 2017-09-19T10:02:57Z

@original-brownbear thanks again for this fix, I rewrote the commit message a bit and will merge this to the active 6.x branches and 5.6 now.

original-brownbear · 2017-09-19T10:03:48Z

@cbuescher thanks!

Request bodys that only consists of a String value can lead to endless loops in the parser of several rest requests like e.g. `_count`. Up to 5.2 this seems to have been caught in the logic guessing the content type of the request, but since then it causes the node to block. This change introduces checks for receiving a valid xContent object before starting the parsing in RestActions#parseTopLevelQueryBuilder(). Closes #26083

* master: (278 commits) Move pre-6.0 node checkpoint to SequenceNumbers Invalid JSON request body caused endless loop (elastic#26680) added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery Filter pre-6.0 nodes for checkpoint invariants Skip bad request REST test on pre-6.0 Reenable BWC tests after disabling for backport Add global checkpoint tracking on the primary [Test] Fix reference/cat/allocation/line_8 test failure [Docs] improved description for fs.total.available_in_bytes (elastic#26657) Fix discovery-file plugin to use custom config path fix testSniffNodes to use the new error message Add check for invalid index in WildcardExpressionResolver (elastic#26409) ...

* master: (67 commits) Restoring from snapshot should force generation of a new history uuid (elastic#26694) test: Use a single primary shard so that the exception can caught in the same way Move pre-6.0 node checkpoint to SequenceNumbers Invalid JSON request body caused endless loop (elastic#26680) added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery Filter pre-6.0 nodes for checkpoint invariants Skip bad request REST test on pre-6.0 Reenable BWC tests after disabling for backport Add global checkpoint tracking on the primary [Test] Fix reference/cat/allocation/line_8 test failure [Docs] improved description for fs.total.available_in_bytes (elastic#26657) Fix discovery-file plugin to use custom config path ...

cbuescher requested changes Sep 18, 2017

View reviewed changes

cbuescher reviewed Sep 18, 2017

View reviewed changes

original-brownbear force-pushed the 26083 branch from dec3229 to 2a6e5e1 Compare September 18, 2017 14:33

cbuescher approved these changes Sep 18, 2017

View reviewed changes

cbuescher added :Query DSL >bug labels Sep 18, 2017

elastic#26083 Invalid JSON request body caused endless loop

65812fc

original-brownbear force-pushed the 26083 branch from 2a6e5e1 to 65812fc Compare September 18, 2017 15:55

cbuescher merged commit 2db3bcc into elastic:master Sep 19, 2017

cbuescher added the v7.0.0 label Sep 19, 2017

original-brownbear deleted the 26083 branch September 19, 2017 10:03

cbuescher added v5.6.2 v6.0.0 v6.1.0 labels Sep 19, 2017

colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed incomplete JSON body on count request making org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder go into endless loop #26680

Fixed incomplete JSON body on count request making org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder go into endless loop #26680

original-brownbear commented Sep 16, 2017 •

edited by clintongormley

Loading

cbuescher Sep 18, 2017

original-brownbear Sep 18, 2017

tlrx Sep 18, 2017

original-brownbear Sep 18, 2017

original-brownbear Sep 18, 2017

original-brownbear Sep 18, 2017

cbuescher Sep 18, 2017

cbuescher Sep 18, 2017

tlrx Sep 18, 2017

original-brownbear Sep 18, 2017 •

edited

Loading

cbuescher Sep 18, 2017

original-brownbear Sep 18, 2017

cbuescher commented Sep 18, 2017

cbuescher left a comment

cbuescher Sep 18, 2017

original-brownbear Sep 18, 2017 •

edited

Loading

cbuescher Sep 18, 2017

cbuescher Sep 18, 2017

original-brownbear Sep 18, 2017

cbuescher Sep 18, 2017

original-brownbear Sep 18, 2017 •

edited

Loading

javanna commented Sep 18, 2017

cbuescher commented Sep 18, 2017

cbuescher commented Sep 18, 2017

original-brownbear commented Sep 18, 2017

cbuescher left a comment

original-brownbear commented Sep 18, 2017

original-brownbear commented Sep 18, 2017

cbuescher commented Sep 19, 2017

original-brownbear commented Sep 19, 2017

cbuescher commented Sep 19, 2017

original-brownbear commented Sep 19, 2017

Fixed incomplete JSON body on count request making org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder go into endless loop #26680

Fixed incomplete JSON body on count request making org.elasticsearch.rest.action.RestActions#parseTopLevelQueryBuilder go into endless loop #26680

Conversation

original-brownbear commented Sep 16, 2017 • edited by clintongormley Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Sep 18, 2017

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

javanna commented Sep 18, 2017

cbuescher commented Sep 18, 2017

cbuescher commented Sep 18, 2017

original-brownbear commented Sep 18, 2017

cbuescher left a comment

Choose a reason for hiding this comment

original-brownbear commented Sep 18, 2017

original-brownbear commented Sep 18, 2017

cbuescher commented Sep 19, 2017

original-brownbear commented Sep 19, 2017

cbuescher commented Sep 19, 2017

original-brownbear commented Sep 19, 2017

original-brownbear commented Sep 16, 2017 •

edited by clintongormley

Loading

original-brownbear Sep 18, 2017 •

edited

Loading

original-brownbear Sep 18, 2017 •

edited

Loading

original-brownbear Sep 18, 2017 •

edited

Loading