From 2518fefa372e1f96e4721af8d9a02ac08b299fc8 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Mon, 7 May 2018 13:56:39 -0400 Subject: [PATCH] Add stricter geohash parsing (#30376) Adds verification that geohashes are not empty and contain only valid characters. It fixes the issue when en empty geohash is treated as [-180, -90] and geohashes with non-geohash character are getting resolved into invalid coordinates. Closes #23579 --- docs/CHANGELOG.asciidoc | 15 +++++ .../common/geo/GeoHashUtils.java | 6 ++ .../elasticsearch/common/geo/GeoPoint.java | 8 ++- .../index/mapper/GeoPointFieldMapper.java | 58 +++++++++++-------- .../common/geo/GeoHashTests.java | 13 ++++- .../mapper/GeoPointFieldMapperTests.java | 47 +++++++++++++++ .../search/geo/GeoPointParsingTests.java | 13 +++++ 7 files changed, 132 insertions(+), 28 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index f9cb572eb81..3e7f2337435 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -177,6 +177,21 @@ Machine Learning:: * Account for gaps in data counts after job is reopened ({pull}30294[#30294]) +Add validation that geohashes are not empty and don't contain unsupported characters ({pull}30376[#30376]) + +[[release-notes-6.3.1]] +== Elasticsearch version 6.3.1 + +//[float] +//=== New Features + +//[float] +//=== Enhancements + +[float] +=== Bug Fixes + +Reduce the number of object allocations made by {security} when resolving the indices and aliases for a request ({pull}30180[#30180]) Rollup:: * Validate timezone in range queries to ensure they match the selected job when searching ({pull}30338[#30338]) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java index d2ca936740e..0ee8d095f49 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java @@ -171,11 +171,17 @@ public class GeoHashUtils { * Encode to a morton long value from a given geohash string */ public static final long mortonEncode(final String hash) { + if (hash.isEmpty()) { + throw new IllegalArgumentException("empty geohash"); + } int level = 11; long b; long l = 0L; for(char c : hash.toCharArray()) { b = (long)(BASE_32_STRING.indexOf(c)); + if (b < 0) { + throw new IllegalArgumentException("unsupported symbol [" + c + "] in geohash [" + hash + "]"); + } l |= (b<<((level--*5) + MORTON_OFFSET)); if (level < 0) { // We cannot handle more than 12 levels diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index e43c9e9a8e3..8a0c3efa5af 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -28,7 +28,6 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.ElasticsearchParseException; -import org.elasticsearch.common.Strings; import java.io.IOException; import java.util.Arrays; @@ -126,7 +125,12 @@ public final class GeoPoint implements ToXContentFragment { } public GeoPoint resetFromGeoHash(String geohash) { - final long hash = mortonEncode(geohash); + final long hash; + try { + hash = mortonEncode(geohash); + } catch (IllegalArgumentException ex) { + throw new ElasticsearchParseException(ex.getMessage(), ex); + } return this.reset(GeoHashUtils.decodeLatitude(hash), GeoHashUtils.decodeLongitude(hash)); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index 2ea31f67e29..551f7c18c1c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -299,14 +299,7 @@ public class GeoPointFieldMapper extends FieldMapper implements ArrayValueMapper if (token == XContentParser.Token.START_ARRAY) { // its an array of array of lon/lat [ [1.2, 1.3], [1.4, 1.5] ] while (token != XContentParser.Token.END_ARRAY) { - try { - parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse)); - } catch (ElasticsearchParseException e) { - if (ignoreMalformed.value() == false) { - throw e; - } - context.addIgnoredField(fieldType.name()); - } + parseGeoPointIgnoringMalformed(context, sparse); token = context.parser().nextToken(); } } else { @@ -326,35 +319,22 @@ public class GeoPointFieldMapper extends FieldMapper implements ArrayValueMapper } else { while (token != XContentParser.Token.END_ARRAY) { if (token == XContentParser.Token.VALUE_STRING) { - parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value())); + parseGeoPointStringIgnoringMalformed(context, sparse); } else { - try { - parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse)); - } catch (ElasticsearchParseException e) { - if (ignoreMalformed.value() == false) { - throw e; - } - } + parseGeoPointIgnoringMalformed(context, sparse); } token = context.parser().nextToken(); } } } } else if (token == XContentParser.Token.VALUE_STRING) { - parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value())); + parseGeoPointStringIgnoringMalformed(context, sparse); } else if (token == XContentParser.Token.VALUE_NULL) { if (fieldType.nullValue() != null) { parse(context, (GeoPoint) fieldType.nullValue()); } } else { - try { - parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse)); - } catch (ElasticsearchParseException e) { - if (ignoreMalformed.value() == false) { - throw e; - } - context.addIgnoredField(fieldType.name()); - } + parseGeoPointIgnoringMalformed(context, sparse); } } @@ -362,6 +342,34 @@ public class GeoPointFieldMapper extends FieldMapper implements ArrayValueMapper return null; } + /** + * Parses geopoint represented as an object or an array, ignores malformed geopoints if needed + */ + private void parseGeoPointIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException { + try { + parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse)); + } catch (ElasticsearchParseException e) { + if (ignoreMalformed.value() == false) { + throw e; + } + context.addIgnoredField(fieldType.name()); + } + } + + /** + * Parses geopoint represented as a string and ignores malformed geopoints if needed + */ + private void parseGeoPointStringIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException { + try { + parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value())); + } catch (ElasticsearchParseException e) { + if (ignoreMalformed.value() == false) { + throw e; + } + context.addIgnoredField(fieldType.name()); + } + } + @Override protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException { super.doXContentBody(builder, includeDefaults, params); diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoHashTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoHashTests.java index 2726380b7e3..1ab67b058f1 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoHashTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoHashTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.geo; import org.apache.lucene.geo.Rectangle; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.test.ESTestCase; /** @@ -95,7 +96,17 @@ public class GeoHashTests extends ESTestCase { Rectangle expectedBbox = GeoHashUtils.bbox(geohash); Rectangle actualBbox = GeoHashUtils.bbox(extendedGeohash); assertEquals("Additional data points above 12 should be ignored [" + extendedGeohash + "]" , expectedBbox, actualBbox); - } } + + public void testInvalidGeohashes() { + IllegalArgumentException ex; + + ex = expectThrows(IllegalArgumentException.class, () -> GeoHashUtils.mortonEncode("55.5")); + assertEquals("unsupported symbol [.] in geohash [55.5]", ex.getMessage()); + + ex = expectThrows(IllegalArgumentException.class, () -> GeoHashUtils.mortonEncode("")); + assertEquals("empty geohash", ex.getMessage()); + } + } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java index 0de90631a14..facafaf180e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java @@ -49,6 +49,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; public class GeoPointFieldMapperTests extends ESSingleNodeTestCase { @@ -398,4 +399,50 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase { assertThat(defaultValue, not(equalTo(doc.rootDoc().getField("location").binaryValue()))); } + public void testInvalidGeohashIgnored() throws Exception { + String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties") + .startObject("location") + .field("type", "geo_point") + .field("ignore_malformed", "true") + .endObject() + .endObject().endObject().endObject()); + + DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser() + .parse("type", new CompressedXContent(mapping)); + + ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type", "1", BytesReference + .bytes(XContentFactory.jsonBuilder() + .startObject() + .field("location", "1234.333") + .endObject()), + XContentType.JSON)); + + assertThat(doc.rootDoc().getField("location"), nullValue()); + } + + + public void testInvalidGeohashNotIgnored() throws Exception { + String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties") + .startObject("location") + .field("type", "geo_point") + .endObject() + .endObject().endObject().endObject()); + + DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser() + .parse("type", new CompressedXContent(mapping)); + + MapperParsingException ex = expectThrows(MapperParsingException.class, + () -> defaultMapper.parse(SourceToParse.source("test", "type", "1", BytesReference + .bytes(XContentFactory.jsonBuilder() + .startObject() + .field("location", "1234.333") + .endObject()), + XContentType.JSON))); + + assertThat(ex.getMessage(), equalTo("failed to parse")); + assertThat(ex.getRootCause().getMessage(), equalTo("unsupported symbol [.] in geohash [1234.333]")); + } + } diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java index f3d109868ef..4b580aa6a24 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java @@ -175,6 +175,19 @@ public class GeoPointParsingTests extends ESTestCase { assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]")); } + public void testInvalidGeoHash() throws IOException { + XContentBuilder content = JsonXContent.contentBuilder(); + content.startObject(); + content.field("geohash", "!!!!"); + content.endObject(); + + XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content)); + parser.nextToken(); + + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("unsupported symbol [!] in geohash [!!!!]")); + } + private XContentParser objectLatLon(double lat, double lon) throws IOException { XContentBuilder content = JsonXContent.contentBuilder(); content.startObject();