Merge pull request #14874 from jasontedor/ipv4-parsing

Do not be lenient when parsing CIDRs

core/src/main/java/org/elasticsearch/common/network/Cidrs.java

@@ -0,0 +1,116 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.common.network;
+
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.Objects;
+
+public final class Cidrs {
+    private Cidrs() {
+    }
+
+    /**
+     * Parses an IPv4 address block in CIDR notation into a pair of
+     * longs representing the bottom and top of the address block
+     *
+     * @param cidr an address block in CIDR notation a.b.c.d/n
+     * @return array representing the address block
+     * @throws IllegalArgumentException if the cidr can not be parsed
+     */
+    public static long[] cidrMaskToMinMax(String cidr) {
+        Objects.requireNonNull(cidr, "cidr");
+        String[] fields = cidr.split("/");
+        if (fields.length != 2) {
+            throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "invalid IPv4/CIDR; expected [a.b.c.d, e] but was [%s] after splitting on \"/\" in [%s]", Arrays.toString(fields), cidr)
+            );
+        }
+        // do not try to parse IPv4-mapped IPv6 address
+        if (fields[0].contains(":")) {
+            throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "invalid IPv4/CIDR; expected [a.b.c.d, e] where a, b, c, d are decimal octets but was [%s] after splitting on \"/\" in [%s]", Arrays.toString(fields), cidr)
+            );
+        }
+        byte[] addressBytes;
+        try {
+            addressBytes = InetAddresses.forString(fields[0]).getAddress();
+        } catch (Throwable t) {
+            throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "invalid IPv4/CIDR; unable to parse [%s] as an IP address literal", fields[0]), t
+            );
+        }
+        long accumulator =
+                ((addressBytes[0] & 0xFFL) << 24) +
+                        ((addressBytes[1] & 0xFFL) << 16) +
+                        ((addressBytes[2] & 0xFFL) << 8) +
+                        ((addressBytes[3] & 0xFFL));
+        int networkMask;
+        try {
+            networkMask = Integer.parseInt(fields[1]);
+        } catch (NumberFormatException e) {
+            throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "invalid IPv4/CIDR; invalid network mask [%s] in [%s]", fields[1], cidr),
+                    e
+            );
+        }
+        if (networkMask < 0 || networkMask > 32) {
+            throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "invalid IPv4/CIDR; invalid network mask [%s], out of range in [%s]", fields[1], cidr)
+            );
+        }
+
+        long blockSize = 1L << (32 - networkMask);
+        // validation
+        if ((accumulator & (blockSize - 1)) != 0) {
+            throw new IllegalArgumentException(
+                    String.format(
+                            Locale.ROOT,
+                            "invalid IPv4/CIDR; invalid address/network mask combination in [%s]; perhaps [%s] was intended?",
+                            cidr,
+                            octetsToCIDR(longToOctets(accumulator - (accumulator & (blockSize - 1))), networkMask)
+                    )
+            );
+        }
+        return new long[] { accumulator, accumulator + blockSize };
+    }
+
+    static int[] longToOctets(long value) {
+        assert value >= 0 && value <= (1L << 32) : value;
+        int[] octets = new int[4];
+        octets[0] = (int)((value >> 24) & 0xFF);
+        octets[1] = (int)((value >> 16) & 0xFF);
+        octets[2] = (int)((value >> 8) & 0xFF);
+        octets[3] = (int)(value & 0xFF);
+        return octets;
+    }
+
+    static String octetsToString(int[] octets) {
+        assert octets != null;
+        assert octets.length == 4;
+        return String.format(Locale.ROOT, "%d.%d.%d.%d", octets[0], octets[1], octets[2], octets[3]);
+    }
+
+    static String octetsToCIDR(int[] octets, int networkMask) {
+        assert octets != null;
+        assert octets.length == 4;
+        return octetsToString(octets) + "/" + networkMask;
+    }
+}

core/src/main/java/org/elasticsearch/index/mapper/ip/IpFieldMapper.java

@@ -31,6 +31,7 @@ import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Numbers;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.network.Cidrs;
 import org.elasticsearch.common.network.InetAddresses;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.Fuzziness;
@@ -48,6 +49,7 @@ import org.elasticsearch.index.mapper.core.LongFieldMapper;
 import org.elasticsearch.index.mapper.core.LongFieldMapper.CustomLongNumericField;
 import org.elasticsearch.index.mapper.core.NumberFieldMapper;
 import org.elasticsearch.index.query.QueryShardContext;
+import org.elasticsearch.search.aggregations.bucket.range.ipv4.InternalIPv4Range;
 
 import java.io.IOException;
 import java.util.Iterator;
@@ -76,7 +78,6 @@ public class IpFieldMapper extends NumberFieldMapper {
     }
 
     private static final Pattern pattern = Pattern.compile("\\.");
-    private static final Pattern MASK_PATTERN = Pattern.compile("(\\d{1,3})\\.(\\d{1,3})\\.(\\d{1,3})\\.(\\d{1,3})/(\\d{1,3})");
 
     public static long ipToLong(String ip) {
         try {
@@ -97,64 +98,6 @@ public class IpFieldMapper extends NumberFieldMapper {
         }
     }
 
-    /**
-     * Computes the min & max ip addresses (represented as long values -
-     * same way as stored in index) represented by the given CIDR mask
-     * expression. The returned array has the length of 2, where the first entry
-     * represents the {@code min} address and the second the {@code max}. A
-     * {@code -1} value for either the {@code min} or the {@code max},
-     * represents an unbounded end. In other words:
-     *
-     * <p>
-     * {@code min == -1 == "0.0.0.0" }
-     * </p>
-     *
-     * and
-     *
-     * <p>
-     * {@code max == -1 == "255.255.255.255" }
-     * </p>
-     */
-    public static long[] cidrMaskToMinMax(String cidr) {
-        Matcher matcher = MASK_PATTERN.matcher(cidr);
-        if (!matcher.matches()) {
-            return null;
-        }
-        int addr = ((Integer.parseInt(matcher.group(1)) << 24) & 0xFF000000) | ((Integer.parseInt(matcher.group(2)) << 16) & 0xFF0000)
-                | ((Integer.parseInt(matcher.group(3)) << 8) & 0xFF00) | (Integer.parseInt(matcher.group(4)) & 0xFF);
-
-        int mask = (-1) << (32 - Integer.parseInt(matcher.group(5)));
-
-        if (Integer.parseInt(matcher.group(5)) == 0) {
-            mask = 0 << 32;
-        }
-
-        int from = addr & mask;
-        long longFrom = intIpToLongIp(from);
-        if (longFrom == 0) {
-            longFrom = -1;
-        }
-
-        int to = from + (~mask);
-        long longTo = intIpToLongIp(to) + 1; // we have to +1 here as the range
-                                             // is non-inclusive on the "to"
-                                             // side
-
-        if (longTo == MAX_IP) {
-            longTo = -1;
-        }
-
-        return new long[] { longFrom, longTo };
-    }
-
-    private static long intIpToLongIp(int i) {
-        long p1 = ((long) ((i >> 24) & 0xFF)) << 24;
-        int p2 = ((i >> 16) & 0xFF) << 16;
-        int p3 = ((i >> 8) & 0xFF) << 8;
-        int p4 = i & 0xFF;
-        return p1 + p2 + p3 + p4;
-    }
-
     public static class Defaults extends NumberFieldMapper.Defaults {
         public static final String NULL_VALUE = null;
 
@@ -274,13 +217,13 @@ public class IpFieldMapper extends NumberFieldMapper {
             if (value != null) {
                 long[] fromTo;
                 if (value instanceof BytesRef) {
-                    fromTo = cidrMaskToMinMax(((BytesRef) value).utf8ToString());
+                    fromTo = Cidrs.cidrMaskToMinMax(((BytesRef) value).utf8ToString());
                 } else {
-                    fromTo = cidrMaskToMinMax(value.toString());
+                    fromTo = Cidrs.cidrMaskToMinMax(value.toString());
                 }
                 if (fromTo != null) {
-                    return rangeQuery(fromTo[0] < 0 ? null : fromTo[0],
-                            fromTo[1] < 0 ? null : fromTo[1], true, false);
+                    return rangeQuery(fromTo[0] == 0 ? null : fromTo[0],
+                            fromTo[1] == InternalIPv4Range.MAX_IP ? null : fromTo[1], true, false);
                 }
             }
             return super.termQuery(value, context);

core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ipv4/IPv4RangeBuilder.java

@@ -19,11 +19,10 @@
 
 package org.elasticsearch.search.aggregations.bucket.range.ipv4;
 
+import org.elasticsearch.common.network.Cidrs;
 import org.elasticsearch.search.aggregations.bucket.range.AbstractRangeBuilder;
 import org.elasticsearch.search.builder.SearchSourceBuilderException;
 
-import static org.elasticsearch.index.mapper.ip.IpFieldMapper.cidrMaskToMinMax;
-
 /**
  * Builder for the {@code IPv4Range} aggregation.
  */
@@ -59,11 +58,13 @@ public class IPv4RangeBuilder extends AbstractRangeBuilder<IPv4RangeBuilder> {
      * Add a range based on a CIDR mask.
      */
     public IPv4RangeBuilder addMaskRange(String key, String mask) {
-        long[] fromTo = cidrMaskToMinMax(mask);
-        if (fromTo == null) {
-            throw new SearchSourceBuilderException("invalid CIDR mask [" + mask + "] in ip_range aggregation [" + getName() + "]");
+        long[] fromTo;
+        try {
+            fromTo = Cidrs.cidrMaskToMinMax(mask);
+        } catch (IllegalArgumentException e) {
+            throw new SearchSourceBuilderException("invalid CIDR mask [" + mask + "] in ip_range aggregation [" + getName() + "]", e);
         }
-        ranges.add(new Range(key, fromTo[0] < 0 ? null : fromTo[0], fromTo[1] < 0 ? null : fromTo[1]));
+        ranges.add(new Range(key, fromTo[0] == 0 ? null : fromTo[0], fromTo[1] == InternalIPv4Range.MAX_IP ? null : fromTo[1]));
         return this;
     }
 
@@ -106,5 +107,4 @@ public class IPv4RangeBuilder extends AbstractRangeBuilder<IPv4RangeBuilder> {
     public IPv4RangeBuilder addUnboundedFrom(String from) {
         return addUnboundedFrom(null, from);
     }
-
 }

core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ipv4/InternalIPv4Range.java

@@ -38,6 +38,7 @@ import static org.elasticsearch.index.mapper.ip.IpFieldMapper.MAX_IP;
  *
  */
 public class InternalIPv4Range extends InternalRange<InternalIPv4Range.Bucket, InternalIPv4Range> {
+    public static final long MAX_IP = 1L << 32;
 
     public final static Type TYPE = new Type("ip_range", "iprange");
 

core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ipv4/IpRangeParser.java

@@ -18,6 +18,7 @@
  */
 package org.elasticsearch.search.aggregations.bucket.range.ipv4;
 
+import org.elasticsearch.common.network.Cidrs;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.mapper.ip.IpFieldMapper;
 import org.elasticsearch.search.SearchParseException;
@@ -125,13 +126,15 @@ public class IpRangeParser implements Aggregator.Parser {
     }
 
     private static void parseMaskRange(String cidr, RangeAggregator.Range range, String aggregationName, SearchContext ctx) {
-        long[] fromTo = IpFieldMapper.cidrMaskToMinMax(cidr);
-        if (fromTo == null) {
+        long[] fromTo;
+        try {
+            fromTo = Cidrs.cidrMaskToMinMax(cidr);
+        } catch (IllegalArgumentException e) {
             throw new SearchParseException(ctx, "invalid CIDR mask [" + cidr + "] in aggregation [" + aggregationName + "]",
-                    null);
+                    null, e);
         }
-        range.from = fromTo[0] < 0 ? Double.NEGATIVE_INFINITY : fromTo[0];
-        range.to = fromTo[1] < 0 ? Double.POSITIVE_INFINITY : fromTo[1];
+        range.from = fromTo[0] == 0 ? Double.NEGATIVE_INFINITY : fromTo[0];
+        range.to = fromTo[1] == InternalIPv4Range.MAX_IP ? Double.POSITIVE_INFINITY : fromTo[1];
         if (range.key == null) {
             range.key = cidr;
         }

core/src/test/java/org/elasticsearch/common/network/CidrsTests.java

@@ -0,0 +1,192 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.common.network;
+
+import org.elasticsearch.common.collect.Tuple;
+import org.elasticsearch.common.network.Cidrs;
+import org.elasticsearch.search.aggregations.bucket.range.ipv4.IPv4RangeBuilder;
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.*;
+
+import static org.hamcrest.Matchers.*;
+
+public class CidrsTests extends ESTestCase {
+    public void testNullCidr() {
+        try {
+            Cidrs.cidrMaskToMinMax(null);
+            fail("expected NullPointerException");
+        } catch (NullPointerException e) {
+            assertThat(e, hasToString(containsString("cidr")));
+        }
+    }
+
+    public void testSplittingSlash() {
+        List<String> cases = new ArrayList<>();
+        cases.add("1.2.3.4");
+        cases.add("1.2.3.4/32/32");
+        cases.add("1.2.3.4/");
+        cases.add("/");
+        for (String test : cases) {
+            try {
+                Cidrs.cidrMaskToMinMax(test);
+                fail("expected IllegalArgumentException after splitting");
+            } catch (IllegalArgumentException e) {
+                assertThat(e, hasToString(containsString("expected [a.b.c.d, e]")));
+                assertThat(e, hasToString(containsString("splitting on \"/\"")));
+            }
+        }
+    }
+
+    public void testSplittingDot() {
+        List<String> cases = new ArrayList<>();
+        cases.add("1.2.3/32");
+        cases.add("1/32");
+        cases.add("1./32");
+        cases.add("1../32");
+        cases.add("1.../32");
+        cases.add("1.2.3.4.5/32");
+        cases.add("/32");
+        for (String test : cases) {
+            try {
+                Cidrs.cidrMaskToMinMax(test);
+                fail("expected IllegalArgumentException after splitting");
+            } catch (IllegalArgumentException e) {
+                assertThat(e, hasToString(containsString("unable to parse")));
+                assertThat(e, hasToString(containsString("as an IP address literal")));
+            }
+        }
+    }
+
+    public void testValidSpecificCases() {
+        List<Tuple<String, long[]>> cases = new ArrayList<>();
+        cases.add(new Tuple<>("192.168.0.0/24", new long[]{(192L << 24) + (168 << 16), (192L << 24) + (168 << 16) + (1 << 8)}));
+        cases.add(new Tuple<>("192.168.128.0/17", new long[]{(192L << 24) + (168 << 16) + (128 << 8), (192L << 24) + (168 << 16) + (128 << 8) + (1 << 15)}));
+        cases.add(new Tuple<>("128.0.0.0/1", new long[]{128L << 24, (128L << 24) + (1L << 31)})); // edge case
+        cases.add(new Tuple<>("0.0.0.0/0", new long[]{0, 1L << 32})); // edge case
+        cases.add(new Tuple<>("0.0.0.0/1", new long[]{0, 1L << 31})); // edge case
+        cases.add(new Tuple<>(
+                "192.168.1.1/32",
+                new long[]{(192L << 24) + (168L << 16) + (1L << 8) + 1L, (192L << 24) + (168L << 16) + (1L << 8) + 1L + 1})
+        ); // edge case
+        for (Tuple<String, long[]> test : cases) {
+            long[] actual = Cidrs.cidrMaskToMinMax(test.v1());
+            assertArrayEquals(test.v1(), test.v2(), actual);
+        }
+    }
+
+    public void testInvalidSpecificOctetCases() {
+        List<String> cases = new ArrayList<>();
+        cases.add("256.0.0.0/8"); // first octet out of range
+        cases.add("255.256.0.0/16"); // second octet out of range
+        cases.add("255.255.256.0/24"); // third octet out of range
+        cases.add("255.255.255.256/32"); // fourth octet out of range
+        cases.add("abc.0.0.0/8"); // octet that can not be parsed
+        cases.add("-1.0.0.0/8"); // first octet out of range
+        cases.add("128.-1.0.0/16"); // second octet out of range
+        cases.add("128.128.-1.0/24"); // third octet out of range
+        cases.add("128.128.128.-1/32"); // fourth octet out of range
+
+        for (String test : cases) {
+            try {
+                Cidrs.cidrMaskToMinMax(test);
+                fail("expected invalid address");
+            } catch (IllegalArgumentException e) {
+                assertThat(e, hasToString(containsString("unable to parse")));
+                assertThat(e, hasToString(containsString("as an IP address literal")));
+            }
+        }
+    }
+
+    public void testInvalidSpecificNetworkMaskCases() {
+        List<String> cases = new ArrayList<>();
+        cases.add("128.128.128.128/-1"); // network mask out of range
+        cases.add("128.128.128.128/33"); // network mask out of range
+        cases.add("128.128.128.128/abc"); // network mask that can not be parsed
+
+        for (String test : cases) {
+            try {
+                Cidrs.cidrMaskToMinMax(test);
+                fail("expected invalid network mask");
+            } catch (IllegalArgumentException e) {
+                assertThat(e, hasToString(containsString("network mask")));
+            }
+        }
+    }
+
+    public void testValidCombinations() {
+        for (long i = 0; i < (1 << 16); i++) {
+            for (int mask = 16; mask <= 32; mask++) {
+                String test = Cidrs.octetsToCIDR(Cidrs.longToOctets(i << 16), mask);
+                long[] actual = Cidrs.cidrMaskToMinMax(test);
+                assertNotNull(test, actual);
+                assertEquals(test, 2, actual.length);
+                assertEquals(test, i << 16, actual[0]);
+                assertEquals(test, (i << 16) + (1L << (32 - mask)), actual[1]);
+            }
+        }
+    }
+
+    public void testInvalidCombinations() {
+        List<String> cases = new ArrayList<>();
+        cases.add("192.168.0.1/24"); // invalid because fourth octet is not zero
+        cases.add("192.168.1.0/16"); // invalid because third octet is not zero
+        cases.add("192.1.0.0/8"); // invalid because second octet is not zero
+        cases.add("128.0.0.0/0"); // invalid because first octet is not zero
+        // create cases that have a bit set outside of the network mask
+        int value = 1;
+        for (int i = 0; i < 31; i++) {
+            cases.add(Cidrs.octetsToCIDR(Cidrs.longToOctets(value), 32 - i - 1));
+            value <<= 1;
+        }
+
+        for (String test : cases) {
+            try {
+                Cidrs.cidrMaskToMinMax(test);
+                fail("expected invalid combination");
+            } catch (IllegalArgumentException e) {
+                assertThat(test, e, hasToString(containsString("invalid address/network mask combination")));
+            }
+        }
+    }
+
+    public void testRandomValidCombinations() {
+        List<Tuple<String, Integer>> cases = new ArrayList<>();
+        // random number of strings with valid octets and valid network masks
+        for (int i = 0; i < randomIntBetween(1, 1024); i++) {
+            int networkMask = randomIntBetween(0, 32);
+            long mask = (1L << (32 - networkMask)) - 1;
+            long address = randomLongInIPv4Range() & ~mask;
+            cases.add(new Tuple<>(Cidrs.octetsToCIDR(Cidrs.longToOctets(address), networkMask), networkMask));
+        }
+
+        for (Tuple<String, Integer> test : cases) {
+            long[] actual = Cidrs.cidrMaskToMinMax(test.v1());
+            assertNotNull(test.v1(), actual);
+            assertEquals(test.v1(), 2, actual.length);
+            // assert the resulting block has the right size
+            assertEquals(test.v1(), 1L << (32 - test.v2()), actual[1] - actual[0]);
+        }
+    }
+
+    private long randomLongInIPv4Range() {
+        return randomLong() & 0x00000000FFFFFFFFL;
+    }
+}

core/src/test/java/org/elasticsearch/search/simple/SimpleSearchIT.java

@@ -107,7 +107,7 @@ public class SimpleSearchIT extends ESIntegTestCase {
         assertHitCount(search, 1l);
     }
 
-    public void testIpCIDR() throws Exception {
+    public void testIpCidr() throws Exception {
         createIndex("test");
 
         client().admin().indices().preparePutMapping("test").setType("type1")
@@ -129,20 +129,15 @@ public class SimpleSearchIT extends ESIntegTestCase {
         assertHitCount(search, 1l);
 
         search = client().prepareSearch()
-                .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "192.168.0.1/24")))
+                .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "192.168.0.0/24")))
                 .execute().actionGet();
         assertHitCount(search, 3l);
 
         search = client().prepareSearch()
-                .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "192.168.0.1/8")))
+                .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "192.0.0.0/8")))
                 .execute().actionGet();
         assertHitCount(search, 4l);
 
-        search = client().prepareSearch()
-                .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "192.168.1.1/24")))
-                .execute().actionGet();
-        assertHitCount(search, 1l);
-
         search = client().prepareSearch()
                 .setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "0.0.0.0/0")))
                 .execute().actionGet();
@@ -155,7 +150,7 @@ public class SimpleSearchIT extends ESIntegTestCase {
 
         assertFailures(client().prepareSearch().setQuery(boolQuery().must(QueryBuilders.termQuery("ip", "0/0/0/0/0"))),
                 RestStatus.BAD_REQUEST,
-                containsString("not a valid ip address"));
+                containsString("invalid IPv4/CIDR; expected [a.b.c.d, e] but was [[0, 0, 0, 0, 0]]"));
     }
 
     public void testSimpleId() {