diff --git a/changes/bug13315 b/changes/bug13315 new file mode 100644 index 000000000..c2ae5ff1f --- /dev/null +++ b/changes/bug13315 @@ -0,0 +1,5 @@ + o Minor features: + - Validate hostnames in SOCKS5 requests more strictly. If SafeSocks + is enabled, reject requests with IP addresses as hostnames. Resolves + ticket 13315. + diff --git a/src/common/util.c b/src/common/util.c index 27cc9df87..1359776b2 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -964,6 +964,68 @@ string_is_key_value(int severity, const char *string) return 1; } +/** Return true if string represents a valid IPv4 adddress in + * 'a.b.c.d' form. + */ +int +string_is_valid_ipv4_address(const char *string) +{ + struct in_addr addr; + + return (tor_inet_pton(AF_INET,string,&addr) == 1); +} + +/** Return true if string represents a valid IPv6 address in + * a form that inet_pton() can parse. + */ +int +string_is_valid_ipv6_address(const char *string) +{ + struct in6_addr addr; + + return (tor_inet_pton(AF_INET6,string,&addr) == 1); +} + +/** Return true iff string matches a pattern of DNS names + * that we allow Tor clients to connect to. + */ +int +string_is_valid_hostname(const char *string) +{ + int result = 1; + smartlist_t *components; + + components = smartlist_new(); + + smartlist_split_string(components,string,".",0,0); + + SMARTLIST_FOREACH_BEGIN(components, char *, c) { + if (c[0] == '-') { + result = 0; + break; + } + + do { + if ((*c >= 'a' && *c <= 'z') || + (*c >= 'A' && *c <= 'Z') || + (*c >= '0' && *c <= '9') || + (*c == '-')) + c++; + else + result = 0; + } while (result && *c); + + } SMARTLIST_FOREACH_END(c); + + SMARTLIST_FOREACH_BEGIN(components, char *, c) { + tor_free(c); + } SMARTLIST_FOREACH_END(c); + + smartlist_free(components); + + return result; +} + /** Return true iff the DIGEST256_LEN bytes in digest are all zero. */ int tor_digest256_is_zero(const char *digest) diff --git a/src/common/util.h b/src/common/util.h index d7feb6e92..9886b2db6 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -227,6 +227,9 @@ 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_ipv4_address(const char *string); +int string_is_valid_ipv6_address(const char *string); int tor_mem_is_zero(const char *mem, size_t len); int tor_digest_is_zero(const char *digest); diff --git a/src/or/buffers.c b/src/or/buffers.c index 080d8fb6c..bd33fe451 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2054,8 +2054,20 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->address[len] = 0; req->port = ntohs(get_uint16(data+5+len)); *drain_out = 5+len+2; - if (!tor_strisprint(req->address) || strchr(req->address,'\"')) { + + if (string_is_valid_ipv4_address(req->address) || + string_is_valid_ipv6_address(req->address)) { + log_unsafe_socks_warning(5,req->address,req->port,safe_socks); + + if (safe_socks) { + socks_request_set_socks5_error(req, SOCKS5_NOT_ALLOWED); + return -1; + } + } + + if (!string_is_valid_hostname(req->address)) { socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR); + log_warn(LD_PROTOCOL, "Your application (using socks5 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 5cc09e200..29faa69fb 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -229,6 +229,42 @@ test_socks_5_supported_commands(void *ptr) tt_int_op(0,==, buf_datalen(buf)); socks_request_clear(socks); + /* SOCKS 5 Should reject RESOLVE [F0] request for IPv4 address + * string if SafeSocks is enabled. */ + + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\xF0\x00\x03\x07"); + ADD_DATA(buf, "8.8.8.8"); + ADD_DATA(buf, "\x01\x02"); + tt_assert(fetch_from_buf_socks(buf,socks,get_options()->TestSocks,1) + == -1); + + tt_int_op(5,==,socks->socks_version); + tt_int_op(10,==,socks->replylen); + tt_int_op(5,==,socks->reply[0]); + tt_int_op(SOCKS5_NOT_ALLOWED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); + + socks_request_clear(socks); + + /* SOCKS 5 should reject RESOLVE [F0] reject for IPv6 address + * string if SafeSocks is enabled. */ + + 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_assert(fetch_from_buf_socks(buf,socks,get_options()->TestSocks,1) + == -1); + + tt_int_op(5,==,socks->socks_version); + tt_int_op(10,==,socks->replylen); + tt_int_op(5,==,socks->reply[0]); + tt_int_op(SOCKS5_NOT_ALLOWED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); + + socks_request_clear(socks); + /* SOCKS 5 Send RESOLVE_PTR [F1] for IP address 2.2.2.5 */ ADD_DATA(buf, "\x05\x01\x00"); ADD_DATA(buf, "\x05\xF1\x00\x01\x02\x02\x02\x05\x01\x03"); diff --git a/src/test/test_util.c b/src/test/test_util.c index a37d7a4aa..04dfe64f5 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -4790,6 +4790,52 @@ test_util_max_mem(void *arg) ; } +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")); + + // Subdomain name cannot start with '-'. + 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")); + + // 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("___abc.org")); + tt_assert(!string_is_valid_hostname("\xff\xffxyz.org")); + tt_assert(!string_is_valid_hostname("word1 word2.net")); + + // XXX: do we allow single-label DNS names? + + done: + return; +} + +static void +test_util_ipv4_validation(void *arg) +{ + (void)arg; + + tt_assert(string_is_valid_ipv4_address("192.168.0.1")); + tt_assert(string_is_valid_ipv4_address("8.8.8.8")); + + tt_assert(!string_is_valid_ipv4_address("abcd")); + tt_assert(!string_is_valid_ipv4_address("300.300.300.300")); + tt_assert(!string_is_valid_ipv4_address("8.8.")); + + done: + return; +} + struct testcase_t util_tests[] = { UTIL_LEGACY(time), UTIL_TEST(parse_http_time, 0), @@ -4863,6 +4909,8 @@ struct testcase_t util_tests[] = { { "socketpair_ersatz", test_util_socketpair, TT_FORK, &socketpair_setup, (void*)"1" }, UTIL_TEST(max_mem, 0), + UTIL_TEST(hostname_validation, 0), + UTIL_TEST(ipv4_validation, 0), END_OF_TESTCASES };