diff --git a/CHANGELOG.md b/CHANGELOG.md index 1da2f241a4c03..5ada2ac850158 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Prevent negative fielddata stats by guarding against stale removals after shard reallocation ([#21667](https://github.com/opensearch-project/OpenSearch/pull/21667)) - Fix unbounded recursion in deserialization that can cause StackOverflowError ([#22404](https://github.com/opensearch-project/OpenSearch/pull/22404)) - Reject out-of-range WLM node threshold updates at validation time ([#22678](https://github.com/opensearch-project/OpenSearch/pull/22678)) +- Add query_string nesting depth limit to prevent StackOverflow ([#22477](https://github.com/opensearch-project/OpenSearch/pull/22477)) ### Security diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/QueryStringNestingDepthIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/QueryStringNestingDepthIT.java new file mode 100644 index 0000000000000..89f27c482a94c --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/QueryStringNestingDepthIT.java @@ -0,0 +1,138 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.search.query; + +import org.opensearch.action.search.SearchPhaseExecutionException; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.common.settings.Settings; +import org.opensearch.test.OpenSearchIntegTestCase; + +import static org.opensearch.index.query.QueryBuilders.queryStringQuery; +import static org.opensearch.search.SearchService.SEARCH_MAX_QUERY_NESTING_DEPTH; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures; +import static org.hamcrest.Matchers.containsString; + +/** + * Integration test for the search.query.max_query_nesting_depth cluster setting. + */ +public class QueryStringNestingDepthIT extends OpenSearchIntegTestCase { + + /** + * Tests the full lifecycle: + * 1. Query with depth > default limit (200) is rejected + * 2. Dynamically raise the limit + * 3. Same query now succeeds + */ + public void testNestingDepthLimitWithDynamicUpdate() throws Exception { + try { + createIndex("test"); + ensureGreen("test"); + client().prepareIndex("test").setId("1").setSource("field", "value").get(); + refresh(); + + String deepQuery = buildNestedQuery(250, "field:value"); + + // Query exceeding default depth (200) should be rejected + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { + client().prepareSearch("test").setQuery(queryStringQuery(deepQuery)).get(); + }); + assertThat(e.getDetailedMessage(), containsString("nesting depth exceeds max allowed depth 200")); + + // Dynamically raise the limit to 300 + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().put(SEARCH_MAX_QUERY_NESTING_DEPTH.getKey(), 300)) + ); + + // Same query (depth 250) should now succeed + SearchResponse response = client().prepareSearch("test").setQuery(queryStringQuery(deepQuery)).get(); + assertNoFailures(response); + assertHitCount(response, 1L); + } finally { + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().putNull(SEARCH_MAX_QUERY_NESTING_DEPTH.getKey())) + ); + } + } + + /** + * Tests that lowering the limit dynamically rejects previously-allowed queries. + */ + public void testLoweringNestingDepthRejectsQueries() throws Exception { + try { + createIndex("test_lower"); + ensureGreen("test_lower"); + client().prepareIndex("test_lower").setId("1").setSource("field", "value").get(); + refresh(); + + // Depth 50 works with default limit (200) + String query = buildNestedQuery(50, "field:value"); + SearchResponse response = client().prepareSearch("test_lower").setQuery(queryStringQuery(query)).get(); + assertNoFailures(response); + assertHitCount(response, 1L); + + // Lower the limit to 30 + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().put(SEARCH_MAX_QUERY_NESTING_DEPTH.getKey(), 30)) + ); + + // Same query (depth 50) should now be rejected + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { + client().prepareSearch("test_lower").setQuery(queryStringQuery(query)).get(); + }); + assertThat(e.getDetailedMessage(), containsString("nesting depth exceeds max allowed depth 30")); + } finally { + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().putNull(SEARCH_MAX_QUERY_NESTING_DEPTH.getKey())) + ); + } + } + + /** + * Tests that extreme nesting (under max_query_string_length) is rejected. + */ + public void testLargeNestingUnderLengthLimitIsRejected() throws Exception { + createIndex("test_large"); + ensureGreen("test_large"); + + // 15000 nested parens = 30011 chars, under the 32000 max_query_string_length + String query = buildNestedQuery(15000, "field:value"); + assertTrue("Payload must be under max_query_string_length", query.length() < 32000); + + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { + client().prepareSearch("test_large").setQuery(queryStringQuery(query)).get(); + }); + assertThat(e.getDetailedMessage(), containsString("nesting depth exceeds max allowed depth")); + } + + private String buildNestedQuery(int nestingDepth, String innerTerm) { + StringBuilder sb = new StringBuilder(nestingDepth * 2 + innerTerm.length()); + for (int i = 0; i < nestingDepth; i++) { + sb.append('('); + } + sb.append(innerTerm); + for (int i = 0; i < nestingDepth; i++) { + sb.append(')'); + } + return sb.toString(); + } +} diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 51dde9733091f..b488471c252d9 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -557,6 +557,7 @@ public void apply(Settings value, Settings current, Settings previous) { SearchService.MAX_AGGREGATION_REWRITE_FILTERS, SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING, SearchService.SEARCH_MAX_QUERY_STRING_LENGTH, + SearchService.SEARCH_MAX_QUERY_NESTING_DEPTH, SearchService.CARDINALITY_AGGREGATION_PRUNING_THRESHOLD, SearchService.KEYWORD_INDEX_OR_DOC_VALUES_ENABLED, CreatePitController.PIT_INIT_KEEP_ALIVE, diff --git a/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java b/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java index 1365bd4608835..e355c2b4d58ba 100644 --- a/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java +++ b/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java @@ -98,6 +98,7 @@ public class QueryStringQueryParser extends XQueryParser { private static final String EXISTS_FIELD = "_exists_"; @SuppressWarnings("NonFinalStaticField") private static int maxQueryStringLength = SearchService.SEARCH_MAX_QUERY_STRING_LENGTH.get(Settings.EMPTY); + private static int maxQueryNestingDepth = SearchService.SEARCH_MAX_QUERY_NESTING_DEPTH.get(Settings.EMPTY); private final QueryShardContext context; private final Map fieldsAndWeights; @@ -864,6 +865,19 @@ public Query parse(String query) throws ParseException { if (query.trim().isEmpty()) { return Queries.newMatchNoDocsQuery("Matching no documents because no terms present"); } + // Check parenthesis nesting depth before delegating to Lucene's recursive-descent parser. + // Deep nesting can cause StackOverflowError due to Lucene's recursive grammar productions. + int nestingDepth = maxParenthesisNestingDepth(query); + if (nestingDepth > maxQueryNestingDepth) { + throw new ParseException( + "Query string parenthesis nesting depth exceeds max allowed depth " + + maxQueryNestingDepth + + " (" + + SearchService.SEARCH_MAX_QUERY_NESTING_DEPTH.getKey() + + "); actual depth: " + + nestingDepth + ); + } if (query.length() > maxQueryStringLength) { throw new ParseException( "Query string length exceeds max allowed length " @@ -877,10 +891,45 @@ public Query parse(String query) throws ParseException { return super.parse(query); } + /** + * Computes the maximum parenthesis nesting depth in the given query string. + * This is a pre-parse check to prevent StackOverflowError in Lucene's + * recursive-descent classic query parser. + */ + static int maxParenthesisNestingDepth(String query) { + int maxDepth = 0; + int currentDepth = 0; + boolean inQuotes = false; + for (int i = 0; i < query.length(); i++) { + char c = query.charAt(i); + if (c == '"') { + inQuotes = !inQuotes; + } else if (!inQuotes) { + if (c == '(') { + currentDepth++; + if (currentDepth > maxDepth) { + maxDepth = currentDepth; + } + } else if (c == ')') { + currentDepth--; + } + } + } + return maxDepth; + } + /** * Sets the maximum allowed length for query strings. This should be only called from SearchService on settings updates. */ public static void setMaxQueryStringLength(int maxQueryStringLength) { QueryStringQueryParser.maxQueryStringLength = maxQueryStringLength; } + + /** + * Sets the maximum allowed parenthesis nesting depth for query strings. + * This should be only called from SearchService on settings updates. + */ + public static void setMaxQueryNestingDepth(int maxQueryNestingDepth) { + QueryStringQueryParser.maxQueryNestingDepth = maxQueryNestingDepth; + } } diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 8c29c48f81644..ed10a42cbc347 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -337,6 +337,15 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv Setting.Property.Dynamic ); + public static final Setting SEARCH_MAX_QUERY_NESTING_DEPTH = Setting.intSetting( + "search.query.max_query_nesting_depth", + 200, + 1, + Integer.MAX_VALUE, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); + public static final Setting CLUSTER_ALLOW_DERIVED_FIELD_SETTING = Setting.boolSetting( "search.derived_field.enabled", true, @@ -483,6 +492,10 @@ public SearchService( clusterService.getClusterSettings() .addSettingsUpdateConsumer(SEARCH_MAX_QUERY_STRING_LENGTH, QueryStringQueryParser::setMaxQueryStringLength); + QueryStringQueryParser.setMaxQueryNestingDepth(SEARCH_MAX_QUERY_NESTING_DEPTH.get(settings)); + clusterService.getClusterSettings() + .addSettingsUpdateConsumer(SEARCH_MAX_QUERY_NESTING_DEPTH, QueryStringQueryParser::setMaxQueryNestingDepth); + allowDerivedField = CLUSTER_ALLOW_DERIVED_FIELD_SETTING.get(settings); clusterService.getClusterSettings().addSettingsUpdateConsumer(CLUSTER_ALLOW_DERIVED_FIELD_SETTING, this::setAllowDerivedField); diff --git a/server/src/test/java/org/opensearch/index/search/QueryStringNestedParenthesesOverflowTests.java b/server/src/test/java/org/opensearch/index/search/QueryStringNestedParenthesesOverflowTests.java new file mode 100644 index 0000000000000..9014293feb95a --- /dev/null +++ b/server/src/test/java/org/opensearch/index/search/QueryStringNestedParenthesesOverflowTests.java @@ -0,0 +1,133 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.search; + +import org.apache.lucene.queryparser.classic.ParseException; +import org.apache.lucene.search.Query; +import org.opensearch.index.query.QueryShardContext; +import org.opensearch.test.OpenSearchSingleNodeTestCase; + +import static org.hamcrest.Matchers.containsString; + +/** + * Tests for the query_string nesting depth limit that guards against + * StackOverflowError from deeply nested parentheses in Lucene's + * recursive-descent classic query parser. + */ +public class QueryStringNestedParenthesesOverflowTests extends OpenSearchSingleNodeTestCase { + + /** + * Verifies that deeply nested parentheses (15000 levels) are rejected + * with a ParseException. + */ + public void testDeeplyNestedParenthesesShouldBeRejected() throws Exception { + QueryShardContext context = createIndex("test-index").newQueryShardContext(0, null, () -> 0L, null); + QueryStringQueryParser parser = new QueryStringQueryParser(context, "field"); + + // 15000 opening parens + "field:value" + 15000 closing parens = 30011 chars (under 32000 limit) + int nestingDepth = 15000; + String maliciousQuery = buildNestedQuery(nestingDepth, "field:value"); + + // Verify payload is under the default max_query_string_length limit + assertTrue( + "Payload must be under 32000 chars (default max_query_string_length), actual: " + maliciousQuery.length(), + maliciousQuery.length() < 32000 + ); + + // Must be rejected with ParseException, not StackOverflowError + ParseException exception = expectThrows(ParseException.class, () -> parser.parse(maliciousQuery)); + assertThat(exception.getMessage(), containsString("nesting depth exceeds max allowed depth")); + assertThat(exception.getMessage(), containsString("search.query.max_query_nesting_depth")); + } + + /** + * Verifies that moderate nesting (50 levels) parses fine — no false positives. + */ + public void testModerateNestingIsAllowed() throws Exception { + QueryShardContext context = createIndex("test-moderate").newQueryShardContext(0, null, () -> 0L, null); + QueryStringQueryParser parser = new QueryStringQueryParser(context, "field"); + + String query = buildNestedQuery(50, "field:value"); + Query result = parser.parse(query); + assertNotNull("Moderate nesting (50 levels) should parse successfully", result); + } + + /** + * Verifies that nesting at exactly the default limit (200) is allowed. + */ + public void testNestingAtExactLimitIsAllowed() throws Exception { + QueryShardContext context = createIndex("test-at-limit").newQueryShardContext(0, null, () -> 0L, null); + QueryStringQueryParser parser = new QueryStringQueryParser(context, "field"); + + String query = buildNestedQuery(200, "field:value"); + Query result = parser.parse(query); + assertNotNull("Nesting at exactly the limit (200) should parse successfully", result); + } + + /** + * Verifies that nesting one level above the default limit (201) is rejected. + */ + public void testNestingOneAboveLimitIsRejected() throws Exception { + QueryShardContext context = createIndex("test-above-limit").newQueryShardContext(0, null, () -> 0L, null); + QueryStringQueryParser parser = new QueryStringQueryParser(context, "field"); + + String query = buildNestedQuery(201, "field:value"); + ParseException exception = expectThrows(ParseException.class, () -> parser.parse(query)); + assertThat(exception.getMessage(), containsString("nesting depth exceeds max allowed depth")); + } + + /** + * Verifies that parentheses inside quoted strings are NOT counted toward nesting depth. + */ + public void testParenthesesInQuotesAreIgnored() throws Exception { + QueryShardContext context = createIndex("test-quotes").newQueryShardContext(0, null, () -> 0L, null); + QueryStringQueryParser parser = new QueryStringQueryParser(context, "field"); + + // Lots of parentheses, but all inside quotes — should not trigger depth limit + StringBuilder sb = new StringBuilder(); + sb.append("field:\""); + for (int i = 0; i < 500; i++) { + sb.append("("); + } + sb.append("value"); + for (int i = 0; i < 500; i++) { + sb.append(")"); + } + sb.append("\""); + + // Should parse without error since parens are inside quotes + Query result = parser.parse(sb.toString()); + assertNotNull("Parentheses inside quotes should not count toward nesting depth", result); + } + + /** + * Verifies that the helper method correctly computes nesting depth. + */ + public void testMaxParenthesisNestingDepthCalculation() { + assertEquals(0, QueryStringQueryParser.maxParenthesisNestingDepth("field:value")); + assertEquals(1, QueryStringQueryParser.maxParenthesisNestingDepth("(field:value)")); + assertEquals(3, QueryStringQueryParser.maxParenthesisNestingDepth("(((field:value)))")); + assertEquals(2, QueryStringQueryParser.maxParenthesisNestingDepth("(a OR (b AND c))")); + // Parentheses in quotes don't count + assertEquals(0, QueryStringQueryParser.maxParenthesisNestingDepth("field:\"(((nested)))\"")); + assertEquals(1, QueryStringQueryParser.maxParenthesisNestingDepth("(field:\"(((inner)))\")")); + } + + private String buildNestedQuery(int nestingDepth, String innerTerm) { + StringBuilder sb = new StringBuilder(nestingDepth * 2 + innerTerm.length()); + for (int i = 0; i < nestingDepth; i++) { + sb.append('('); + } + sb.append(innerTerm); + for (int i = 0; i < nestingDepth; i++) { + sb.append(')'); + } + return sb.toString(); + } +}