Merge branch 'bugs25036_25055_clean_033' into maint-0.3.3

6 files changed, 138 insertions, 40 deletions

diff --git a/changes/bugs_25036_25055 b/changes/bugs_25036_25055
new file mode 100644
index 0000000000..daa46321c0
--- /dev/null
+++ b/changes/bugs_25036_25055
@@ -0,0 +1,7 @@
+  o Minor bugfixes (networking):
+    - Tor will not reject IPv6 address strings from TorBrowser when they
+      are passed as hostnames in SOCKS5 requests. Fixes bug 25036,
+      bugfix on Tor 0.3.1.2.
+    - string_is_valid_hostname() will not consider IP strings to be valid
+      hostnames. Fixes bug 25055; bugfix on Tor 0.2.5.5.
+
diff --git a/src/common/util.c b/src/common/util.c
index 90204befc0..a68fd30d09 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -1071,6 +1071,36 @@ string_is_valid_ipv6_address(const char *string)
   return (tor_inet_pton(AF_INET6,string,&addr) == 1);
 }
 
+/** Return true iff <b>string</b> is a valid destination address,
+ * i.e. either a DNS hostname or IPv4/IPv6 address string.
+ */
+int
+string_is_valid_dest(const char *string)
+{
+  char *tmp = NULL;
+  int retval;
+  size_t len;
+
+  if (string == NULL)
+    return 0;
+
+  len = strlen(string);
+
+  if (len == 0)
+    return 0;
+
+  if (string[0] == '[' && string[len - 1] == ']')
+    string = tmp = tor_strndup(string + 1, len - 2);
+
+  retval = string_is_valid_ipv4_address(string) ||
+    string_is_valid_ipv6_address(string) ||
+    string_is_valid_nonrfc_hostname(string);
+
+  tor_free(tmp);
+
+  return retval;
+}
+
 /** Return true iff <b>string</b> matches a pattern of DNS names
  * that we allow Tor clients to connect to.
  *
@@ -1078,37 +1108,51 @@ string_is_valid_ipv6_address(const char *string)
  * with misconfigured zones that have been encountered in the wild.
  */
 int
-string_is_valid_hostname(const char *string)
+string_is_valid_nonrfc_hostname(const char *string)
 {
   int result = 1;
+  int has_trailing_dot;
+  char *last_label;
   smartlist_t *components;
 
+  if (!string || strlen(string) == 0)
+    return 0;
+
+  if (string_is_valid_ipv4_address(string))
+    return 0;
+
   components = smartlist_new();
 
   smartlist_split_string(components,string,".",0,0);
 
+  if (BUG(smartlist_len(components) == 0))
+    return 0; // LCOV_EXCL_LINE should be impossible given the earlier checks.
+
+  /* Allow a single terminating '.' used rarely to indicate domains
+   * are FQDNs rather than relative. */
+  last_label = (char *)smartlist_get(components,
+                                     smartlist_len(components) - 1);
+  has_trailing_dot = (last_label[0] == '\0');
+  if (has_trailing_dot) {
+    smartlist_pop_last(components);
+    tor_free(last_label);
+    last_label = NULL;
+  }
+
   SMARTLIST_FOREACH_BEGIN(components, char *, c) {
     if ((c[0] == '-') || (*c == '_')) {
       result = 0;
       break;
     }
 
-    /* Allow a single terminating '.' used rarely to indicate domains
-     * are FQDNs rather than relative. */
-    if ((c_sl_idx > 0) && (c_sl_idx + 1 == c_sl_len) && !*c) {
-      continue;
-    }
-
     do {
-      if ((*c >= 'a' && *c <= 'z') ||
-          (*c >= 'A' && *c <= 'Z') ||
-          (*c >= '0' && *c <= '9') ||
-          (*c == '-') || (*c == '_'))
-        c++;
-      else
-        result = 0;
+      result = (TOR_ISALNUM(*c) || (*c == '-') || (*c == '_'));
+      c++;
     } while (result && *c);
 
+    if (result == 0) {
+      break;
+    }
   } SMARTLIST_FOREACH_END(c);
 
   SMARTLIST_FOREACH_BEGIN(components, char *, c) {
diff --git a/src/common/util.h b/src/common/util.h
index 2ee0ea28cd..9380789128 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -233,7 +233,8 @@ const char *find_str_at_start_of_line(const char *haystack,
                                       const char *needle);
 int string_is_C_identifier(const char *string);
 int string_is_key_value(int severity, const char *string);
-int string_is_valid_hostname(const char *string);
+int string_is_valid_dest(const char *string);
+int string_is_valid_nonrfc_hostname(const char *string);
 int string_is_valid_ipv4_address(const char *string);
 int string_is_valid_ipv6_address(const char *string);
 
diff --git a/src/or/proto_socks.c b/src/or/proto_socks.c
index 91633d02af..8700fe1269 100644
--- a/src/or/proto_socks.c
+++ b/src/or/proto_socks.c
@@ -393,7 +393,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
           req->port = ntohs(get_uint16(data+5+len));
           *drain_out = 5+len+2;
 
-          if (!string_is_valid_hostname(req->address)) {
+          if (!string_is_valid_dest(req->address)) {
             socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR);
 
             log_warn(LD_PROTOCOL,
@@ -518,7 +518,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
       log_debug(LD_APP,"socks4: Everything is here. Success.");
       strlcpy(req->address, startaddr ? startaddr : tmpbuf,
               sizeof(req->address));
-      if (!string_is_valid_hostname(req->address)) {
+      if (!string_is_valid_dest(req->address)) {
         log_warn(LD_PROTOCOL,
                  "Your application (using socks4 to port %d) gave Tor "
                  "a malformed hostname: %s. Rejecting the connection.",
diff --git a/src/test/test_socks.c b/src/test/test_socks.c
index 9ae7530e22..8da7191e82 100644
--- a/src/test/test_socks.c
+++ b/src/test/test_socks.c
@@ -347,17 +347,35 @@ test_socks_5_supported_commands(void *ptr)
 
   socks_request_clear(socks);
 
-  /* SOCKS 5 should NOT reject RESOLVE [F0] reject for IPv6 address
+  /* SOCKS 5 should NOT reject RESOLVE [F0] request for IPv6 address
    * string if SafeSocks is enabled. */
 
   ADD_DATA(buf, "\x05\x01\x00");
+  ADD_DATA(buf, "\x05\xF0\x00\x03\x29");
+  ADD_DATA(buf, "[2001:0db8:85a3:0000:0000:8a2e:0370:7334]");
+  ADD_DATA(buf, "\x01\x02");
+  tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, 1),
+            OP_EQ, 1);
+
+  tt_str_op("[2001:0db8:85a3:0000:0000:8a2e:0370:7334]", OP_EQ,
+    socks->address);
+  tt_int_op(258, OP_EQ, socks->port);
+
+  tt_int_op(0, OP_EQ, buf_datalen(buf));
+
+  socks_request_clear(socks);
+
+  /* Also allow bracket-less form. */
+
+  ADD_DATA(buf, "\x05\x01\x00");
   ADD_DATA(buf, "\x05\xF0\x00\x03\x27");
   ADD_DATA(buf, "2001:0db8:85a3:0000:0000:8a2e:0370:7334");
   ADD_DATA(buf, "\x01\x02");
   tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, 1),
-            OP_EQ, -1);
+            OP_EQ, 1);
 
-  tt_str_op("2001:0db8:85a3:0000:0000:8a2e:0370:7334", OP_EQ, socks->address);
+  tt_str_op("2001:0db8:85a3:0000:0000:8a2e:0370:7334", OP_EQ,
+    socks->address);
   tt_int_op(258, OP_EQ, socks->port);
 
   tt_int_op(0, OP_EQ, buf_datalen(buf));
diff --git a/src/test/test_util.c b/src/test/test_util.c
index b67fad58e3..036f739b89 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -5542,47 +5542,74 @@ test_util_max_mem(void *arg)
 }
 
 static void
+test_util_dest_validation_edgecase(void *arg)
+{
+  (void)arg;
+
+  tt_assert(!string_is_valid_dest(NULL));
+  tt_assert(!string_is_valid_dest(""));
+
+  done:
+  return;
+}
+
+static void
 test_util_hostname_validation(void *arg)
 {
   (void)arg;
 
   // Lets try valid hostnames first.
-  tt_assert(string_is_valid_hostname("torproject.org"));
-  tt_assert(string_is_valid_hostname("ocw.mit.edu"));
-  tt_assert(string_is_valid_hostname("i.4cdn.org"));
-  tt_assert(string_is_valid_hostname("stanford.edu"));
-  tt_assert(string_is_valid_hostname("multiple-words-with-hypens.jp"));
+  tt_assert(string_is_valid_nonrfc_hostname("torproject.org"));
+  tt_assert(string_is_valid_nonrfc_hostname("ocw.mit.edu"));
+  tt_assert(string_is_valid_nonrfc_hostname("i.4cdn.org"));
+  tt_assert(string_is_valid_nonrfc_hostname("stanford.edu"));
+  tt_assert(string_is_valid_nonrfc_hostname("multiple-words-with-hypens.jp"));
 
   // Subdomain name cannot start with '-' or '_'.
-  tt_assert(!string_is_valid_hostname("-torproject.org"));
-  tt_assert(!string_is_valid_hostname("subdomain.-domain.org"));
-  tt_assert(!string_is_valid_hostname("-subdomain.domain.org"));
-  tt_assert(!string_is_valid_hostname("___abc.org"));
+  tt_assert(!string_is_valid_nonrfc_hostname("-torproject.org"));
+  tt_assert(!string_is_valid_nonrfc_hostname("subdomain.-domain.org"));
+  tt_assert(!string_is_valid_nonrfc_hostname("-subdomain.domain.org"));
+  tt_assert(!string_is_valid_nonrfc_hostname("___abc.org"));
 
   // Hostnames cannot contain non-alphanumeric characters.
-  tt_assert(!string_is_valid_hostname("%%domain.\\org."));
-  tt_assert(!string_is_valid_hostname("***x.net"));
-  tt_assert(!string_is_valid_hostname("\xff\xffxyz.org"));
-  tt_assert(!string_is_valid_hostname("word1 word2.net"));
+  tt_assert(!string_is_valid_nonrfc_hostname("%%domain.\\org."));
+  tt_assert(!string_is_valid_nonrfc_hostname("***x.net"));
+  tt_assert(!string_is_valid_nonrfc_hostname("\xff\xffxyz.org"));
+  tt_assert(!string_is_valid_nonrfc_hostname("word1 word2.net"));
 
   // Test workaround for nytimes.com stupidity, technically invalid,
   // but we allow it since they are big, even though they are failing to
   // comply with a ~30 year old standard.
-  tt_assert(string_is_valid_hostname("core3_euw1.fabrik.nytimes.com"));
+  tt_assert(string_is_valid_nonrfc_hostname("core3_euw1.fabrik.nytimes.com"));
 
   // Firefox passes FQDNs with trailing '.'s  directly to the SOCKS proxy,
   // which is redundant since the spec states DOMAINNAME addresses are fully
   // qualified.  While unusual, this should be tollerated.
-  tt_assert(string_is_valid_hostname("core9_euw1.fabrik.nytimes.com."));
-  tt_assert(!string_is_valid_hostname("..washingtonpost.is.better.com"));
-  tt_assert(!string_is_valid_hostname("so.is..ft.com"));
-  tt_assert(!string_is_valid_hostname("..."));
+  tt_assert(string_is_valid_nonrfc_hostname("core9_euw1.fabrik.nytimes.com."));
+  tt_assert(!string_is_valid_nonrfc_hostname(
+                                         "..washingtonpost.is.better.com"));
+  tt_assert(!string_is_valid_nonrfc_hostname("so.is..ft.com"));
+  tt_assert(!string_is_valid_nonrfc_hostname("..."));
 
   // XXX: do we allow single-label DNS names?
   // We shouldn't for SOCKS (spec says "contains a fully-qualified domain name"
   // but only test pathologically malformed traling '.' cases for now.
-  tt_assert(!string_is_valid_hostname("."));
-  tt_assert(!string_is_valid_hostname(".."));
+  tt_assert(!string_is_valid_nonrfc_hostname("."));
+  tt_assert(!string_is_valid_nonrfc_hostname(".."));
+
+  // IP address strings are not hostnames.
+  tt_assert(!string_is_valid_nonrfc_hostname("8.8.8.8"));
+  tt_assert(!string_is_valid_nonrfc_hostname("[2a00:1450:401b:800::200e]"));
+  tt_assert(!string_is_valid_nonrfc_hostname("2a00:1450:401b:800::200e"));
+
+  // We allow alphanumeric TLDs. For discussion, see ticket #25055.
+  tt_assert(string_is_valid_nonrfc_hostname("lucky.13"));
+  tt_assert(string_is_valid_nonrfc_hostname("luck.y13"));
+  tt_assert(string_is_valid_nonrfc_hostname("luck.y13."));
+
+  // We allow punycode TLDs. For examples, see
+  // http://data.iana.org/TLD/tlds-alpha-by-domain.txt
+  tt_assert(string_is_valid_nonrfc_hostname("example.xn--l1acc"));
 
   done:
   return;
@@ -6208,6 +6235,7 @@ struct testcase_t util_tests[] = {
     &passthrough_setup, (void*)"1" },
   UTIL_TEST(max_mem, 0),
   UTIL_TEST(hostname_validation, 0),
+  UTIL_TEST(dest_validation_edgecase, 0),
   UTIL_TEST(ipv4_validation, 0),
   UTIL_TEST(writepid, 0),
   UTIL_TEST(get_avail_disk_space, 0),