From 506ac2ca71ab44ebb8db9441da954422a75fe251 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 14 Sep 2016 19:51:21 -0700 Subject: [PATCH] NetUtil.bytesToIpAddress bug Motivation: NetUtil.bytesToIpAddress does not correctly translate IPv4 address to String. Also IPv6 addresses may not follow minimization conventions when converting to a String (see rfc 5952). Modifications: - NetUtil.bytesToIpAddress should correctly handle negative byte values for IPv4 - NetUtil.bytesToIpAddress should leverage existing to string conversion code in NetUtil Result: Fixes https://github.com/netty/netty/issues/5821 --- .../codec/socksx/v5/Socks5AddressDecoder.java | 2 +- .../v5/Socks5CommandRequestDecoderTest.java | 2 +- .../v5/Socks5CommandResponseDecoderTest.java | 5 +- .../src/main/java/io/netty/util/NetUtil.java | 61 ++++++++++--------- .../test/java/io/netty/util/NetUtilTest.java | 36 ++++++++--- 5 files changed, 62 insertions(+), 44 deletions(-) diff --git a/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java b/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java index 83b15c42f1..bc37469edb 100644 --- a/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java +++ b/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java @@ -52,7 +52,7 @@ public interface Socks5AddressDecoder { } else { byte[] tmp = new byte[IPv6_LEN]; in.readBytes(tmp); - return NetUtil.bytesToIpAddress(tmp, 0, IPv6_LEN); + return NetUtil.bytesToIpAddress(tmp); } } else { throw new DecoderException("unsupported address type: " + (addrType.byteValue() & 0xFF)); diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java index 8b0478a4b1..728ee1eb7a 100755 --- a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java +++ b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java @@ -67,7 +67,7 @@ public class Socks5CommandRequestDecoderTest { @Test public void testCmdRequestDecoderIPv6() { String[] hosts = { - NetUtil.bytesToIpAddress(IPAddressUtil.textToNumericFormatV6("::1"), 0, 16) }; + NetUtil.bytesToIpAddress(IPAddressUtil.textToNumericFormatV6("::1")) }; int[] ports = {1, 32769, 65535}; for (Socks5CommandType cmdType: Arrays.asList(Socks5CommandType.BIND, Socks5CommandType.CONNECT, diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java index 150a4169a1..23f8cbe589 100755 --- a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java +++ b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java @@ -22,7 +22,8 @@ import org.junit.Test; import java.util.Arrays; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; public class Socks5CommandResponseDecoderTest { @@ -92,7 +93,7 @@ public class Socks5CommandResponseDecoderTest { test(cmdStatus, Socks5AddressType.IPv6, "2001:db8:85a3:42:1000:8a2e:370:7334", 80); test(cmdStatus, Socks5AddressType.IPv6, - "1111:111:11:1:0:0:0:1", 80); + "1111:111:11:1::1", 80); } } } diff --git a/common/src/main/java/io/netty/util/NetUtil.java b/common/src/main/java/io/netty/util/NetUtil.java index b990d0e988..a645cd70d6 100644 --- a/common/src/main/java/io/netty/util/NetUtil.java +++ b/common/src/main/java/io/netty/util/NetUtil.java @@ -16,7 +16,6 @@ package io.netty.util; import io.netty.util.internal.PlatformDependent; -import io.netty.util.internal.StringUtil; import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLoggerFactory; @@ -500,6 +499,16 @@ public final class NetUtil { return buf.toString(); } + /** + * Converts 4-byte or 16-byte data into an IPv4 or IPv6 string respectively. + * + * @throws IllegalArgumentException + * if {@code length} is not {@code 4} nor {@code 16} + */ + public static String bytesToIpAddress(byte[] bytes) { + return bytesToIpAddress(bytes, 0, bytes.length); + } + /** * Converts 4-byte or 16-byte data into an IPv4 or IPv6 string respectively. * @@ -507,34 +516,22 @@ public final class NetUtil { * if {@code length} is not {@code 4} nor {@code 16} */ public static String bytesToIpAddress(byte[] bytes, int offset, int length) { - if (length == 4) { - StringBuilder buf = new StringBuilder(15); - - buf.append(bytes[offset ++] >> 24 & 0xff); - buf.append('.'); - buf.append(bytes[offset ++] >> 16 & 0xff); - buf.append('.'); - buf.append(bytes[offset ++] >> 8 & 0xff); - buf.append('.'); - buf.append(bytes[offset] & 0xff); - - return buf.toString(); - } - - if (length == 16) { - final StringBuilder sb = new StringBuilder(39); - final int endOffset = offset + 14; - - for (; offset < endOffset; offset += 2) { - StringUtil.toHexString(sb, bytes, offset, 2); - sb.append(':'); + switch (length) { + case 4: { + return new StringBuilder(15) + .append(bytes[offset] & 0xff) + .append('.') + .append(bytes[offset + 1] & 0xff) + .append('.') + .append(bytes[offset + 2] & 0xff) + .append('.') + .append(bytes[offset + 3] & 0xff).toString(); } - StringUtil.toHexString(sb, bytes, offset, 2); - - return sb.toString(); + case 16: + return toAddressString(bytes, offset, false); + default: + throw new IllegalArgumentException("length: " + length + " (expected: 4 or 16)"); } - - throw new IllegalArgumentException("length: " + length + " (expected: 4 or 16)"); } public static boolean isValidIpV6Address(String ipAddress) { @@ -981,13 +978,17 @@ public final class NetUtil { return ip.getHostAddress(); } if (!(ip instanceof Inet6Address)) { - throw new IllegalArgumentException("Unhandled type: " + ip.getClass()); + throw new IllegalArgumentException("Unhandled type: " + ip); } - final byte[] bytes = ip.getAddress(); + return toAddressString(ip.getAddress(), 0, ipv4Mapped); + } + + private static String toAddressString(byte[] bytes, int offset, boolean ipv4Mapped) { final int[] words = new int[IPV6_WORD_COUNT]; int i; - for (i = 0; i < words.length; ++i) { + final int end = offset + words.length; + for (i = offset; i < end; ++i) { words[i] = ((bytes[i << 1] & 0xff) << 8) | (bytes[(i << 1) + 1] & 0xff); } diff --git a/common/src/test/java/io/netty/util/NetUtilTest.java b/common/src/test/java/io/netty/util/NetUtilTest.java index f0f600cf8a..ecd60d5d3c 100644 --- a/common/src/test/java/io/netty/util/NetUtilTest.java +++ b/common/src/test/java/io/netty/util/NetUtilTest.java @@ -17,12 +17,17 @@ package io.netty.util; import org.junit.Test; +import java.net.Inet6Address; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; +import static io.netty.util.NetUtil.bytesToIpAddress; +import static io.netty.util.NetUtil.createByteArrayFromIpAddressString; +import static io.netty.util.NetUtil.getByName; +import static io.netty.util.NetUtil.toAddressString; import static org.junit.Assert.*; public class NetUtilTest { @@ -44,7 +49,8 @@ public class NetUtilTest { "10.255.255.254", "0afffffe", "172.18.5.4", "ac120504", "0.0.0.0", "00000000", - "127.0.0.1", "7f000001"); + "127.0.0.1", "7f000001", + "1.2.3.4", "01020304"); private static final Map invalidIpV4Hosts = new TestMap( "1.256.3.4", null, @@ -438,30 +444,40 @@ public class NetUtilTest { @Test public void testCreateByteArrayFromIpAddressString() { for (Entry e : validIpV4Hosts.entrySet()) { - assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey())); + assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey())); } for (Entry e : invalidIpV4Hosts.entrySet()) { - assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey())); + assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey())); } for (Entry e : validIpV6Hosts.entrySet()) { - assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey())); + assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey())); } for (Entry e : invalidIpV6Hosts.entrySet()) { - assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey())); + assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey())); + } + } + + @Test + public void testBytesToIpAddress() throws UnknownHostException { + for (Entry e : validIpV4Hosts.entrySet()) { + assertEquals(e.getKey(), bytesToIpAddress(createByteArrayFromIpAddressString(e.getKey()))); + } + for (Entry testEntry : ipv6ToAddressStrings.entrySet()) { + assertEquals(testEntry.getValue(), bytesToIpAddress(testEntry.getKey())); } } @Test public void testIp6AddressToString() throws UnknownHostException { for (Entry testEntry : ipv6ToAddressStrings.entrySet()) { - assertEquals(testEntry.getValue(), NetUtil.toAddressString(InetAddress.getByAddress(testEntry.getKey()))); + assertEquals(testEntry.getValue(), toAddressString(InetAddress.getByAddress(testEntry.getKey()))); } } @Test public void testIp4AddressToString() throws UnknownHostException { for (Entry e : validIpV4Hosts.entrySet()) { - assertEquals(e.getKey(), NetUtil.toAddressString(InetAddress.getByAddress(unhex(e.getValue())))); + assertEquals(e.getKey(), toAddressString(InetAddress.getByAddress(unhex(e.getValue())))); } } @@ -470,18 +486,18 @@ public class NetUtilTest { for (Entry testEntry : ipv4MappedToIPv6AddressStrings.entrySet()) { assertEquals( testEntry.getValue(), - NetUtil.toAddressString(NetUtil.getByName(testEntry.getKey(), true), true)); + toAddressString(getByName(testEntry.getKey(), true), true)); } } @Test public void testinvalidIpv4MappedIp6GetByName() { for (String testEntry : invalidIpV4Hosts.keySet()) { - assertNull(NetUtil.getByName(testEntry, true)); + assertNull(getByName(testEntry, true)); } for (String testEntry : invalidIpV6Hosts.keySet()) { - assertNull(NetUtil.getByName(testEntry, true)); + assertNull(getByName(testEntry, true)); } }