Browse Source

limit the depth of nested bool queries (#66204)

limit the depth of nested bool queries 

Introduce a new node level setting `indices.query.bool.max_nested_depth`
that controls the depth of nested bool queries.
Throw an error if a nested depth of a bool query exceeds the maximum
allowed nested depth.

Closes #55303
Yang Cheng 4 years ago
parent
commit
168d98b7dd

+ 8 - 1
docs/reference/modules/indices/search-settings.asciidoc

@@ -24,4 +24,11 @@ few resources.
 Maximum number of <<search-aggregations-bucket,aggregation buckets>> allowed in
 a single response. Defaults to 65,535.
 +
-Requests that attempt to return more than this limit will return an error.
+Requests that attempt to return more than this limit will return an error.
+
+[[indices-query-bool-max-nested-depth]]
+`indices.query.bool.max_nested_depth`::
+(<<static-cluster-setting,Static>>, integer) Maximum nested depth of bool queries. Defaults to `20`.
++
+This setting limits the nesting depth of bool queries. Deep nesting of boolean queries may lead to
+stack overflow.

+ 1 - 0
server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

@@ -454,6 +454,7 @@ public final class ClusterSettings extends AbstractScopedSettings {
             ResourceWatcherService.RELOAD_INTERVAL_MEDIUM,
             ResourceWatcherService.RELOAD_INTERVAL_LOW,
             SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING,
+            SearchModule.INDICES_MAX_NESTED_DEPTH_SETTING,
             ThreadPool.ESTIMATED_TIME_INTERVAL_SETTING,
             FastVectorHighlighter.SETTING_TV_HIGHLIGHT_MULTI_VALUE,
             Node.BREAKER_TYPE_KEY,

+ 5 - 1
server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java

@@ -298,6 +298,10 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
      * Parses a query excluding the query element that wraps it
      */
     public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws IOException {
+        return parseInnerQueryBuilder(parser, Integer.valueOf(0));
+    }
+
+    public static QueryBuilder parseInnerQueryBuilder(XContentParser parser, Integer nestedDepth) throws IOException {
         if (parser.currentToken() != XContentParser.Token.START_OBJECT) {
             if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
                 throw new ParsingException(parser.getTokenLocation(), "[_na] query malformed, must start with start_object");
@@ -317,7 +321,7 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
         }
         QueryBuilder result;
         try {
-            result = parser.namedObject(QueryBuilder.class, queryName, null);
+            result = parser.namedObject(QueryBuilder.class, queryName, nestedDepth);
         } catch (NamedObjectNotFoundException e) {
             String message = String.format(Locale.ROOT, "unknown query [%s]%s", queryName,
                     SuggestingErrorOnUnknown.suggest(queryName, e.getCandidates()));

+ 25 - 7
server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java

@@ -43,6 +43,7 @@ import java.util.function.Consumer;
 import java.util.stream.Stream;
 
 import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded;
+import static org.elasticsearch.search.SearchModule.INDICES_MAX_NESTED_DEPTH_SETTING;
 
 /**
  * A Query that matches documents matching boolean combinations of other queries.
@@ -59,6 +60,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
     private static final ParseField MUST = new ParseField("must");
     private static final ParseField MINIMUM_SHOULD_MATCH = new ParseField("minimum_should_match");
     private static final ParseField ADJUST_PURE_NEGATIVE = new ParseField("adjust_pure_negative");
+    private static int maxNestedDepth = 20;
 
     private final List<QueryBuilder> mustClauses = new ArrayList<>();
 
@@ -91,6 +93,17 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
         minimumShouldMatch = in.readOptionalString();
     }
 
+    /**
+     * Set the maximum nested depth of bool queries.
+     * Default value is 20.
+     */
+    public static void setMaxNestedDepth(int maxNestedDepth) {
+        if (maxNestedDepth < 1) {
+            throw new IllegalArgumentException("maxNestedDepth must be >= 1");
+        }
+        BoolQueryBuilder.maxNestedDepth = maxNestedDepth;
+    }
+
     @Override
     protected void doWriteTo(StreamOutput out) throws IOException {
         writeQueries(out, mustClauses);
@@ -271,15 +284,15 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
         builder.endArray();
     }
 
-    private static final ObjectParser<BoolQueryBuilder, Void> PARSER = new ObjectParser<>("bool", BoolQueryBuilder::new);
+    private static final ObjectParser<BoolQueryBuilder, Integer> PARSER = new ObjectParser<>("bool", BoolQueryBuilder::new);
     static {
-        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::must), (p, c) -> parseInnerQueryBuilder(p),
+        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::must), (p, c) -> parseInnerQueryBuilder(p, c),
             MUST);
-        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::should), (p, c) -> parseInnerQueryBuilder(p),
+        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::should), (p, c) -> parseInnerQueryBuilder(p, c),
             SHOULD);
-        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::mustNot), (p, c) -> parseInnerQueryBuilder(p),
+        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::mustNot), (p, c) -> parseInnerQueryBuilder(p, c),
             MUST_NOT);
-        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::filter), (p, c) -> parseInnerQueryBuilder(p),
+        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::filter), (p, c) -> parseInnerQueryBuilder(p, c),
             FILTER);
         PARSER.declareBoolean(BoolQueryBuilder::adjustPureNegative, ADJUST_PURE_NEGATIVE);
         PARSER.declareField(BoolQueryBuilder::minimumShouldMatch, (p, c) -> p.textOrNull(),
@@ -288,8 +301,13 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
         PARSER.declareFloat(BoolQueryBuilder::boost, BOOST_FIELD);
     }
 
-    public static BoolQueryBuilder fromXContent(XContentParser parser) throws IOException, ParsingException {
-        return PARSER.parse(parser, null);
+    public static BoolQueryBuilder fromXContent(XContentParser parser, Integer nestedDepth) throws IOException, ParsingException {
+        nestedDepth++;
+        if (nestedDepth > maxNestedDepth) {
+            throw new IllegalArgumentException("The nested depth of the query exceeds the maximum nested depth for bool queries set in [" +
+                INDICES_MAX_NESTED_DEPTH_SETTING.getKey() + "]");
+        }
+        return PARSER.parse(parser, nestedDepth);
     }
 
     @Override

+ 12 - 1
server/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -21,6 +21,7 @@ package org.elasticsearch.search;
 
 import org.apache.lucene.search.BooleanQuery;
 import org.elasticsearch.common.NamedRegistry;
+import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.geo.GeoShapeType;
 import org.elasticsearch.common.geo.ShapesAvailability;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
@@ -269,6 +270,9 @@ public class SearchModule {
     public static final Setting<Integer> INDICES_MAX_CLAUSE_COUNT_SETTING = Setting.intSetting("indices.query.bool.max_clause_count",
             1024, 1, Integer.MAX_VALUE, Setting.Property.NodeScope);
 
+    public static final Setting<Integer> INDICES_MAX_NESTED_DEPTH_SETTING = Setting.intSetting("indices.query.bool.max_nested_depth",
+        20, 1, Integer.MAX_VALUE, Setting.Property.NodeScope);
+
     private final Map<String, Highlighter> highlighters;
 
     private final List<FetchSubPhase> fetchSubPhases = new ArrayList<>();
@@ -778,7 +782,8 @@ public class SearchModule {
         registerQuery(new QuerySpec<>(QueryStringQueryBuilder.NAME, QueryStringQueryBuilder::new, QueryStringQueryBuilder::fromXContent));
         registerQuery(new QuerySpec<>(BoostingQueryBuilder.NAME, BoostingQueryBuilder::new, BoostingQueryBuilder::fromXContent));
         BooleanQuery.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings));
-        registerQuery(new QuerySpec<>(BoolQueryBuilder.NAME, BoolQueryBuilder::new, BoolQueryBuilder::fromXContent));
+        registerBoolQuery(new ParseField(BoolQueryBuilder.NAME), BoolQueryBuilder::new);
+        BoolQueryBuilder.setMaxNestedDepth(INDICES_MAX_NESTED_DEPTH_SETTING.get(settings));
         registerQuery(new QuerySpec<>(TermQueryBuilder.NAME, TermQueryBuilder::new, TermQueryBuilder::fromXContent));
         registerQuery(new QuerySpec<>(TermsQueryBuilder.NAME, TermsQueryBuilder::new, TermsQueryBuilder::fromXContent));
         registerQuery(new QuerySpec<>(FuzzyQueryBuilder.NAME, FuzzyQueryBuilder::new, FuzzyQueryBuilder::fromXContent));
@@ -857,6 +862,12 @@ public class SearchModule {
                 (p, c) -> spec.getParser().fromXContent(p)));
     }
 
+    private void registerBoolQuery(ParseField name, Writeable.Reader reader) {
+        namedWriteables.add(new NamedWriteableRegistry.Entry(QueryBuilder.class, name.getPreferredName(), reader));
+        namedXContents.add(new NamedXContentRegistry.Entry(QueryBuilder.class, name,
+            (p, c) -> BoolQueryBuilder.fromXContent(p, (Integer) c)));
+    }
+
     public FetchPhase getFetchPhase() {
         return new FetchPhase(fetchSubPhases);
     }

+ 16 - 0
server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java

@@ -29,6 +29,7 @@ import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.AbstractQueryTestCase;
 import org.hamcrest.Matchers;
 
@@ -39,8 +40,10 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
+import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
 import static org.elasticsearch.index.query.QueryBuilders.termQuery;
+import static org.elasticsearch.search.SearchModule.INDICES_MAX_NESTED_DEPTH_SETTING;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
@@ -447,4 +450,17 @@ public class BoolQueryBuilderTests extends AbstractQueryTestCase<BoolQueryBuilde
                 () -> boolQuery.toQuery(context));
         assertEquals("Rewrite first", e.getMessage());
     }
+
+    public void testExceedMaxNestedDepth() throws IOException {
+        BoolQueryBuilder query = new BoolQueryBuilder();
+        query.should(new BoolQueryBuilder().should(new BoolQueryBuilder().should(RandomQueryBuilder.createQuery(random()))));
+        BoolQueryBuilder.setMaxNestedDepth(2);
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, query.toString())) {
+            XContentParseException e = expectThrows(XContentParseException.class,
+                () -> parseInnerQueryBuilder(parser));
+            assertThat(e.getCause().getCause(), Matchers.instanceOf(IllegalArgumentException.class));
+            assertEquals("The nested depth of the query exceeds the maximum nested depth for bool queries set in [" +
+                INDICES_MAX_NESTED_DEPTH_SETTING.getKey() + "]", e.getCause().getCause().getMessage());
+        }
+    }
 }