From 8d0e0922a512a08a287c25bed6462fa244738603 Mon Sep 17 00:00:00 2001 From: Nikolay Fedorovskikh Date: Wed, 28 Jun 2017 15:54:55 +0500 Subject: [PATCH] SocksCmdRequest and SocksCmdResponse are trying to convert host from IDN for the non-DOMAIN address types Motivation: In the SocksCmdRequest and SocksCmdResponse constructors a host param converts from IDN to ascii compatible form regardless address type. Modifications: Use `IDN#toASCII` only for `DOMAIN` address type. Result: More correct host handling in socks commands. --- .../handler/codec/socks/SocksCmdRequest.java | 10 ++-- .../handler/codec/socks/SocksCmdResponse.java | 14 ++---- .../codec/socks/SocksCmdRequestTest.java | 50 +++++++++++++++++++ .../codec/socks/SocksCmdResponseTest.java | 48 ++++++++++++++++++ 4 files changed, 109 insertions(+), 13 deletions(-) diff --git a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdRequest.java b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdRequest.java index e47463492f..18bd65f7b9 100644 --- a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdRequest.java +++ b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdRequest.java @@ -51,9 +51,11 @@ public final class SocksCmdRequest extends SocksRequest { } break; case DOMAIN: - if (IDN.toASCII(host).length() > 255) { - throw new IllegalArgumentException(host + " IDN: " + IDN.toASCII(host) + " exceeds 255 char limit"); + String asciiHost = IDN.toASCII(host); + if (asciiHost.length() > 255) { + throw new IllegalArgumentException(host + " IDN: " + asciiHost + " exceeds 255 char limit"); } + host = asciiHost; break; case IPv6: if (!NetUtil.isValidIpV6Address(host)) { @@ -68,7 +70,7 @@ public final class SocksCmdRequest extends SocksRequest { } this.cmdType = cmdType; this.addressType = addressType; - this.host = IDN.toASCII(host); + this.host = host; this.port = port; } @@ -96,7 +98,7 @@ public final class SocksCmdRequest extends SocksRequest { * @return host that is used as a parameter in {@link SocksCmdType} */ public String host() { - return IDN.toUnicode(host); + return addressType == SocksAddressType.DOMAIN ? IDN.toUnicode(host) : host; } /** diff --git a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponse.java b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponse.java index a7a8b412e9..94ee51678e 100644 --- a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponse.java +++ b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponse.java @@ -75,10 +75,11 @@ public final class SocksCmdResponse extends SocksResponse { } break; case DOMAIN: - if (IDN.toASCII(host).length() > 255) { - throw new IllegalArgumentException(host + " IDN: " + - IDN.toASCII(host) + " exceeds 255 char limit"); + String asciiHost = IDN.toASCII(host); + if (asciiHost.length() > 255) { + throw new IllegalArgumentException(host + " IDN: " + asciiHost + " exceeds 255 char limit"); } + host = asciiHost; break; case IPv6: if (!NetUtil.isValidIpV6Address(host)) { @@ -88,7 +89,6 @@ public final class SocksCmdResponse extends SocksResponse { case UNKNOWN: break; } - host = IDN.toASCII(host); } if (port < 0 || port > 65535) { throw new IllegalArgumentException(port + " is not in bounds 0 <= x <= 65535"); @@ -126,11 +126,7 @@ public final class SocksCmdResponse extends SocksResponse { * or null when there was no host specified during response construction */ public String host() { - if (host != null) { - return IDN.toUnicode(host); - } else { - return null; - } + return host != null && addressType == SocksAddressType.DOMAIN ? IDN.toUnicode(host) : host; } /** diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdRequestTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdRequestTest.java index 1fc0d9c199..37439a7e6a 100644 --- a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdRequestTest.java +++ b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdRequestTest.java @@ -15,8 +15,13 @@ */ package io.netty.handler.codec.socks; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.util.CharsetUtil; import org.junit.Test; +import java.net.IDN; + import static org.junit.Assert.*; public class SocksCmdRequestTest { @@ -72,6 +77,51 @@ public class SocksCmdRequestTest { } } + @Test + public void testHostNotEncodedForUnknown() { + String asciiHost = "xn--e1aybc.xn--p1ai"; + short port = 10000; + + SocksCmdRequest rq = new SocksCmdRequest(SocksCmdType.BIND, SocksAddressType.UNKNOWN, asciiHost, port); + assertEquals(asciiHost, rq.host()); + + ByteBuf buffer = Unpooled.buffer(16); + rq.encodeAsByteBuf(buffer); + + buffer.resetReaderIndex(); + assertEquals(SocksProtocolVersion.SOCKS5.byteValue(), buffer.readByte()); + assertEquals(SocksCmdType.BIND.byteValue(), buffer.readByte()); + assertEquals((byte) 0x00, buffer.readByte()); + assertEquals(SocksAddressType.UNKNOWN.byteValue(), buffer.readByte()); + assertFalse(buffer.isReadable()); + + buffer.release(); + } + + @Test + public void testIDNEncodeToAsciiForDomain() { + String host = "тест.рф"; + String asciiHost = IDN.toASCII(host); + short port = 10000; + + SocksCmdRequest rq = new SocksCmdRequest(SocksCmdType.BIND, SocksAddressType.DOMAIN, host, port); + assertEquals(host, rq.host()); + + ByteBuf buffer = Unpooled.buffer(24); + rq.encodeAsByteBuf(buffer); + + buffer.resetReaderIndex(); + assertEquals(SocksProtocolVersion.SOCKS5.byteValue(), buffer.readByte()); + assertEquals(SocksCmdType.BIND.byteValue(), buffer.readByte()); + assertEquals((byte) 0x00, buffer.readByte()); + assertEquals(SocksAddressType.DOMAIN.byteValue(), buffer.readByte()); + assertEquals((byte) asciiHost.length(), buffer.readUnsignedByte()); + assertEquals(asciiHost, buffer.readCharSequence(asciiHost.length(), CharsetUtil.US_ASCII)); + assertEquals(port, buffer.readUnsignedShort()); + + buffer.release(); + } + @Test public void testValidPortRange() { try { diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseTest.java index 420c5108f7..6861f761de 100644 --- a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseTest.java +++ b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseTest.java @@ -17,8 +17,11 @@ package io.netty.handler.codec.socks; import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; +import io.netty.util.CharsetUtil; import org.junit.Test; +import java.net.IDN; + import static org.junit.Assert.*; public class SocksCmdResponseTest { @@ -108,6 +111,51 @@ public class SocksCmdResponseTest { assertByteBufEquals(expected, buffer); } + @Test + public void testHostNotEncodedForUnknown() { + String asciiHost = "xn--e1aybc.xn--p1ai"; + short port = 10000; + + SocksCmdResponse rs = new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.UNKNOWN, asciiHost, port); + assertEquals(asciiHost, rs.host()); + + ByteBuf buffer = Unpooled.buffer(16); + rs.encodeAsByteBuf(buffer); + + buffer.resetReaderIndex(); + assertEquals(SocksProtocolVersion.SOCKS5.byteValue(), buffer.readByte()); + assertEquals(SocksCmdStatus.SUCCESS.byteValue(), buffer.readByte()); + assertEquals((byte) 0x00, buffer.readByte()); + assertEquals(SocksAddressType.UNKNOWN.byteValue(), buffer.readByte()); + assertFalse(buffer.isReadable()); + + buffer.release(); + } + + @Test + public void testIDNEncodeToAsciiForDomain() { + String host = "тест.рф"; + String asciiHost = IDN.toASCII(host); + short port = 10000; + + SocksCmdResponse rs = new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.DOMAIN, host, port); + assertEquals(host, rs.host()); + + ByteBuf buffer = Unpooled.buffer(24); + rs.encodeAsByteBuf(buffer); + + buffer.resetReaderIndex(); + assertEquals(SocksProtocolVersion.SOCKS5.byteValue(), buffer.readByte()); + assertEquals(SocksCmdStatus.SUCCESS.byteValue(), buffer.readByte()); + assertEquals((byte) 0x00, buffer.readByte()); + assertEquals(SocksAddressType.DOMAIN.byteValue(), buffer.readByte()); + assertEquals((byte) asciiHost.length(), buffer.readUnsignedByte()); + assertEquals(asciiHost, buffer.readCharSequence(asciiHost.length(), CharsetUtil.US_ASCII)); + assertEquals(port, buffer.readUnsignedShort()); + + buffer.release(); + } + /** * Verifies that Response cannot be constructed with invalid IP. */