Merge remote-tracking branch 'tor-github/pr/556'

changes/ticket27620

@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Rework SOCKS wire format handling to rely on trunnel-generated
+      parsing/generation code. Resolves ticket 27620.

src/tools/include.am

@@ -5,9 +5,11 @@ noinst_PROGRAMS+= src/tools/tor-cov-resolve
 endif
 
 src_tools_tor_resolve_SOURCES = src/tools/tor-resolve.c
-src_tools_tor_resolve_LDFLAGS =
+src_tools_tor_resolve_LDFLAGS = @TOR_LDFLAGS_openssl@
 src_tools_tor_resolve_LDADD = \
+	src/trunnel/libor-trunnel.a \
 	$(TOR_UTIL_LIBS) \
+        $(TOR_CRYPTO_LIBS) $(TOR_LIBS_CRYPTLIB)\
 	$(rust_ldadd) \
 	@TOR_LIB_MATH@ @TOR_LIB_WS32@ @TOR_LIB_IPHLPAPI@ @TOR_LIB_USERENV@
 
@@ -15,8 +17,11 @@ if COVERAGE_ENABLED
 src_tools_tor_cov_resolve_SOURCES = src/tools/tor-resolve.c
 src_tools_tor_cov_resolve_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS)
 src_tools_tor_cov_resolve_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS)
+src_tools_tor_cov_resolve_LDFLAGS = @TOR_LDFLAGS_openssl@
 src_tools_tor_cov_resolve_LDADD = \
+	src/trunnel/libor-trunnel.a \
 	$(TOR_UTIL_TESTING_LIBS) \
+	$(TOR_CRYPTO_TESTING_LIBS) $(TOR_LIBS_CRYPTLIB) \
 	@TOR_LIB_MATH@ @TOR_LIB_WS32@
 endif
 

src/tools/tor-resolve.c

@@ -15,6 +15,7 @@
 #include "lib/string/util_string.h"
 
 #include "lib/net/socks5_status.h"
+#include "trunnel/socks5.h"
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -49,68 +50,204 @@
 
 static void usage(void) ATTR_NORETURN;
 
+/**
+ * Set <b>out</b> to a pointer to newly allocated buffer containing
+ * SOCKS4a RESOLVE request with <b>username</b> and <b>hostname</b>.
+ * Return number of bytes in the buffer if succeeded or -1 if failed.
+ */
+static ssize_t
+build_socks4a_resolve_request(uint8_t **out,
+                              const char *username,
+                              const char *hostname)
+{
+  tor_assert(out);
+  tor_assert(username);
+  tor_assert(hostname);
+
+  const char *errmsg = NULL;
+  uint8_t *output = NULL;
+  socks4_client_request_t *rq = socks4_client_request_new();
+
+  socks4_client_request_set_version(rq, 4);
+  socks4_client_request_set_command(rq, CMD_RESOLVE);
+  socks4_client_request_set_port(rq, 0);
+  socks4_client_request_set_addr(rq, 0x00000001u);
+  socks4_client_request_set_username(rq, username);
+  socks4_client_request_set_socks4a_addr_hostname(rq, hostname);
+
+  errmsg = socks4_client_request_check(rq);
+  if (errmsg) {
+    goto cleanup;
+  }
+
+  ssize_t encoded_len = socks4_client_request_encoded_len(rq);
+  if (encoded_len <= 0) {
+    errmsg = "socks4_client_request_encoded_len failed";
+    goto cleanup;
+  }
+
+  output = tor_malloc(encoded_len);
+  memset(output, 0, encoded_len);
+
+  encoded_len = socks4_client_request_encode(output, encoded_len, rq);
+  if (encoded_len <= 0) {
+    errmsg = "socks4_client_request_encode failed";
+    goto cleanup;
+  }
+
+  *out = output;
+
+ cleanup:
+  socks4_client_request_free(rq);
+  if (errmsg) {
+    log_err(LD_NET, "build_socks4a_resolve_request failed: %s", errmsg);
+    *out = NULL;
+    tor_free(output);
+  }
+  return errmsg ? -1 : encoded_len;
+}
+
+#define SOCKS5_ATYPE_HOSTNAME 0x03
+#define SOCKS5_ATYPE_IPV4     0x01
+#define SOCKS5_ATYPE_IPV6     0x04
+
+/**
+ * Set <b>out</b> to pointer to newly allocated buffer containing
+ * SOCKS5 RESOLVE/RESOLVE_PTR request with given <b>hostname<b>.
+ * Generate a reverse request if <b>reverse</b> is true.
+ * Return the number of bytes in the buffer if succeeded or -1 if failed.
+ */
+static ssize_t
+build_socks5_resolve_request(uint8_t **out,
+                             const char *hostname,
+                             int reverse)
+{
+  const char *errmsg = NULL;
+  uint8_t *outbuf = NULL;
+  int is_ip_address;
+  tor_addr_t addr;
+  size_t addrlen;
+  int ipv6;
+  is_ip_address = tor_addr_parse(&addr, hostname) != -1;
+  if (!is_ip_address && reverse) {
+    log_err(LD_GENERAL, "Tried to do a reverse lookup on a non-IP!");
+    return -1;
+  }
+  ipv6 = reverse && tor_addr_family(&addr) == AF_INET6;
+  addrlen = reverse ? (ipv6 ? 16 : 4) : 1 + strlen(hostname);
+  if (addrlen > UINT8_MAX) {
+    log_err(LD_GENERAL, "Hostname is too long!");
+    return -1;
+  }
+
+  socks5_client_request_t *rq = socks5_client_request_new();
+
+  socks5_client_request_set_version(rq, 5);
+  /* RESOLVE_PTR or RESOLVE */
+  socks5_client_request_set_command(rq, reverse ? CMD_RESOLVE_PTR :
+                                                  CMD_RESOLVE);
+  socks5_client_request_set_reserved(rq, 0);
+
+  uint8_t atype = SOCKS5_ATYPE_HOSTNAME;
+  if (reverse)
+    atype = ipv6 ? SOCKS5_ATYPE_IPV6 : SOCKS5_ATYPE_IPV4;
+
+  socks5_client_request_set_atype(rq, atype);
+
+  switch (atype) {
+    case SOCKS5_ATYPE_IPV4: {
+      socks5_client_request_set_dest_addr_ipv4(rq,
+                                        tor_addr_to_ipv4h(&addr));
+    } break;
+    case SOCKS5_ATYPE_IPV6: {
+      uint8_t *ipv6_array =
+        socks5_client_request_getarray_dest_addr_ipv6(rq);
+
+      tor_assert(ipv6_array);
+
+      memcpy(ipv6_array, tor_addr_to_in6_addr8(&addr), 16);
+    } break;
+
+    case SOCKS5_ATYPE_HOSTNAME: {
+      domainname_t *dn = domainname_new();
+      domainname_set_len(dn, addrlen - 1);
+      domainname_setlen_name(dn, addrlen - 1);
+      char *dn_buf = domainname_getarray_name(dn);
+      memcpy(dn_buf, hostname, addrlen - 1);
+
+      errmsg = domainname_check(dn);
+
+      if (errmsg) {
+        domainname_free(dn);
+        goto cleanup;
+      } else {
+        socks5_client_request_set_dest_addr_domainname(rq, dn);
+      }
+    } break;
+    default:
+      tor_assert_unreached();
+      break;
+  }
+
+  socks5_client_request_set_dest_port(rq, 0);
+
+  errmsg = socks5_client_request_check(rq);
+  if (errmsg) {
+    goto cleanup;
+  }
+
+  ssize_t encoded_len = socks5_client_request_encoded_len(rq);
+  if (encoded_len < 0) {
+    errmsg = "Cannot predict encoded length";
+    goto cleanup;
+  }
+
+  outbuf = tor_malloc(encoded_len);
+  memset(outbuf, 0, encoded_len);
+
+  encoded_len = socks5_client_request_encode(outbuf, encoded_len, rq);
+  if (encoded_len < 0) {
+    errmsg = "encoding failed";
+    goto cleanup;
+  }
+
+  *out = outbuf;
+
+ cleanup:
+  socks5_client_request_free(rq);
+  if (errmsg) {
+    tor_free(outbuf);
+    log_err(LD_NET, "build_socks5_resolve_request failed with error: %s",
+            errmsg);
+  }
+
+  return errmsg ? -1 : encoded_len;
+}
+
 /** Set *<b>out</b> to a newly allocated SOCKS4a resolve request with
  * <b>username</b> and <b>hostname</b> as provided.  Return the number
  * of bytes in the request. */
 static ssize_t
-build_socks_resolve_request(char **out,
+build_socks_resolve_request(uint8_t **out,
                             const char *username,
                             const char *hostname,
                             int reverse,
                             int version)
 {
-  size_t len = 0;
   tor_assert(out);
   tor_assert(username);
   tor_assert(hostname);
 
+  tor_assert(version == 4 || version == 5);
+
   if (version == 4) {
-    len = 8 + strlen(username) + 1 + strlen(hostname) + 1;
-    *out = tor_malloc(len);
-    (*out)[0] = 4;      /* SOCKS version 4 */
-    (*out)[1] = '\xF0'; /* Command: resolve. */
-    set_uint16((*out)+2, htons(0)); /* port: 0. */
-    set_uint32((*out)+4, htonl(0x00000001u)); /* addr: 0.0.0.1 */
-    memcpy((*out)+8, username, strlen(username)+1);
-    memcpy((*out)+8+strlen(username)+1, hostname, strlen(hostname)+1);
+    return build_socks4a_resolve_request(out, username, hostname);
   } else if (version == 5) {
-    int is_ip_address;
-    tor_addr_t addr;
-    size_t addrlen;
-    int ipv6;
-    is_ip_address = tor_addr_parse(&addr, hostname) != -1;
-    if (!is_ip_address && reverse) {
-      log_err(LD_GENERAL, "Tried to do a reverse lookup on a non-IP!");
-      return -1;
-    }
-    ipv6 = reverse && tor_addr_family(&addr) == AF_INET6;
-    addrlen = reverse ? (ipv6 ? 16 : 4) : 1 + strlen(hostname);
-    if (addrlen > UINT8_MAX) {
-      log_err(LD_GENERAL, "Hostname is too long!");
-      return -1;
-    }
-    len = 6 + addrlen;
-    *out = tor_malloc(len);
-    (*out)[0] = 5; /* SOCKS version 5 */
-    (*out)[1] = reverse ? '\xF1' : '\xF0'; /* RESOLVE_PTR or RESOLVE */
-    (*out)[2] = 0; /* reserved. */
-    if (reverse) {
-      (*out)[3] = ipv6 ? 4 : 1;
-      if (ipv6)
-        memcpy((*out)+4, tor_addr_to_in6_addr8(&addr), 16);
-      else
-        set_uint32((*out)+4, tor_addr_to_ipv4n(&addr));
-    } else {
-      (*out)[3] = 3;
-      (*out)[4] = (char)(uint8_t)(addrlen - 1);
-      memcpy((*out)+5, hostname, addrlen - 1);
-    }
-    set_uint16((*out)+4+addrlen, 0); /* port */
-  } else {
-    tor_assert(0);
+    return build_socks5_resolve_request(out, hostname, reverse);
   }
 
-  return len;
+  tor_assert_unreached();
+  return -1;
 }
 
 static void
@@ -134,34 +271,50 @@ parse_socks4a_resolve_response(const char *hostname,
                                const char *response, size_t len,
                                tor_addr_t *addr_out)
 {
+  int result = 0;
   uint8_t status;
   tor_assert(response);
   tor_assert(addr_out);
 
-  if (len < RESPONSE_LEN_4) {
+  socks4_server_reply_t *reply;
+
+  ssize_t parsed = socks4_server_reply_parse(&reply,
+                                             (const uint8_t *)response,
+                                             len);
+
+  if (parsed == -1) {
+    log_warn(LD_PROTOCOL, "Failed parsing SOCKS4a response");
+    result = -1; goto cleanup;
+  }
+
+  if (parsed == -2) {
     log_warn(LD_PROTOCOL,"Truncated socks response.");
-    return -1;
+    result = -1; goto cleanup;
   }
-  if (((uint8_t)response[0])!=0) { /* version: 0 */
+
+  if (socks4_server_reply_get_version(reply) != 0) { /* version: 0 */
     log_warn(LD_PROTOCOL,"Nonzero version in socks response: bad format.");
-    return -1;
+    result = -1; goto cleanup;
   }
-  status = (uint8_t)response[1];
-  if (get_uint16(response+2)!=0) { /* port: 0 */
+  if (socks4_server_reply_get_port(reply) != 0) { /* port: 0 */
     log_warn(LD_PROTOCOL,"Nonzero port in socks response: bad format.");
-    return -1;
+    result = -1; goto cleanup;
   }
+  status = socks4_server_reply_get_status(reply);
   if (status != 90) {
     log_warn(LD_NET,"Got status response '%d': socks request failed.", status);
     if (!strcasecmpend(hostname, ".onion")) {
       onion_warning(hostname);
-      return -1;
+      result = -1; goto cleanup;
     }
-    return -1;
+    result = -1; goto cleanup;
   }
 
-  tor_addr_from_ipv4n(addr_out, get_uint32(response+4));
-  return 0;
+  tor_addr_from_ipv4h(addr_out, socks4_server_reply_get_addr(reply));
+
+ cleanup:
+  socks4_server_reply_free(reply);
+  return result;
 }
 
 /* It would be nice to let someone know what SOCKS5 issue a user may have */
@@ -205,7 +358,7 @@ do_resolve(const char *hostname,
   int s = -1;
   struct sockaddr_storage ss;
   socklen_t socklen;
-  char *req = NULL;
+  uint8_t *req = NULL;
   ssize_t len = 0;
 
   tor_assert(hostname);
@@ -230,23 +383,57 @@ do_resolve(const char *hostname,
   }
 
   if (version == 5) {
-    char method_buf[2];
-    if (write_all_to_socket(s, "\x05\x01\x00", 3) != 3) {
+    socks5_client_version_t *v = socks5_client_version_new();
+
+    socks5_client_version_set_version(v, 5);
+    socks5_client_version_set_n_methods(v, 1);
+    socks5_client_version_setlen_methods(v, 1);
+    socks5_client_version_set_methods(v, 0, 0x00);
+
+    tor_assert(!socks5_client_version_check(v));
+    ssize_t encoded_len = socks5_client_version_encoded_len(v);
+    tor_assert(encoded_len > 0);
+
+    uint8_t *buf = tor_malloc(encoded_len);
+    encoded_len = socks5_client_version_encode(buf, encoded_len, v);
+    tor_assert(encoded_len > 0);
+
+    socks5_client_version_free(v);
+
+    if (write_all_to_socket(s, (const char *)buf,
+                            encoded_len) != encoded_len) {
       log_err(LD_NET, "Error sending SOCKS5 method list.");
+      tor_free(buf);
+
       goto err;
     }
-    if (read_all_from_socket(s, method_buf, 2) != 2) {
+
+    tor_free(buf);
+
+    uint8_t method_buf[2];
+
+    if (read_all_from_socket(s, (char *)method_buf, 2) != 2) {
       log_err(LD_NET, "Error reading SOCKS5 methods.");
       goto err;
     }
-    if (method_buf[0] != '\x05') {
-      log_err(LD_NET, "Unrecognized socks version: %u",
-              (unsigned)method_buf[0]);
+
+    socks5_server_method_t *m;
+    ssize_t parsed = socks5_server_method_parse(&m, method_buf,
+                                                sizeof(method_buf));
+
+    if (parsed < 2) {
+      log_err(LD_NET, "Failed to parse SOCKS5 method selection "
+                      "message");
       goto err;
     }
-    if (method_buf[1] != '\x00') {
+
+    uint8_t method = socks5_server_method_get_method(m);
+
+    socks5_server_method_free(m);
+
+    if (method != 0x00) {
       log_err(LD_NET, "Unrecognized socks authentication method: %u",
-              (unsigned)method_buf[1]);
+              (unsigned)method);
       goto err;
     }
   }
@@ -257,7 +444,7 @@ do_resolve(const char *hostname,
     tor_assert(!req);
     goto err;
   }
-  if (write_all_to_socket(s, req, len) != len) {
+  if (write_all_to_socket(s, (const char *)req, len) != len) {
     log_sock_error("sending SOCKS request", s);
     tor_free(req);
     goto err;
@@ -276,55 +463,66 @@ do_resolve(const char *hostname,
       goto err;
     }
   } else {
-    char reply_buf[16];
-    if (read_all_from_socket(s, reply_buf, 4) != 4) {
-      log_err(LD_NET, "Error reading SOCKS5 response.");
+    uint8_t reply_buf[512];
+
+    len = read_all_from_socket(s, (char *)reply_buf,
+                               sizeof(reply_buf));
+
+    socks5_server_reply_t *reply;
+
+    ssize_t parsed = socks5_server_reply_parse(&reply,
+                                               reply_buf,
+                                               len);
+    if (parsed == -1) {
+      log_err(LD_NET, "Failed parsing SOCKS5 response");
       goto err;
     }
-    if (reply_buf[0] != 5) {
-      log_err(LD_NET, "Bad SOCKS5 reply version.");
+
+    if (parsed == -2) {
+      log_err(LD_NET, "Truncated SOCKS5 response");
       goto err;
     }
+
     /* Give a user some useful feedback about SOCKS5 errors */
-    if (reply_buf[1] != 0) {
+    uint8_t reply_field = socks5_server_reply_get_reply(reply);
+    if (reply_field != 0) {
       log_warn(LD_NET,"Got SOCKS5 status response '%u': %s",
-               (unsigned)reply_buf[1],
-               socks5_reason_to_string(reply_buf[1]));
-      if (reply_buf[1] == 4 && !strcasecmpend(hostname, ".onion")) {
+               (unsigned)reply_field,
+               socks5_reason_to_string(reply_field));
+      if (reply_field == 4 && !strcasecmpend(hostname, ".onion")) {
         onion_warning(hostname);
       }
+
+      socks5_server_reply_free(reply);
       goto err;
     }
-    if (reply_buf[3] == 1) {
+
+    uint8_t atype = socks5_server_reply_get_atype(reply);
+
+    if (atype == SOCKS5_ATYPE_IPV4) {
       /* IPv4 address */
-      if (read_all_from_socket(s, reply_buf, 4) != 4) {
-        log_err(LD_NET, "Error reading address in socks5 response.");
-        goto err;
-      }
-      tor_addr_from_ipv4n(result_addr, get_uint32(reply_buf));
-    } else if (reply_buf[3] == 4) {
+      tor_addr_from_ipv4h(result_addr,
+        socks5_server_reply_get_bind_addr_ipv4(reply));
+    } else if (atype == SOCKS5_ATYPE_IPV6) {
       /* IPv6 address */
-      if (read_all_from_socket(s, reply_buf, 16) != 16) {
-        log_err(LD_NET, "Error reading address in socks5 response.");
-        goto err;
-      }
-      tor_addr_from_ipv6_bytes(result_addr, reply_buf);
-    } else if (reply_buf[3] == 3) {
+      tor_addr_from_ipv6_bytes(result_addr,
+        (const char *)socks5_server_reply_getarray_bind_addr_ipv6(reply));
+    } else if (atype == SOCKS5_ATYPE_HOSTNAME) {
       /* Domain name */
-      size_t result_len;
-      if (read_all_from_socket(s, reply_buf, 1) != 1) {
-        log_err(LD_NET, "Error reading address_length in socks5 response.");
-        goto err;
-      }
-      result_len = *(uint8_t*)(reply_buf);
+      domainname_t *dn =
+        socks5_server_reply_get_bind_addr_domainname(reply);
+
+      size_t result_len = (size_t)domainname_get_len(dn);
+
       *result_hostname = tor_malloc(result_len+1);
-      if (read_all_from_socket(s, *result_hostname, result_len)
-          != (int) result_len) {
-        log_err(LD_NET, "Error reading hostname in socks5 response.");
-        goto err;
-      }
+
+      strncpy(*result_hostname, (char *)domainname_getstr_name(dn),
+              result_len);
+
       (*result_hostname)[result_len] = '\0';
     }
+
+    socks5_server_reply_free(reply);
   }
 
   tor_close_socket(s);

src/trunnel/socks5.c

@@ -1694,7 +1694,6 @@ socks4_server_reply_new(void)
   socks4_server_reply_t *val = trunnel_calloc(1, sizeof(socks4_server_reply_t));
   if (NULL == val)
     return NULL;
-  val->version = 4;
   return val;
 }
 
@@ -1724,7 +1723,7 @@ socks4_server_reply_get_version(const socks4_server_reply_t *inp)
 int
 socks4_server_reply_set_version(socks4_server_reply_t *inp, uint8_t val)
 {
-  if (! ((val == 4))) {
+  if (! ((val == 0 || val == 4))) {
      TRUNNEL_SET_ERROR_CODE(inp);
      return -1;
   }
@@ -1771,7 +1770,7 @@ socks4_server_reply_check(const socks4_server_reply_t *obj)
     return "Object was NULL";
   if (obj->trunnel_error_code_)
     return "A set function failed on this object";
-  if (! (obj->version == 4))
+  if (! (obj->version == 0 || obj->version == 4))
     return "Integer out of bounds";
   return NULL;
 }
@@ -1785,7 +1784,7 @@ socks4_server_reply_encoded_len(const socks4_server_reply_t *obj)
      return -1;
 
 
-  /* Length of u8 version IN [4] */
+  /* Length of u8 version IN [0, 4] */
   result += 1;
 
   /* Length of u8 status */
@@ -1823,7 +1822,7 @@ socks4_server_reply_encode(uint8_t *output, const size_t avail, const socks4_ser
   trunnel_assert(encoded_len >= 0);
 #endif
 
-  /* Encode u8 version IN [4] */
+  /* Encode u8 version IN [0, 4] */
   trunnel_assert(written <= avail);
   if (avail - written < 1)
     goto truncated;
@@ -1886,11 +1885,11 @@ socks4_server_reply_parse_into(socks4_server_reply_t *obj, const uint8_t *input,
   ssize_t result = 0;
   (void)result;
 
-  /* Parse u8 version IN [4] */
+  /* Parse u8 version IN [0, 4] */
   CHECK_REMAINING(1, truncated);
   obj->version = (trunnel_get_uint8(ptr));
   remaining -= 1; ptr += 1;
-  if (! (obj->version == 4))
+  if (! (obj->version == 0 || obj->version == 4))
     goto fail;
 
   /* Parse u8 status */

src/trunnel/socks5.trunnel

@@ -86,7 +86,7 @@ struct socks4_client_request {
 }
 
 struct socks4_server_reply {
-   u8 version IN [4];
+   u8 version IN [0,4];
    u8 status;
    u16 port;
    u32 addr;