commit c8132aab9291527db2ba0524fbd84a3041bcd6fd Author: Yawning Angel <yawning@schwanenlied.me> Date: Wed Oct 1 14:16:59 2014 +0000 Send back SOCKS5 errors for all of the address related failures. Cases that now send errors: * Malformed IP address (SOCKS5_GENERAL_ERROR) * CONNECT/RESOLVE request with IP, when SafeSocks is set (SOCKS5_NOT_ALLOWED) * RESOLVE_PTR request with FQDN (SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED) * Malformed FQDN (SOCKS5_GENERAL_ERROR) * Unknown address type (SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED) Fixes bug 13314. --- changes/bug13314 | 4 +++ src/or/buffers.c | 11 +++++++- src/test/test_socks.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/changes/bug13314 b/changes/bug13314 new file mode 100644 index 0000000..e9017fa --- /dev/null +++ b/changes/bug13314 @@ -0,0 +1,4 @@ + o Bugfixes: + - Handle malformed SOCKS5 requests properly by responding with an + appropriate error message before closing a TCP connection to the + user. Fixes bug 13314. diff --git a/src/or/buffers.c b/src/or/buffers.c index d174f81..9f02824 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2009,6 +2009,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, tor_addr_to_str(tmpbuf, &destaddr, sizeof(tmpbuf), 1); if (strlen(tmpbuf)+1 > MAX_SOCKS_ADDR_LEN) { + socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR); log_warn(LD_APP, "socks5 IP takes %d bytes, which doesn't fit in %d. " "Rejecting.", @@ -2021,14 +2022,18 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, if (req->command != SOCKS_COMMAND_RESOLVE_PTR && !addressmap_have_mapping(req->address,0)) { 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; + } } return 1; } case 3: /* fqdn */ log_debug(LD_APP,"socks5: fqdn address type"); if (req->command == SOCKS_COMMAND_RESOLVE_PTR) { + socks_request_set_socks5_error(req, + SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED); log_warn(LD_APP, "socks5 received RESOLVE_PTR command with " "hostname type. Rejecting."); return -1; @@ -2039,6 +2044,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, return 0; /* not yet */ } if (len+1 > MAX_SOCKS_ADDR_LEN) { + socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR); log_warn(LD_APP, "socks5 hostname is %d bytes, which doesn't fit in " "%d. Rejecting.", len+1,MAX_SOCKS_ADDR_LEN); @@ -2049,6 +2055,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 (!tor_strisprint(req->address) || strchr(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.", @@ -2062,6 +2069,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, "necessary. This is good.", req->port); return 1; default: /* unsupported */ + socks_request_set_socks5_error(req, + SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED); log_warn(LD_APP,"socks5: unsupported address type %d. Rejecting.", (int) *(data+3)); return -1; diff --git a/src/test/test_socks.c b/src/test/test_socks.c index 2b8f824..9aaa16e 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -384,6 +384,77 @@ test_socks_5_auth_before_negotiation(void *ptr) ; } +/** Perform malformed SOCKS 5 commands */ +static void +test_socks_5_malformed_commands(void *ptr) +{ + SOCKS_TEST_INIT(); + + /* XXX: Stringified address length > MAX_SOCKS_ADDR_LEN will never happen */ + + /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369, with SafeSocks set */ + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); + tt_int_op(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]); + + buf_clear(buf); + socks_request_clear(socks); + + /* SOCKS 5 Send RESOLVE_PTR [F1] for FQDN torproject.org */ + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\xF1\x00\x03\x0Etorproject.org\x11\x11"); + tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks),==, -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_ADDRESS_TYPE_NOT_SUPPORTED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); + + buf_clear(buf); + socks_request_clear(socks); + + /* XXX: len + 1 > MAX_SOCKS_ADDR_LEN (FQDN request) will never happen */ + + /* SOCKS 5 Send CONNECT [01] to FQDN """"".com */ + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\x01\x00\x03\x09\"\"\"\"\".com\x11\x11"); + tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks),==, -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_GENERAL_ERROR,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); + + buf_clear(buf); + socks_request_clear(socks); + + /* SOCKS 5 Send CONNECT [01] to address type 0x23 */ + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\x01\x00\x23\x02\x02\x02\x02\x11\x11"); + tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks),==, -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_ADDRESS_TYPE_NOT_SUPPORTED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); + + done: + ; +} + #define SOCKSENT(name) \ { #name, test_socks_##name, TT_FORK, &socks_setup, NULL } @@ -397,6 +468,7 @@ struct testcase_t socks_tests[] = { SOCKSENT(5_auth_before_negotiation), SOCKSENT(5_authenticate), SOCKSENT(5_authenticate_with_data), + SOCKSENT(5_malformed_commands), END_OF_TESTCASES };