From 0fa58d23a2eb67a4851f10740acb19be31f0ed5b Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 23 Jan 2018 16:57:26 +0100 Subject: [PATCH] Ensure we protect Collections obtained from scripts from self-referencing (#28335) Self referencing maps can cause SOE if they are iterated ie. in their toString methods. This chance adds some protected to the usage of those collections. --- .../common/util/CollectionUtils.java | 38 +++++++++++++++++++ .../common/xcontent/XContentBuilder.java | 37 ++---------------- .../scripted/ScriptedMetricAggregator.java | 2 + .../BucketScriptPipelineAggregator.java | 3 +- .../aggregations/support/ValuesSource.java | 5 ++- .../support/values/ScriptBytesValues.java | 2 + .../subphase/ScriptFieldsFetchSubPhase.java | 2 + .../search/sort/ScriptSortBuilder.java | 5 ++- .../common/util/CollectionUtilsTests.java | 16 ++++++++ .../common/xcontent/BaseXContentTestCase.java | 23 +++++------ .../CustomReflectionObjectHandler.java | 6 +++ .../test/painless/15_update.yaml | 2 +- .../test/painless/30_search.yaml | 36 ++++++++++++++++++ 13 files changed, 129 insertions(+), 48 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java b/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java index 54a49f7e4f254..08d02cdea3172 100644 --- a/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java +++ b/core/src/main/java/org/elasticsearch/common/util/CollectionUtils.java @@ -19,16 +19,20 @@ package org.elasticsearch.common.util; +import java.nio.file.Path; import java.util.AbstractList; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.IdentityHashMap; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.RandomAccess; +import java.util.Set; import com.carrotsearch.hppc.ObjectArrayList; import org.apache.lucene.util.BytesRef; @@ -221,6 +225,40 @@ public static int[] toArray(Collection ints) { return ints.stream().mapToInt(s -> s).toArray(); } + public static void ensureNoSelfReferences(Object value) { + Iterable it = convert(value); + if (it != null) { + ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>())); + } + } + + private static Iterable convert(Object value) { + if (value == null) { + return null; + } + if (value instanceof Map) { + return ((Map) value).values(); + } else if ((value instanceof Iterable) && (value instanceof Path == false)) { + return (Iterable) value; + } else if (value instanceof Object[]) { + return Arrays.asList((Object[]) value); + } else { + return null; + } + } + + private static void ensureNoSelfReferences(final Iterable value, Object originalReference, final Set ancestors) { + if (value != null) { + if (ancestors.add(originalReference) == false) { + throw new IllegalArgumentException("Iterable object is self-referencing itself"); + } + for (Object o : value) { + ensureNoSelfReferences(convert(o), o, ancestors); + } + ancestors.remove(originalReference); + } + } + private static class RotatedList extends AbstractList implements RandomAccess { private final List in; diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index f0427ce246669..dcd50a4082b2c 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.text.Text; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.CollectionUtils; import org.joda.time.DateTimeZone; import org.joda.time.ReadableInstant; import org.joda.time.format.DateTimeFormatter; @@ -38,12 +39,10 @@ import java.io.InputStream; import java.io.OutputStream; import java.nio.file.Path; -import java.util.Arrays; import java.util.Calendar; import java.util.Collections; import java.util.Date; import java.util.HashMap; -import java.util.IdentityHashMap; import java.util.Locale; import java.util.Map; import java.util.Objects; @@ -783,7 +782,7 @@ XContentBuilder values(Object[] values) throws IOException { // checks that the array of object does not contain references to itself because // iterating over entries will cause a stackoverflow error - ensureNoSelfReferences(values); + CollectionUtils.ensureNoSelfReferences(values); startArray(); for (Object o : values) { @@ -869,7 +868,7 @@ public XContentBuilder map(Map values) throws IOException { // checks that the map does not contain references to itself because // iterating over map entries will cause a stackoverflow error - ensureNoSelfReferences(values); + CollectionUtils.ensureNoSelfReferences(values); startObject(); for (Map.Entry value : values.entrySet()) { @@ -895,7 +894,7 @@ private XContentBuilder value(Iterable values) throws IOException { } else { // checks that the iterable does not contain references to itself because // iterating over entries will cause a stackoverflow error - ensureNoSelfReferences(values); + CollectionUtils.ensureNoSelfReferences(values); startArray(); for (Object value : values) { @@ -1066,32 +1065,4 @@ static void ensureNotNull(Object value, String message) { throw new IllegalArgumentException(message); } } - - static void ensureNoSelfReferences(Object value) { - ensureNoSelfReferences(value, Collections.newSetFromMap(new IdentityHashMap<>())); - } - - private static void ensureNoSelfReferences(final Object value, final Set ancestors) { - if (value != null) { - - Iterable it; - if (value instanceof Map) { - it = ((Map) value).values(); - } else if ((value instanceof Iterable) && (value instanceof Path == false)) { - it = (Iterable) value; - } else if (value instanceof Object[]) { - it = Arrays.asList((Object[]) value); - } else { - return; - } - - if (ancestors.add(value) == false) { - throw new IllegalArgumentException("Object has already been built and is self-referencing itself"); - } - for (Object o : it) { - ensureNoSelfReferences(o, ancestors); - } - ancestors.remove(value); - } - } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java index cee7b3402f3e4..c27349fdb2cec 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregator.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.metrics.scripted; import org.apache.lucene.index.LeafReaderContext; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.script.Script; @@ -78,6 +79,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) { Object aggregation; if (combineScript != null) { aggregation = combineScript.run(); + CollectionUtils.ensureNoSelfReferences(aggregation); } else { aggregation = params.get("_agg"); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java index 87df926ebab55..274db1967dd00 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java @@ -114,10 +114,11 @@ public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext } else { ExecutableScript executableScript = reduceContext.scriptService().executable(compiledScript, vars); Object returned = executableScript.run(); + // no need to check for self references since only numbers are valid if (returned == null) { newBuckets.add(bucket); } else { - if (!(returned instanceof Number)) { + if ((returned instanceof Number) == false) { throw new AggregationExecutionException("series_arithmetic script for reducer [" + name() + "] must return a Number"); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java index ca3424ae7042b..3fcd339c455ca 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java @@ -29,6 +29,7 @@ import org.apache.lucene.util.Bits; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.lucene.ScorerAware; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.index.fielddata.AtomicOrdinalsFieldData; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; @@ -437,7 +438,9 @@ public void setDocument(int docId) { for (int i = 0; i < count; ++i) { final BytesRef value = bytesValues.valueAt(i); script.setNextAggregationValue(value.utf8ToString()); - values[i].copyChars(script.run().toString()); + Object run = script.run(); + CollectionUtils.ensureNoSelfReferences(run); + values[i].copyChars(run.toString()); } sort(); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java index ce36fec941671..0ebf834fdd9dc 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/support/values/ScriptBytesValues.java @@ -20,6 +20,7 @@ import org.apache.lucene.search.Scorer; import org.elasticsearch.common.lucene.ScorerAware; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortingBinaryDocValues; import org.elasticsearch.script.LeafSearchScript; @@ -43,6 +44,7 @@ private void set(int i, Object o) { if (o == null) { values[i].clear(); } else { + CollectionUtils.ensureNoSelfReferences(o); values[i].copyChars(o.toString()); } } diff --git a/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java b/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java index d5f587db29dab..572a9141cfaf7 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsFetchSubPhase.java @@ -20,6 +20,7 @@ import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.search.SearchHitField; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.search.fetch.FetchSubPhase; import org.elasticsearch.search.internal.SearchContext; @@ -49,6 +50,7 @@ public void hitExecute(SearchContext context, HitContext hitContext) { final Object value; try { value = leafScript.unwrap(leafScript.run()); + CollectionUtils.ensureNoSelfReferences(value); } catch (RuntimeException e) { if (scriptField.ignoreException()) { continue; diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index 0527217bc14de..d3f4088f7d72b 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -266,7 +267,9 @@ protected SortedBinaryDocValues getValues(LeafReaderContext context) throws IOEx @Override public BytesRef get(int docID) { leafScript.setDocument(docID); - spare.copyChars(leafScript.run().toString()); + final Object run = leafScript.run(); + CollectionUtils.ensureNoSelfReferences(run); + spare.copyChars(run.toString()); return spare.get(); } }; diff --git a/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java b/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java index 8c192a2a35091..2ca8189a972fd 100644 --- a/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java +++ b/core/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java @@ -25,16 +25,21 @@ import org.apache.lucene.util.Counter; import org.elasticsearch.test.ESTestCase; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.SortedSet; import java.util.TreeSet; +import static java.util.Collections.emptyMap; import static org.elasticsearch.common.util.CollectionUtils.eagerPartition; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -176,4 +181,15 @@ public void testPerfectPartition() { eagerPartition(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12), 6) ); } + + public void testEnsureNoSelfReferences() { + CollectionUtils.ensureNoSelfReferences(emptyMap()); + CollectionUtils.ensureNoSelfReferences(null); + + Map map = new HashMap<>(); + map.put("field", map); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> CollectionUtils.ensureNoSelfReferences(map)); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); + } } diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java index eb0adc1990c8f..d755b8b82ac03 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java @@ -34,6 +34,7 @@ import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.text.Text; import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.xcontent.XContentParser.Token; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matcher; @@ -853,19 +854,19 @@ public void testEnsureNotNull() { } public void testEnsureNoSelfReferences() throws IOException { - XContentBuilder.ensureNoSelfReferences(emptyMap()); - XContentBuilder.ensureNoSelfReferences(null); + CollectionUtils.ensureNoSelfReferences(emptyMap()); + CollectionUtils.ensureNoSelfReferences(null); Map map = new HashMap<>(); map.put("field", map); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map)); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } /** * Test that the same map written multiple times do not trigger the self-reference check in - * {@link XContentBuilder#ensureNoSelfReferences(Object)} + * {@link CollectionUtils#ensureNoSelfReferences(Object)} */ public void testRepeatedMapsAndNoSelfReferences() throws Exception { Map mapB = singletonMap("b", "B"); @@ -898,7 +899,7 @@ public void testSelfReferencingMapsOneLevel() throws IOException { map1.put("map0", map0); // map 1 -> map 0 loop IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0)); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testSelfReferencingMapsTwoLevels() throws IOException { @@ -916,7 +917,7 @@ public void testSelfReferencingMapsTwoLevels() throws IOException { map2.put("map0", map0); // map 2 -> map 0 loop IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0)); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testSelfReferencingObjectsArray() throws IOException { @@ -929,13 +930,13 @@ public void testSelfReferencingObjectsArray() throws IOException { .startObject() .field("field", values) .endObject()); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); e = expectThrows(IllegalArgumentException.class, () -> builder() .startObject() .array("field", values) .endObject()); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testSelfReferencingIterable() throws IOException { @@ -948,7 +949,7 @@ public void testSelfReferencingIterable() throws IOException { .startObject() .field("field", (Iterable) values) .endObject()); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testSelfReferencingIterableOneLevel() throws IOException { @@ -963,7 +964,7 @@ public void testSelfReferencingIterableOneLevel() throws IOException { .startObject() .field("field", (Iterable) values) .endObject()); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testSelfReferencingIterableTwoLevels() throws IOException { @@ -983,7 +984,7 @@ public void testSelfReferencingIterableTwoLevels() throws IOException { map2.put("map0", map0); // map 2 -> map 0 loop IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> builder().map(map0)); - assertThat(e.getMessage(), containsString("Object has already been built and is self-referencing itself")); + assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself")); } public void testNamedObject() throws IOException { diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java index eef9d7af8dd01..79319369489fd 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java @@ -20,6 +20,7 @@ package org.elasticsearch.script.mustache; import com.github.mustachejava.reflect.ReflectionObjectHandler; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.iterable.Iterables; import java.lang.reflect.Array; @@ -154,4 +155,9 @@ public Iterator iterator() { } } + @Override + public String stringify(Object object) { + CollectionUtils.ensureNoSelfReferences(object); + return super.stringify(object); + } } diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml index a64ad904c4963..5e68bec648da2 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yaml @@ -137,4 +137,4 @@ - match: { error.root_cause.0.type: "remote_transport_exception" } - match: { error.type: "illegal_argument_exception" } - - match: { error.reason: "Object has already been built and is self-referencing itself" } + - match: { error.reason: "Iterable object is self-referencing itself" } diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yaml index 28679cb223fd1..af21827287fb8 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yaml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/30_search.yaml @@ -406,3 +406,39 @@ - match: { hits.hits.0._score: 1.0 } - match: { aggregations.value_agg.buckets.0.key: 2 } - match: { aggregations.value_agg.buckets.0.doc_count: 1 } + +--- +"Return self-referencing map": + - do: + indices.create: + index: test + body: + settings: + number_of_shards: "1" + + - do: + index: + index: test + type: test + id: 1 + body: { "genre": 1 } + + - do: + indices.refresh: {} + + - do: + catch: request + index: test + search: + body: + aggs: + genre: + terms: + script: + lang: painless + source: "def x = [:] ; def y = [:] ; x.a = y ; y.a = x ; return x" + + - match: { error.root_cause.0.type: "illegal_argument_exception" } + - match: { error.root_cause.0.reason: "Iterable object is self-referencing itself" } + - match: { error.type: "search_phase_execution_exception" } + - match: { error.reason: "all shards failed" }