Repository: netty/netty
Issue: SOCKS5 Response command does not support bound host and port

SOCKS5 protocol defines that response to CONNECT command can contain host and port the proxy server connects to the target from. Current implementation fills these fields with zeroes and doesn't support customization.

The support for bound fields should be added to io.netty.handler.codec.socks.SocksCmdResponse class.


Issue comments:
PR welcome :)

> Am 02.02.2014 um 20:29 schrieb Vladimír Schäfer notifications@github.com:
> 
> SOCKS5 protocol defines that response to CONNECT command can contain host and port the proxy server connects to the target from. Current implementation fills these fields with zeroes and doesn't support customization.
> 
> The support for bound fields should be added to io.netty.handler.codec.socks.SocksCmdResponse class.
> 
> —
> Reply to this email directly or view it on GitHub.

On the way, back in few minutes :)

Provided you'd decide to accept the pull - do you think I could send another one for the 4.0.x branch? It's essentially an improvement, not a bug fix, but perhaps it's not against your policies of merging to fix releases?

@vschafer don't worry I will cherry pick your change to 4.0 once it was merged ;)

@vschafer thanks for your work.
btw where did this feature is used ?

You should add checks in SocksCmdResponse constructor for provided boundHost and boundPort just like it is done in SocksCmdRequest

@normanmaurer Thank you Norman :)

@bk1te No problem, I'll add the checks later today. My worry was that this could break backwards compatibility - someone was sending invalid data for e.g. IPv4 earlier (100.100.1), but as it was essentially ignored (except for a length check) it didn't fail the decoding process. Now someone updates to 4.0.x and boom there's a regression as the constructor will reject the data. Not very likely, but possible. So what if we'd add the constructor check for 5.0, but skip it in 4.x?

I'm prototyping a custom proxy for a cloud application and using Netty (SOCKS5+WebSocket) as part of it. When testing with Putty (configured to use SOCKS5) I ran into an issue when Putty interpreted the zeroed host+port incorrectly and added an extra 0x00 into the actual data stream.

@vschafer yeah I think adding the check only in master is ok :)

@vschafer @normanmaurer I think this checks would not break comptability, because you just add new constructor with additional data and didn`t remove the old one without bounded address. For IPv4 and IPv6 address types your point with invalid address is not an issue, cause you convert bytes to address. Your example is for domain address type and for it it is not an issue, in socks rfc there is nothing written about domain address type validation, so you would return SocksCmdResponse with address type DOMAIN and requested address and port.
So I vote for checks everywhere! 

@bk1te @vschafer true... so checks everywhere ;)

@bk1te @normanmaurer Yes, the old constructor stays, but the SocksCmdResponseDecoder has changed - it now calls the new constructor and that's the place where (imho) you could now get an IllegalArgumentException when receiving a SocksResponse (while e.g. writing an application which acts as a SOCKS5 client). This could happen in case you receive an invalid IPv4 or IPv6 address in the incoming stream (e.g. correct amount of bytes resulting in string "127.000000.000" with subsequent fail in NetUtil.isValidIpV4Address). I agree that there's no issue with the domain address types.

This is really marginal and I'm just trying to convey any possible risks related to the patch :) The implementation of checks will follow later today.

Why do you think that if you pass 4 bytes you would receive incorrect IPv4 address and same for IPv6 address

My bad, I lived in a world where the whole string, dots included, gets sent over the wire. Just disregard it, aka so checks everywhere.


Pull request review comments:
maybe move this after the switch so we not convert an invalid address ?

could be static


Patch that resolved the issue:
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 03218b69213..567ceb63fff 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
@@ -16,9 +16,13 @@
 package io.netty.handler.codec.socks;
 
 import io.netty.buffer.ByteBuf;
+import io.netty.util.CharsetUtil;
+import io.netty.util.NetUtil;
+
+import java.net.IDN;
 
 /**
- * An socks cmd response.
+ * A socks cmd response.
  *
  * @see SocksCmdRequest
  * @see SocksCmdResponseDecoder
@@ -27,7 +31,11 @@ public final class SocksCmdResponse extends SocksResponse {
     private final SocksCmdStatus cmdStatus;
 
     private final SocksAddressType addressType;
+    private final String host;
+    private final int port;
+
     // All arrays are initialized on construction time to 0/false/null remove array Initialization
+    private static final byte[] DOMAIN_ZEROED = {0x00};
     private static final byte[] IPv4_HOSTNAME_ZEROED = {0x00, 0x00, 0x00, 0x00};
     private static final byte[] IPv6_HOSTNAME_ZEROED = {0x00, 0x00, 0x00, 0x00,
             0x00, 0x00, 0x00, 0x00,
@@ -35,6 +43,23 @@ public final class SocksCmdResponse extends SocksResponse {
             0x00, 0x00, 0x00, 0x00};
 
     public SocksCmdResponse(SocksCmdStatus cmdStatus, SocksAddressType addressType) {
+        this(cmdStatus, addressType, null, 0);
+    }
+
+    /**
+     * Constructs new response and includes provided host and port as part of it.
+     *
+     * @param cmdStatus status of the response
+     * @param addressType type of host parameter
+     * @param host host (BND.ADDR field) is address that server used when connecting to the target host.
+     *             When null a value of 4/8 0x00 octets will be used for IPv4/IPv6 and a single 0x00 byte will be
+     *             used for domain addressType. Value is converted to ASCII using {@link IDN#toASCII(String)}.
+     * @param port port (BND.PORT field) that the server assigned to connect to the target host
+     * @throws NullPointerException in case cmdStatus or addressType are missing
+     * @throws IllegalArgumentException in case host or port cannot be validated
+     * @see IDN#toASCII(String)
+     */
+    public SocksCmdResponse(SocksCmdStatus cmdStatus, SocksAddressType addressType, String host, int port) {
         super(SocksResponseType.CMD);
         if (cmdStatus == null) {
             throw new NullPointerException("cmdStatus");
@@ -42,8 +67,36 @@ public SocksCmdResponse(SocksCmdStatus cmdStatus, SocksAddressType addressType)
         if (addressType == null) {
             throw new NullPointerException("addressType");
         }
+        if (host != null) {
+            switch (addressType) {
+                case IPv4:
+                    if (!NetUtil.isValidIpV4Address(host)) {
+                        throw new IllegalArgumentException(host + " is not a valid IPv4 address");
+                    }
+                    break;
+                case DOMAIN:
+                    if (IDN.toASCII(host).length() > 255) {
+                        throw new IllegalArgumentException(host + " IDN: " +
+                                IDN.toASCII(host) + " exceeds 255 char limit");
+                    }
+                    break;
+                case IPv6:
+                    if (!NetUtil.isValidIpV6Address(host)) {
+                        throw new IllegalArgumentException(host + " is not a valid IPv6 address");
+                    }
+                    break;
+                case UNKNOWN:
+                    break;
+            }
+            host = IDN.toASCII(host);
+        }
+        if (port < 0 && port >= 65535) {
+            throw new IllegalArgumentException(port + " is not in bounds 0 < x < 65536");
+        }
         this.cmdStatus = cmdStatus;
         this.addressType = addressType;
+        this.host = host;
+        this.port = port;
     }
 
     /**
@@ -64,6 +117,32 @@ public SocksAddressType addressType() {
         return addressType;
     }
 
+    /**
+     * Returns host that is used as a parameter in {@link io.netty.handler.codec.socks.SocksCmdType}.
+     * Host (BND.ADDR field in response) is address that server used when connecting to the target host.
+     * This is typically different from address which client uses to connect to the SOCKS server.
+     *
+     * @return host that is used as a parameter in {@link io.netty.handler.codec.socks.SocksCmdType}
+     *         or null when there was no host specified during response construction
+     */
+    public String host() {
+        if (host != null) {
+            return IDN.toUnicode(host);
+        } else {
+            return null;
+        }
+    }
+
+    /**
+     * Returns port that is used as a parameter in {@link io.netty.handler.codec.socks.SocksCmdType}.
+     * Port (BND.PORT field in response) is port that the server assigned to connect to the target host.
+     *
+     * @return port that is used as a parameter in {@link io.netty.handler.codec.socks.SocksCmdType}
+     */
+    public int port() {
+        return port;
+    }
+
     @Override
     public void encodeAsByteBuf(ByteBuf byteBuf) {
         byteBuf.writeByte(protocolVersion().byteValue());
@@ -72,19 +151,25 @@ public void encodeAsByteBuf(ByteBuf byteBuf) {
         byteBuf.writeByte(addressType.byteValue());
         switch (addressType) {
             case IPv4: {
-                byteBuf.writeBytes(IPv4_HOSTNAME_ZEROED);
-                byteBuf.writeShort(0);
+                byte[] hostContent = host == null ?
+                        IPv4_HOSTNAME_ZEROED : NetUtil.createByteArrayFromIpAddressString(host);
+                byteBuf.writeBytes(hostContent);
+                byteBuf.writeShort(port);
                 break;
             }
             case DOMAIN: {
-                byteBuf.writeByte(1);   // domain length
-                byteBuf.writeByte(0);   // domain value
-                byteBuf.writeShort(0);  // port value
+                byte[] hostContent = host == null ?
+                        DOMAIN_ZEROED : host.getBytes(CharsetUtil.US_ASCII);
+                byteBuf.writeByte(hostContent.length);   // domain length
+                byteBuf.writeBytes(hostContent);   // domain value
+                byteBuf.writeShort(port);  // port value
                 break;
             }
             case IPv6: {
-                byteBuf.writeBytes(IPv6_HOSTNAME_ZEROED);
-                byteBuf.writeShort(0);
+                byte[] hostContent = host == null
+                        ? IPv6_HOSTNAME_ZEROED : NetUtil.createByteArrayFromIpAddressString(host);
+                byteBuf.writeBytes(hostContent);
+                byteBuf.writeShort(port);
                 break;
             }
         }
diff --git a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponseDecoder.java b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponseDecoder.java
index e313128be49..d93674b752d 100644
--- a/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponseDecoder.java
+++ b/codec-socks/src/main/java/io/netty/handler/codec/socks/SocksCmdResponseDecoder.java
@@ -67,20 +67,20 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf byteBuf, List<Object> o
                     case IPv4: {
                         host = SocksCommonUtils.intToIp(byteBuf.readInt());
                         port = byteBuf.readUnsignedShort();
-                        msg = new SocksCmdResponse(cmdStatus, addressType);
+                        msg = new SocksCmdResponse(cmdStatus, addressType, host, port);
                         break;
                     }
                     case DOMAIN: {
                         fieldLength = byteBuf.readByte();
                         host = byteBuf.readBytes(fieldLength).toString(CharsetUtil.US_ASCII);
                         port = byteBuf.readUnsignedShort();
-                        msg = new SocksCmdResponse(cmdStatus, addressType);
+                        msg = new SocksCmdResponse(cmdStatus, addressType, host, port);
                         break;
                     }
                     case IPv6: {
                         host = SocksCommonUtils.ipv6toStr(byteBuf.readBytes(16).array());
                         port = byteBuf.readUnsignedShort();
-                        msg = new SocksCmdResponse(cmdStatus, addressType);
+                        msg = new SocksCmdResponse(cmdStatus, addressType, host, port);
                         break;
                     }
                     case UNKNOWN:


Test patch:
diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseDecoderTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseDecoderTest.java
index 62af41aa7cf..1a98a8f765e 100644
--- a/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseDecoderTest.java
+++ b/codec-socks/src/test/java/io/netty/handler/codec/socks/SocksCmdResponseDecoderTest.java
@@ -26,27 +26,58 @@ public class SocksCmdResponseDecoderTest {
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(SocksCmdResponseDecoderTest.class);
 
     private static void testSocksCmdResponseDecoderWithDifferentParams(
-            SocksCmdStatus cmdStatus, SocksAddressType addressType) {
+            SocksCmdStatus cmdStatus, SocksAddressType addressType, String host, int port) {
         logger.debug("Testing cmdStatus: " + cmdStatus + " addressType: " + addressType);
-        SocksResponse msg = new SocksCmdResponse(cmdStatus, addressType);
+        SocksResponse msg = new SocksCmdResponse(cmdStatus, addressType, host, port);
         SocksCmdResponseDecoder decoder = new SocksCmdResponseDecoder();
         EmbeddedChannel embedder = new EmbeddedChannel(decoder);
         SocksCommonTestUtils.writeMessageIntoEmbedder(embedder, msg);
         if (addressType == SocksAddressType.UNKNOWN) {
             assertTrue(embedder.readInbound() instanceof UnknownSocksResponse);
         } else {
-            msg = embedder.readInbound();
+            msg = (SocksResponse) embedder.readInbound();
             assertEquals(((SocksCmdResponse) msg).cmdStatus(), cmdStatus);
+            if (host != null) {
+                assertEquals(((SocksCmdResponse) msg).host(), host);
+            }
+            assertEquals(((SocksCmdResponse) msg).port(), port);
         }
         assertNull(embedder.readInbound());
     }
 
+    /**
+     * Verifies that sent socks messages are decoded correctly.
+     */
     @Test
     public void testSocksCmdResponseDecoder() {
-        for (SocksCmdStatus cmdStatus: SocksCmdStatus.values()) {
-            for (SocksAddressType addressType: SocksAddressType.values()) {
-                testSocksCmdResponseDecoderWithDifferentParams(cmdStatus, addressType);
+        for (SocksCmdStatus cmdStatus : SocksCmdStatus.values()) {
+            for (SocksAddressType addressType : SocksAddressType.values()) {
+                testSocksCmdResponseDecoderWithDifferentParams(cmdStatus, addressType, null, 0);
             }
         }
     }
+
+    /**
+     * Verifies that invalid bound host will fail with IllegalArgumentException during encoding.
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void testInvalidAddress() {
+        testSocksCmdResponseDecoderWithDifferentParams(SocksCmdStatus.SUCCESS, SocksAddressType.IPv4, "1", 80);
+    }
+
+    /**
+     * Verifies that send socks messages are decoded correctly when bound host and port are set.
+     */
+    @Test
+    public void testSocksCmdResponseDecoderIncludingHost() {
+        for (SocksCmdStatus cmdStatus : SocksCmdStatus.values()) {
+            testSocksCmdResponseDecoderWithDifferentParams(cmdStatus, SocksAddressType.IPv4,
+                    "127.0.0.1", 80);
+            testSocksCmdResponseDecoderWithDifferentParams(cmdStatus, SocksAddressType.DOMAIN,
+                    "testDomain.com", 80);
+            testSocksCmdResponseDecoderWithDifferentParams(cmdStatus, SocksAddressType.IPv6,
+                    "2001:db8:85a3:42:1000:8a2e:370:7334", 80);
+        }
+    }
+
 }
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 319db95f14e..f8d69f138f4 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
@@ -15,6 +15,8 @@
  */
 package io.netty.handler.codec.socks;
 
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
@@ -33,4 +35,92 @@ public void testConstructorParamsAreNotNull() {
             assertTrue(e instanceof NullPointerException);
         }
     }
+
+    /**
+     * Verifies content of the response when domain is not specified.
+     */
+    @Test
+    public void testEmptyDomain() {
+        SocksCmdResponse socksCmdResponse = new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.DOMAIN);
+        assertNull(socksCmdResponse.host());
+        assertEquals(0, socksCmdResponse.port());
+        ByteBuf buffer = Unpooled.buffer(20);
+        socksCmdResponse.encodeAsByteBuf(buffer);
+        byte[] expected = {
+                0x05, // version
+                0x00, // success reply
+                0x00, // reserved
+                0x03, // address type domain
+                0x01, // length of domain
+                0x00, // domain value
+                0x00, // port value
+                0x00
+        };
+        assertByteBufEquals(expected, buffer);
+    }
+
+    /**
+     * Verifies content of the response when IPv4 address is specified.
+     */
+    @Test
+    public void testIPv4Host() {
+        SocksCmdResponse socksCmdResponse = new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.IPv4,
+                "127.0.0.1", 80);
+        assertEquals("127.0.0.1", socksCmdResponse.host());
+        assertEquals(80, socksCmdResponse.port());
+        ByteBuf buffer = Unpooled.buffer(20);
+        socksCmdResponse.encodeAsByteBuf(buffer);
+        byte[] expected = {
+                0x05, // version
+                0x00, // success reply
+                0x00, // reserved
+                0x01, // address type IPv4
+                0x7F, // address 127.0.0.1
+                0x00,
+                0x00,
+                0x01,
+                0x00, // port
+                0x50
+                };
+        assertByteBufEquals(expected, buffer);
+    }
+
+    /**
+     * Verifies that empty domain is allowed Response.
+     */
+    @Test
+    public void testEmptyBoundAddress() {
+        SocksCmdResponse socksCmdResponse = new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.DOMAIN,
+                "", 80);
+        assertEquals("", socksCmdResponse.host());
+        assertEquals(80, socksCmdResponse.port());
+        ByteBuf buffer = Unpooled.buffer(20);
+        socksCmdResponse.encodeAsByteBuf(buffer);
+        byte[] expected = {
+                0x05, // version
+                0x00, // success reply
+                0x00, // reserved
+                0x03, // address type domain
+                0x00, // domain length
+                0x00, // port
+                0x50
+        };
+        assertByteBufEquals(expected, buffer);
+    }
+
+    /**
+     * Verifies that Response cannot be constructed with invalid IP.
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void testInvalidBoundAddress() {
+        new SocksCmdResponse(SocksCmdStatus.SUCCESS, SocksAddressType.IPv4, "127.0.0", 1000);
+    }
+
+    private static void assertByteBufEquals(byte[] expected, ByteBuf actual) {
+        byte[] actualBytes = new byte[actual.readableBytes()];
+        actual.readBytes(actualBytes);
+        assertEquals("Generated response has incorrect length", expected.length, actualBytes.length);
+        assertArrayEquals("Generated response differs from expected", expected, actualBytes);
+    }
+
 }