diff --git a/changes/bugs_25036_25055 b/changes/bugs_25036_25055 new file mode 100644 index 000000000..daa46321c --- /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 90204befc..a68fd30d0 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 string 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 string 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 2ee0ea28c..938078912 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 91633d02a..8700fe126 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 9ae7530e2..8da7191e8 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 b67fad58e..036f739b8 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -5541,48 +5541,75 @@ 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),