From 1ea9a6fd72b66ec634446cbd2119641a5ed1e703 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Sun, 12 Oct 2014 19:33:08 +0300 Subject: [PATCH 1/9] Introducing helper function to validate DNS name strings. --- src/common/util.c | 36 ++++++++++++++++++++++++++++++++++++ src/common/util.h | 1 + src/test/test_util.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/src/common/util.c b/src/common/util.c index f4d293c838..606b9e1ea0 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -957,6 +957,42 @@ string_is_key_value(int severity, const char *string) return 1; } +/** Return true iff string is valid DNS name, as defined in + * RFC 1035 Section 2.3.1. + */ +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_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 61bb05f016..2634adc62b 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -227,6 +227,7 @@ 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 tor_mem_is_zero(const char *mem, size_t len); int tor_digest_is_zero(const char *digest); diff --git a/src/test/test_util.c b/src/test/test_util.c index c67aa71b02..fb3ce7d941 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -4122,6 +4122,36 @@ 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; +} + struct testcase_t util_tests[] = { UTIL_LEGACY(time), UTIL_TEST(parse_http_time, 0), @@ -4194,6 +4224,7 @@ 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), END_OF_TESTCASES }; From e8e45ff13ed86d8851bab77d65d899d0ca6e3b89 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Sun, 12 Oct 2014 20:39:00 +0300 Subject: [PATCH 2/9] Introducing helper function to validate IPv4 address strings. --- src/common/util.c | 19 +++++++++++++++++-- src/common/util.h | 1 + src/test/test_util.c | 21 +++++++++++++++++++-- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 606b9e1ea0..ba9d78afac 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -957,8 +957,19 @@ string_is_key_value(int severity, const char *string) return 1; } -/** Return true iff string is valid DNS name, as defined in - * RFC 1035 Section 2.3.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 sockaddr_in sockaddr; + + return (tor_inet_pton(AF_INET,string,&sockaddr) == 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) @@ -988,6 +999,10 @@ string_is_valid_hostname(const char *string) } SMARTLIST_FOREACH_END(c); + SMARTLIST_FOREACH_BEGIN(components, char *, c) { + tor_free(c); + } SMARTLIST_FOREACH_END(c); + smartlist_free(components); return result; diff --git a/src/common/util.h b/src/common/util.h index 2634adc62b..dcb54f0aea 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -228,6 +228,7 @@ const char *find_str_at_start_of_line(const char *haystack, 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 tor_mem_is_zero(const char *mem, size_t len); int tor_digest_is_zero(const char *digest); diff --git a/src/test/test_util.c b/src/test/test_util.c index fb3ce7d941..fba90da492 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -4138,9 +4138,9 @@ test_util_hostname_validation(void *arg) 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("%%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")); @@ -4152,6 +4152,22 @@ test_util_hostname_validation(void *arg) 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), @@ -4225,6 +4241,7 @@ struct testcase_t util_tests[] = { &socketpair_setup, (void*)"1" }, UTIL_TEST(max_mem, 0), UTIL_TEST(hostname_validation, 0), + UTIL_TEST(ipv4_validation, 0), END_OF_TESTCASES }; From 2862b769deaaaa40347ffe808349c4e139e7eb45 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Sun, 12 Oct 2014 21:04:15 +0300 Subject: [PATCH 3/9] Validating SOCKS5 hostname more correctly. --- src/or/buffers.c | 10 +++++++++- src/test/test_socks.c | 11 +++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index d174f8147a..e98f56932d 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2048,7 +2048,15 @@ 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)) { + log_unsafe_socks_warning(5,req->address,req->port,safe_socks); + + if (safe_socks) + return -1; + } + + if (!string_is_valid_hostname(req->address)) { 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 2b8f824b50..b9520b5c5c 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -229,6 +229,17 @@ 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); + 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"); From 2f1068e68a95a2ad9ba058297b54622323b216c6 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Tue, 14 Oct 2014 21:53:48 +0300 Subject: [PATCH 4/9] Adding helper function that checks if string is a valid IPv6 address. --- src/common/util.c | 11 +++++++++++ src/common/util.h | 1 + 2 files changed, 12 insertions(+) diff --git a/src/common/util.c b/src/common/util.c index ba9d78afac..e9ee437190 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -968,6 +968,17 @@ string_is_valid_ipv4_address(const char *string) return (tor_inet_pton(AF_INET,string,&sockaddr) == 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 sockaddr_in sockaddr_dummy; + + return (inet_pton(AF_INET6,string,&sockaddr_dummy) == 1); +} + /** Return true iff string matches a pattern of DNS names * that we allow Tor clients to connect to. */ diff --git a/src/common/util.h b/src/common/util.h index dcb54f0aea..5eecfada28 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -229,6 +229,7 @@ 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); From 0da4ddda4f8f1c3c931349e45acbdcae2b7cc750 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Tue, 14 Oct 2014 21:56:04 +0300 Subject: [PATCH 5/9] Checking if FQDN is actually IPv6 address string and handling that case. --- src/common/util.c | 2 +- src/or/buffers.c | 3 ++- src/test/test_socks.c | 11 +++++++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index e9ee437190..c292c798cc 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -976,7 +976,7 @@ string_is_valid_ipv6_address(const char *string) { struct sockaddr_in sockaddr_dummy; - return (inet_pton(AF_INET6,string,&sockaddr_dummy) == 1); + return (tor_inet_pton(AF_INET6,string,&sockaddr_dummy) == 1); } /** Return true iff string matches a pattern of DNS names diff --git a/src/or/buffers.c b/src/or/buffers.c index e98f56932d..354bec64bc 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2049,7 +2049,8 @@ 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_ipv4_address(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) diff --git a/src/test/test_socks.c b/src/test/test_socks.c index b9520b5c5c..ba6b9a9771 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -240,6 +240,17 @@ test_socks_5_supported_commands(void *ptr) == -1); 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); + 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"); From fc9591da727817923e1084d1a73d5dd8a52e6948 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Tue, 14 Oct 2014 22:09:44 +0300 Subject: [PATCH 6/9] Adding changes file for 13315. --- changes/bug13315 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug13315 diff --git a/changes/bug13315 b/changes/bug13315 new file mode 100644 index 0000000000..c2ae5ff1f8 --- /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. + From 51e247361824fa64f4322fb59e9d2cffd9d72cba Mon Sep 17 00:00:00 2001 From: rl1987 Date: Tue, 21 Oct 2014 20:50:32 +0300 Subject: [PATCH 7/9] Sending 'Not allowed' error message before closing the connection. --- src/or/buffers.c | 4 +++- src/test/test_socks.c | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 354bec64bc..691845ec10 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2053,8 +2053,10 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, string_is_valid_ipv6_address(req->address)) { log_unsafe_socks_warning(5,req->address,req->port,safe_socks); - if (safe_socks) + if (safe_socks) { + socks_request_set_socks5_error(req, SOCKS5_NOT_ALLOWED); return -1; + } } if (!string_is_valid_hostname(req->address)) { diff --git a/src/test/test_socks.c b/src/test/test_socks.c index ba6b9a9771..a3fe07fdc5 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -238,6 +238,13 @@ test_socks_5_supported_commands(void *ptr) 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 @@ -249,6 +256,13 @@ test_socks_5_supported_commands(void *ptr) 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 */ From 254ab5a8deb9ed5fb571016f402ffd7243208d73 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 4 Nov 2014 00:45:14 -0500 Subject: [PATCH 8/9] Use correct argument types for inet_pton. (I blame whoever decided that using a void* for a union was a good idea.) --- src/common/util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index c292c798cc..83002997ef 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -963,9 +963,9 @@ string_is_key_value(int severity, const char *string) int string_is_valid_ipv4_address(const char *string) { - struct sockaddr_in sockaddr; + struct in_addr addr; - return (tor_inet_pton(AF_INET,string,&sockaddr) == 1); + return (tor_inet_pton(AF_INET,string,&addr) == 1); } /** Return true if string represents a valid IPv6 address in @@ -974,9 +974,9 @@ string_is_valid_ipv4_address(const char *string) int string_is_valid_ipv6_address(const char *string) { - struct sockaddr_in sockaddr_dummy; + struct in6_addr addr; - return (tor_inet_pton(AF_INET6,string,&sockaddr_dummy) == 1); + return (tor_inet_pton(AF_INET6,string,&addr) == 1); } /** Return true iff string matches a pattern of DNS names From 74cbd8d55953969c15c60f025889b1e07a185a79 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 4 Nov 2014 00:46:32 -0500 Subject: [PATCH 9/9] fix indentation --- src/common/util.c | 56 +++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 83002997ef..c6a39898d9 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -963,9 +963,9 @@ string_is_key_value(int severity, const char *string) int string_is_valid_ipv4_address(const char *string) { - struct in_addr addr; + struct in_addr addr; - return (tor_inet_pton(AF_INET,string,&addr) == 1); + return (tor_inet_pton(AF_INET,string,&addr) == 1); } /** Return true if string represents a valid IPv6 address in @@ -974,9 +974,9 @@ string_is_valid_ipv4_address(const char *string) int string_is_valid_ipv6_address(const char *string) { - struct in6_addr addr; + struct in6_addr addr; - return (tor_inet_pton(AF_INET6,string,&addr) == 1); + return (tor_inet_pton(AF_INET6,string,&addr) == 1); } /** Return true iff string matches a pattern of DNS names @@ -985,38 +985,38 @@ string_is_valid_ipv6_address(const char *string) int string_is_valid_hostname(const char *string) { - int result = 1; - smartlist_t *components; + int result = 1; + smartlist_t *components; - components = smartlist_new(); + components = smartlist_new(); - smartlist_split_string(components,string,".",0,0); + smartlist_split_string(components,string,".",0,0); - SMARTLIST_FOREACH_BEGIN(components, char *, c) { - if (c[0] == '-') { - result = 0; - break; - } + 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); + 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_END(c); - SMARTLIST_FOREACH_BEGIN(components, char *, c) { - tor_free(c); - } SMARTLIST_FOREACH_END(c); + SMARTLIST_FOREACH_BEGIN(components, char *, c) { + tor_free(c); + } SMARTLIST_FOREACH_END(c); - smartlist_free(components); + smartlist_free(components); - return result; + return result; } /** Return true iff the DIGEST256_LEN bytes in digest are all zero. */