NetUtil IPv6 bugs related to IPv4 and compression #6613

Scottmitch · 2017-04-06T23:30:41Z

Motivation:
NetUtil#getByName and NetUtil#isValidIpV6Address do not strictly enforce the format of IPv4 addresses that are allowed to be embedded in IPv6 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5. This may lead to invalid addresses being parsed, or invalid addresses being considered valid.

Modifications:

NetUtil#isValidIpV6Address should enforce the IPv4-Compatible and IPv4-Mapped are the only valid formats for including IPv4 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5
NetUtil#getByName should more stritcly parse IPv6 addresses which contain IPv4 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5

Result:
NetUtil#getByName and NetUtil#isValidIpV6Address respect the IPv6 RFC which defines the valid formats for embedding IPv4 addresses.

Scottmitch · 2017-04-06T23:31:30Z

@trustin @fenik17 - PTAL. Please suggest additional unit tests if you think of interesting test cases.

netkins · 2017-04-07T00:59:36Z

common/src/main/java/io/netty/util/NetUtil.java

+     * </ul>
+     * @return byte array representation of the {@code ip} or {@code null} if not a valid IP address.
+     */
+    private static byte[] getIPv6ByName(CharSequence ip, boolean ipv4Mapped) {


The Cyclomatic Complexity of this method "getIPv6ByName" is 93 which is greater than 25 authorized.

fenik17 · 2017-04-07T08:32:55Z

Can we add support for IPv6 addresses with [] and % into a getIPv6ByName() method? Something like that:

public static byte[] createByteArrayFromIpAddressString(String ipAddressString) {
    if (isValidIpV4Address(ipAddressString)) {
        ....
    }
    return getIPv6ByName(ipAddressString, true);
}

private static byte[] getIPv6ByName(CharSequence ip, boolean ipv4Mapped) {
    ...
    int startIdx; // instead of '0' below
    int endIdx;   // instead of 'ipLength - 1' below
    if (ip.charAt(0) == '[') {
        startIdx = 1;
        endIdx = ip.length() - 2;
    } else {
        startIdx = 0;
        endIdx = ip.length() - 1;
    }
    ...
    loop: for (int i = startIdx; i <= endIdx; i++) {
        ...
        case '%':
            endIdx = i - 1;
            break loop;
        }
        ...
    }
    ...
    if (startIdx == 1 && (ipv6Seperators == 0 || ip.charAt(ip.length() - 1) != ']')) {
        // "[x.x.x.x]" - invalid ipv4
        // "[x:x:x::x" - invalid ipv6
        return null;
    }
    ...
}

This will allow us not to call a isValidIpV6Address() method in the createByteArrayFromIpAddressString(), and make getByName more universal. Also, we will be able to test getByName() on a par with the isValidIpV4Address() and isValidIpV6Address():

    @Test
    public void testIsValidIpV4Address() {
        for (String host : validIpV4Hosts.keySet()) {
            assertTrue(host, NetUtil.isValidIpV4Address(host));
            assertNotNull(host, NetUtil.getByName(host));
        }
        for (String host : invalidIpV4Hosts.keySet()) {
            assertFalse(host, NetUtil.isValidIpV4Address(host));
            assertNull(host, NetUtil.getByName(host));
        }
    }

    @Test
    public void testIsValidIpV6Address() {
        for (String host : validIpV6Hosts.keySet()) {
            assertTrue(host, NetUtil.isValidIpV6Address(host));
            assertNotNull(host, NetUtil.getByName(host));
        }
        for (String host : invalidIpV6Hosts.keySet()) {
            assertFalse(host, NetUtil.isValidIpV6Address(host));
            assertNull(host, NetUtil.getByName(host));
        }
    }

fenik17 · 2017-04-07T11:55:11Z

In addition to the previous comment - InetAddress.getByName() from the JDK supports an [] and %.

fenik17 · 2017-04-07T13:17:31Z

common/src/main/java/io/netty/util/NetUtil.java

+            !isValidIPv4MappedSeparators(bytes[currentIndex - 1], bytes[currentIndex - 2])) {
+            return false;
+        }
+        currentIndex -= 2;


Maybe -= 2 with an Unsafe and -= 3 without?
With -2 test failed for ip "::ffff:1.2.3.4" whithout an Unsafe.

Netkins doesn't run tests with -Dio.netty.noUnsafe=true?

good catch ... should be -=3 ... the indexing is off the for unsafe case.

Netkins doesn't run tests with -Dio.netty.noUnsafe=true?

No. It just assumes the default for the platform.

fenik17 · 2017-04-07T14:08:38Z

common/src/main/java/io/netty/util/NetUtil.java

+                                         !isValidNumericChar(ip.charAt(i - 3))) ||
+                            tmp == 2 && (!isValidNumericChar(ip.charAt(i - 1)) ||
+                                         !isValidNumericChar(ip.charAt(i - 2))) ||
+                            tmp == 1 && !isValidNumericChar(ip.charAt(i - 1)))) {


all of last part need only for the ipv4Seperators == 1:

|| ( ipv4Seperators == 1 && ( (!ipv4Mapped || currentIndex != 0 && !isValidIPv4Mapped(bytes, currentIndex)) || (tmp == 3 && (!isValidNumericChar(ip.charAt(i - 1)) || !isValidNumericChar(ip.charAt(i - 2)) || !isValidNumericChar(ip.charAt(i - 3))) || tmp == 2 && (!isValidNumericChar(ip.charAt(i - 1)) || !isValidNumericChar(ip.charAt(i - 2))) || tmp == 1 && !isValidNumericChar(ip.charAt(i - 1))) ) ) {

Good catch. The validation after we see the first ipv4Separators is done in the default case below.

fenik17 · 2017-04-07T14:18:15Z

@Scottmitch fix the typos ipv6Seperators -> ipv6Separators, ipv4Seperators -> ipv4Separators please :)

Scottmitch · 2017-04-07T16:47:06Z

Can we add support for IPv6 addresses with [] and % into a getIPv6ByName() method?

Lets do a followup PR for this enhancement.

netkins · 2017-04-07T18:12:12Z

common/src/main/java/io/netty/util/NetUtil.java

-                        || (ipv6Seperators > 0 && (currentIndex + compressLength < 12))
+                ++ipv4Separators;
+                tmp = i - begin; // tmp is the length of the current segment.
+                if (tmp > IPV4_MAX_CHAR_BETWEEN_SEPARATOR


Reduce the number of conditional operators (19) used in the expression (maximum allowed 5).

fenik17 · 2017-04-07T19:22:19Z

convertToBytes() can be removed at now.

Lets do a followup PR for this enhancement.

Ok.

Scottmitch · 2017-04-07T20:43:07Z

convertToBytes() can be removed at now.

+1

fenik17 · 2017-04-09T20:20:32Z

Please suggest additional unit tests if you think of interesting test cases.

0:0:0:0:0:ffff::10.0.0.1

isValidIpV6Address("0:0:0:0:0:ffff::10.0.0.1") => false
getByName("0:0:0:0:0:ffff::10.0.0.1")          => not null

normanmaurer

lgtm...

normanmaurer · 2017-04-21T15:42:22Z

Let me merge this...

fenik17 · 2017-04-21T18:15:03Z

The getByName() method still does not check IPv4-Mapped addresses, e.g. for "1:0:0:0:0:ffff:10.0.0.1" or "0:0:0:0:0:ffff::10.0.0.1" it return a not null.
But isValidIpV6Address() has such check (return false).

fenik17 · 2017-04-21T18:25:26Z

I can fix it in another PR if @Scottmitch have not time..

Scottmitch · 2017-04-24T16:57:12Z

@fenik17 - Thanks for pointing out this unit test .. let me add this unit test and fix the issue

fenik17 · 2017-04-24T19:13:41Z

common/src/main/java/io/netty/util/NetUtil.java

                compressLength = 12;
-            } else if (ipv6Seperators >= IPV6_MIN_SEPARATORS &&
+            } else if (ipv6Separators >= IPV6_MIN_SEPARATORS &&
                           ip.charAt(ipLength - 1) != ':' &&


this check - ip.charAt(ipLength - 1) != ':' - seems superfluous or wrong for the ipv4Separators > 0.

agreed ... this should be caught in the loop above.

fenik17 · 2017-04-24T21:16:08Z

One more unit test for invalid address: "1:0:0:0:0:ffff:10.0.0.1".
Problem in the isValidIPv4Mapped().

fenik17 · 2017-04-24T21:21:58Z

Maybe better use equals() with zeroed array instead of getLong() / getInt() in the isValidIPv4Mapped()?

netkins · 2017-04-24T22:07:14Z

common/src/main/java/io/netty/util/NetUtil.java

-                           ip.charAt(ipLength - 1) != ':' &&
-                           (!isCompressed && (ipv6Seperators == 6 && ip.charAt(0) != ':') ||
-                            isCompressed && (ipv6Seperators + 1 < IPV6_MAX_SEPARATORS &&
+            } else if (ipv6Separators >= IPV6_MIN_SEPARATORS &&


Reduce the number of conditional operators (7) used in the expression (maximum allowed 5).

Scottmitch · 2017-04-24T23:25:22Z

Maybe better use equals() with zeroed array instead of getLong() / getInt() in the isValidIPv4Mapped()?

I considered doing this initially ... I will just add a isZero method to PlatformDependent instead to make this more generally available an independently unit tested. This way we won't need an intermediate allocation.

Motivation: NetUtil#getByName and NetUtil#isValidIpV6Address do not strictly enforce the format of IPv4 addresses that are allowed to be embedded in IPv6 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5. This may lead to invalid addresses being parsed, or invalid addresses being considered valid. Compression of a single IPv6 word was also not handled correctly if there are 7 : characters. Modifications: - NetUtil#isValidIpV6Address should enforce the IPv4-Compatible and IPv4-Mapped are the only valid formats for including IPv4 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5 - NetUtil#getByName should more stritcly parse IPv6 addresses which contain IPv4 addresses as specified in https://tools.ietf.org/html/rfc4291#section-2.5.5 - NetUtil should allow compression even if the number of : characters is 7. - NetUtil#createByteArrayFromIpAddressString should use the same IP string to byte[] translation which is used in NetUtil#getByName Result: NetUtil#getByName and NetUtil#isValidIpV6Address respect the IPv6 RFC which defines the valid formats for embedding IPv4 addresses.

Scottmitch · 2017-04-25T22:31:54Z

4.1 (9cb858f) 4.0 (241cdc6)

fenik17 · 2017-04-27T15:55:13Z

common/src/test/java/io/netty/util/NetUtilTest.java

+            "0:0:0:0:0:0:10.0.0.1",                      "00000000000000000000ffff0a000001",
+            "0:0:0:0:0::10.0.0.1",                       "00000000000000000000ffff0a000001",
+            "::0:0:0:0:0:10.0.0.1",                      "00000000000000000000ffff0a000001",
+            "0:0:0:0:0:0::10.0.0.1",                     "00000000000000000000ffff0a000001",


@Scottmitch oops. Is it really valid?

Yes we allow IPv4 compatible addresses as input. The rational is to be liberal in what we accept but more strict in what we generate (we only generate IPv4 mapped).

// We allow IPv4 Mapped (https://tools.ietf.org/html/rfc4291#section-2.5.5.1) // and IPv4 compatible (https://tools.ietf.org/html/rfc4291#section-2.5.5.1). // The IPv4 compatible is deprecated, but it allows parsing of plain IPv4 addressed into IPv6-Mapped addresses.

I'm about of count of zeros and "::" after. There can not be more bits in place of "::".
This is confirmed by examples [1] and some online-validators: [2] and [3].

3. An alternative form that is sometimes more convenient when dealing with a mixed environment of IPv4 and IPv6 nodes is x:x:x:x:x:x:d.d.d.d, where the 'x's are the hexadecimal values of the six high-order 16-bit pieces of the address, and the 'd's are the decimal values of the four low-order 8-bit pieces of the address (standard IPv4 representation). Examples: 0:0:0:0:0:0:13.1.68.3 0:0:0:0:0:FFFF:129.144.52.38 or in compressed form: ::13.1.68.3 ::FFFF:129.144.52.38

[1] https://tools.ietf.org/html/rfc4291#section-2.2
[2] http://formvalidation.io/validators/ip/ ("Try it" in the end)
[3] https://www.helpsystems.com/intermapper/ipv6-test-address-validation

Thanks for pointing this out. I agree there is a bug. Let me come up with a PR.

Scottmitch added the defect label Apr 6, 2017

Scottmitch added this to the 4.0.46.Final milestone Apr 6, 2017

Scottmitch self-assigned this Apr 6, 2017

Scottmitch requested a review from trustin April 6, 2017 23:30

Scottmitch mentioned this pull request Apr 6, 2017

Optimizations in NetUtil #6493

Closed

netkins reviewed Apr 7, 2017

View reviewed changes

Scottmitch force-pushed the netutil_bugs branch from c910986 to bf59daf Compare April 7, 2017 01:04

fenik17 reviewed Apr 7, 2017

View reviewed changes

netkins reviewed Apr 7, 2017

View reviewed changes

Scottmitch force-pushed the netutil_bugs branch from f1ab45b to e0105eb Compare April 7, 2017 19:07

Scottmitch force-pushed the netutil_bugs branch from e0105eb to b2aeee0 Compare April 7, 2017 20:44

normanmaurer approved these changes Apr 18, 2017

View reviewed changes

trustin approved these changes Apr 19, 2017

View reviewed changes

fenik17 reviewed Apr 24, 2017

View reviewed changes

Scottmitch force-pushed the netutil_bugs branch 3 times, most recently from ff2b72d to 9f0c151 Compare April 24, 2017 20:37

netkins reviewed Apr 24, 2017

View reviewed changes

Scottmitch force-pushed the netutil_bugs branch from 9f0c151 to 2751c01 Compare April 24, 2017 23:26

fenik17 approved these changes Apr 25, 2017

View reviewed changes

Scottmitch closed this Apr 25, 2017

Scottmitch deleted the netutil_bugs branch April 25, 2017 22:31

fenik17 reviewed Apr 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetUtil IPv6 bugs related to IPv4 and compression #6613

NetUtil IPv6 bugs related to IPv4 and compression #6613

Scottmitch commented Apr 6, 2017

Scottmitch commented Apr 6, 2017

netkins Apr 7, 2017 •

edited

Loading

fenik17 commented Apr 7, 2017 •

edited

Loading

fenik17 commented Apr 7, 2017

fenik17 Apr 7, 2017 •

edited

Loading

fenik17 Apr 7, 2017

Scottmitch Apr 7, 2017

fenik17 Apr 7, 2017

Scottmitch Apr 7, 2017

fenik17 commented Apr 7, 2017

Scottmitch commented Apr 7, 2017

netkins Apr 7, 2017

fenik17 commented Apr 7, 2017

Scottmitch commented Apr 7, 2017

fenik17 commented Apr 9, 2017

normanmaurer left a comment

normanmaurer commented Apr 21, 2017

fenik17 commented Apr 21, 2017 •

edited

Loading

fenik17 commented Apr 21, 2017

Scottmitch commented Apr 24, 2017

fenik17 Apr 24, 2017

Scottmitch Apr 24, 2017

fenik17 commented Apr 24, 2017 •

edited

Loading

fenik17 commented Apr 24, 2017 •

edited

Loading

netkins Apr 24, 2017

Scottmitch commented Apr 24, 2017

Scottmitch commented Apr 25, 2017

fenik17 Apr 27, 2017

Scottmitch Apr 27, 2017

fenik17 Apr 27, 2017 •

edited

Loading

Scottmitch Apr 27, 2017

Scottmitch Apr 27, 2017

NetUtil IPv6 bugs related to IPv4 and compression #6613

NetUtil IPv6 bugs related to IPv4 and compression #6613

Conversation

Scottmitch commented Apr 6, 2017

Scottmitch commented Apr 6, 2017

netkins Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

fenik17 commented Apr 7, 2017 • edited Loading

fenik17 commented Apr 7, 2017

fenik17 Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenik17 commented Apr 7, 2017

Scottmitch commented Apr 7, 2017

Choose a reason for hiding this comment

fenik17 commented Apr 7, 2017

Scottmitch commented Apr 7, 2017

fenik17 commented Apr 9, 2017

normanmaurer left a comment

Choose a reason for hiding this comment

normanmaurer commented Apr 21, 2017

fenik17 commented Apr 21, 2017 • edited Loading

fenik17 commented Apr 21, 2017

Scottmitch commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenik17 commented Apr 24, 2017 • edited Loading

fenik17 commented Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Scottmitch commented Apr 24, 2017

Scottmitch commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenik17 Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netkins Apr 7, 2017 •

edited

Loading

fenik17 commented Apr 7, 2017 •

edited

Loading

fenik17 Apr 7, 2017 •

edited

Loading

fenik17 commented Apr 21, 2017 •

edited

Loading

fenik17 commented Apr 24, 2017 •

edited

Loading

fenik17 commented Apr 24, 2017 •

edited

Loading

fenik17 Apr 27, 2017 •

edited

Loading