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

Author: imotov

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])

server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java

@@ -171,11 +171,17 @@ public static final String stringEncodeFromMortonLong(long hashedVal, final int
      * 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

server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java

@@ -28,7 +28,6 @@
 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 GeoPoint resetFromIndexableField(IndexableField field) {
     }
 
     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));
     }
 

server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java

@@ -299,14 +299,7 @@ public Mapper parse(ParseContext context) throws IOException {
                 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,42 +319,57 @@ public Mapper parse(ParseContext context) throws IOException {
                     } 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);
             }
         }
 
         context.path().remove();
         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);

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 void testLongGeohashes() {
             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());
+    }
+
 }

server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java

@@ -49,6 +49,7 @@
 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 void testNullValue() throws Exception {
         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]"));
+    }
+
 }

server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java

@@ -175,6 +175,19 @@ public void testInvalidField() throws IOException {
         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();