Rectifies SOCKS5 protocol field overrun hazards

jumpnow · Jan 19, 2019 · cb9559b · cb9559b
1 parent 6bb36b1
commit cb9559b
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 20 deletions.
diff --git a/ncat/ncat.h b/ncat/ncat.h
@@ -191,7 +191,7 @@ struct socks5_request {
     char cmd;
     char rsv;
     char atyp;
-    char dst[SOCKS_BUFF_SIZE]; // addr/name and port info
+    unsigned char dst[SOCKS_BUFF_SIZE]; // addr/name and port info
 } __attribute__((packed));
 #ifdef WIN32
 #pragma pack()
@@ -265,11 +265,16 @@ struct socks5_request {
 
 #define SOCKS5_USR_MAXLEN       255
 #define SOCKS5_PWD_MAXLEN       255
+#define SOCKS5_DST_MAXLEN       255
 
 #if SOCKS_BUFF_SIZE < (1 + SOCKS5_USR_MAXLEN) + (1 + SOCKS5_PWD_MAXLEN)
 #error SOCKS_BUFF_SIZE is defined too small to handle SOCKS5 authentication
 #endif
 
+#if SOCKS_BUFF_SIZE < (1 + SOCKS5_DST_MAXLEN) + 2
+#error SOCKS_BUFF_SIZE is defined too small to handle SOCKS5 destination
+#endif
+
 /* Length of IPv6 address */
 #ifndef INET6_ADDRSTRLEN
 #define INET6_ADDRSTRLEN 46

diff --git a/ncat/ncat_connect.c b/ncat/ncat_connect.c
@@ -661,9 +661,10 @@ static int do_proxy_socks5(void)
     struct socks5_connect socks5msg;
     uint32_t inetaddr;
     char inet6addr[16];
-    unsigned short proxyport = htons(o.portno);
+    uint16_t proxyport = htons(o.portno);
     char socksbuf[8];
-    int sd,len,lenfqdn;
+    int sd;
+    size_t dstlen, targetlen;
     struct socks5_request socks5msg2;
     struct socks5_auth socks5auth;
     char *uptr, *pptr;
@@ -816,43 +817,38 @@ static int do_proxy_socks5(void)
             socks5msg2.atyp = SOCKS5_ATYP_IPv4;
             inetaddr = inet_addr(o.target);
             memcpy(socks5msg2.dst, &inetaddr, 4);
-            len = 4;
+            dstlen = 4;
             break;
 
         case 2: // IPv6 address family
             socks5msg2.atyp = SOCKS5_ATYP_IPv6;
             inet_pton(AF_INET6,o.target,&inet6addr);
             memcpy(socks5msg2.dst, inet6addr,16);
-            len = 16;
+            dstlen = 16;
             break;
 
         case -1: // FQDN
             socks5msg2.atyp = SOCKS5_ATYP_NAME;
-            lenfqdn=strlen(o.target);
-            if (lenfqdn > SOCKS_BUFF_SIZE-5){
-                loguser("Error: host name too long.\n");
+            targetlen=strlen(o.target);
+            if (targetlen > SOCKS5_DST_MAXLEN){
+                loguser("Error: hostname length exceeds %d.\n", SOCKS5_DST_MAXLEN);
                 close(sd);
                 return -1;
             }
-            socks5msg2.dst[0]=lenfqdn;
-            memcpy(socks5msg2.dst+1,o.target,lenfqdn);
-            len = 1 + lenfqdn;
+            dstlen = 0;
+            socks5msg2.dst[dstlen++] = targetlen;
+            memcpy(socks5msg2.dst + dstlen, o.target, targetlen);
+            dstlen += targetlen;
             break;
 
         default: // this shall not happen
             ncat_assert(0);
     }
 
-    memcpy(socks5msg2.dst+len, &proxyport, sizeof(proxyport));
-    len += 2 + 1 + 3;
+    memcpy(socks5msg2.dst + dstlen, &proxyport, 2);
+    dstlen += 2;
 
-    if (len > sizeof(socks5msg2)){
-        loguser("Error: address information too large.\n");
-        close(sd);
-        return -1;
-    }
-
-    if (send(sd, (char *) &socks5msg2, len, 0) < 0) {
+    if (send(sd, (char *) &socks5msg2, offsetof(struct socks5_request , dst) + dstlen, 0) < 0) {
         loguser("Error: sending proxy request: %s.\n", socket_strerror(socket_errno()));
         close(sd);
         return -1;