ncat corrupts SOCKS5 auth with long credentials

[nnposter]

Ncat is using a fixed size buffer to send username and password to SOCKS5 proxy. It does validate that the credentials fit into the buffer. What it does not do is the check that the length of each of the two fields fits within a single byte, as prescribed by RFC 1929. This omission results in a corrupted authentication request sent to the proxy.

The patch below refactors the SOCKS5 authentication code to rectify the issue. As a side effect it also:

• adds support for zero-length passwords
• allows passwords to contain colons
• fixes Makes sure that proxy_auth is properly disposed  #975.

Please let me know if you have any questions or concerns. Otherwise I will commit the patch in a few weeks.

--- a/ncat/ncat.h
+++ b/ncat/ncat.h
@@ -177,13 +177,13 @@
 
 struct socks5_connect {
     char ver;
-    char nmethods;
+    unsigned char nmethods;
     char methods[3];
 } __attribute__((packed));
 
 struct socks5_auth {
-  char ver; // must be always 1
-  char data[SOCKS_BUFF_SIZE];
+    char ver; // must be always 1
+    unsigned char data[SOCKS_BUFF_SIZE];
 } __attribute__((packed));
 
 struct socks5_request {
@@ -263,6 +263,12 @@
 #define SOCKS5_ATYP_NAME        3
 #define SOCKS5_ATYP_IPv6        4
 
+#define SOCKS5_USR_MAXLEN       255
+#define SOCKS5_PWD_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
 
 /* Length of IPv6 address */
 #ifndef INET6_ADDRSTRLEN
--- a/ncat/ncat_connect.c
+++ b/ncat/ncat_connect.c
@@ -664,9 +664,8 @@
     int sd,len,lenfqdn;
     struct socks5_request socks5msg2;
     struct socks5_auth socks5auth;
-    char *proxy_auth;
-    char *username;
-    char *password;
+    char *uptr, *pptr;
+    size_t authlen, ulen, plen;
 
     sd = do_connect(SOCK_STREAM);
     if (sd == -1) {
@@ -683,17 +682,13 @@
 
     zmem(&socks5msg,sizeof(socks5msg));
     socks5msg.ver = SOCKS5_VERSION;
-    socks5msg.nmethods = 1;
-    socks5msg.methods[0] = SOCKS5_AUTH_NONE;
-    len = 3;
+    socks5msg.nmethods = 0;
+    socks5msg.methods[socks5msg.nmethods++] = SOCKS5_AUTH_NONE;
 
-    if (o.proxy_auth){
-        socks5msg.nmethods ++;
-        socks5msg.methods[1] = SOCKS5_AUTH_USERPASS;
-        len ++;
-    }
+    if (o.proxy_auth)
+        socks5msg.methods[socks5msg.nmethods++] = SOCKS5_AUTH_USERPASS;
 
-    if (send(sd, (char *) &socks5msg, len, 0) < 0) {
+    if (send(sd, (char *)&socks5msg, offsetof(struct socks5_connect, methods) + socks5msg.nmethods, 0) < 0) {
         loguser("Error: proxy request: %s.\n", socket_strerror(socket_errno()));
         close(sd);
         return -1;
@@ -706,46 +701,47 @@
         return -1;
     }
 
-    if (socksbuf[0] != 5){
+    if (socksbuf[0] != SOCKS5_VERSION) {
         loguser("Error: got wrong server version in response.\n");
         close(sd);
         return -1;
     }
 
-    switch(socksbuf[1]) {
+    switch((unsigned char)socksbuf[1]) {
         case SOCKS5_AUTH_NONE:
             if (o.verbose)
                 loguser("No authentication needed.\n");
             break;
 
-        case SOCKS5_AUTH_GSSAPI:
-            loguser("GSSAPI authentication method not supported.\n");
-            close(sd);
-            return -1;
-
         case SOCKS5_AUTH_USERPASS:
             if (o.verbose)
                 loguser("Doing username and password authentication.\n");
 
             if(!o.proxy_auth){
-                loguser("Error: proxy requested to do authentication, but no credentials were provided.\n");
+                /* Proxy must not select a method not offered by the client */
+                loguser("Error: proxy selected invalid authentication method.\n");
                 close(sd);
                 return -1;
             }
 
-            if (strlen(o.proxy_auth) > SOCKS_BUFF_SIZE-2){
-                loguser("Error: username and password are too long to fit into buffer.\n");
+            /* Split up the proxy auth argument. */
+            uptr = o.proxy_auth;
+            pptr = strchr(o.proxy_auth, ':');
+            if (pptr == NULL) {
+                loguser("Error: invalid username:password combo.\n");
                 close(sd);
                 return -1;
             }
 
-            /* Split up the proxy auth argument. */
-            proxy_auth = Strdup(o.proxy_auth);
-            username = strtok(proxy_auth, ":");
-            password = strtok(NULL, ":");
-            if (password == NULL || username == NULL) {
-                free(proxy_auth);
-                loguser("Error: empty username or password.\n");
+            ulen = (pptr++) - uptr;
+            plen = strlen(pptr);
+            if (ulen > SOCKS5_USR_MAXLEN) {
+                loguser("Error: username length exceeds %d.\n", SOCKS5_USR_MAXLEN);
+                close(sd);
+                return -1;
+            }
+            if (plen > SOCKS5_PWD_MAXLEN) {
+                loguser("Error: password length exceeds %d.\n", SOCKS5_PWD_MAXLEN);
                 close(sd);
                 return -1;
             }
@@ -766,15 +762,16 @@
              */
 
             socks5auth.ver = 1;
-            socks5auth.data[0] = strlen(username);
-            memcpy(socks5auth.data+1,username,strlen(username));
-            len = 2 + strlen(username); // (version + strlen) + username
+            authlen = 0;
+            socks5auth.data[authlen++] = ulen;
+            memcpy(socks5auth.data + authlen, uptr, ulen);
+            authlen += ulen;
 
-            socks5auth.data[len-1]=strlen(password);
-            memcpy(socks5auth.data+len,password,strlen(password));
-            len += 1 + strlen(password);
+            socks5auth.data[authlen++] = plen;
+            memcpy(socks5auth.data + authlen, pptr, plen);
+            authlen += plen;
 
-            if (send(sd, (char *) &socks5auth, len, 0) < 0) {
+            if (send(sd, (char *) &socks5auth, offsetof(struct socks5_auth, data) + authlen, 0) < 0) {
                 loguser("Error: sending proxy authentication.\n");
                 close(sd);
                 return -1;
@@ -794,8 +791,14 @@
 
             break;
 
+        case SOCKS5_AUTH_FAILED:
+            loguser("Error: no acceptable authentication method proposed.\n");
+            close(sd);
+            return -1;
+
         default:
-            loguser("Error - can't choose any authentication method.\n");
+            /* Proxy must not select a method not offered by the client */
+            loguser("Error: proxy selected invalid authentication method.\n");
             close(sd);
             return -1;
     }
@@ -832,6 +835,10 @@
             socks5msg2.dst[0]=lenfqdn;
             memcpy(socks5msg2.dst+1,o.target,lenfqdn);
             len = 1 + lenfqdn;
+            break;
+
+        default: // this shall not happen
+            ncat_assert(0);
     }
 
     memcpy(socks5msg2.dst+len, &proxyport, sizeof(proxyport));

[dmiller-nmap]

Very cool. It all looks good, though I have the same hangup in reading it as I did in #984: using postincrement on a pointer is just confusing to me; I much prefer to increment the pointer in a separate statement, so that it is clear which value is being used when. I don't know why I have this objection when stuff like postincrement of an array index, as you did with auth methods, strikes me more as an elegant solution. Anyway, this is all just too many words to say: ship it!