From bf136b94de39de65638ce3daaf2e87731cd0a44a Mon Sep 17 00:00:00 2001 From: Robert Hogan Date: Tue, 3 Aug 2010 22:28:55 +0100 Subject: [PATCH 01/14] bug1666 - Pass-through support for SOCKS5 authentication If a SOCKS5 client insists on authentication, allow it to negotiate a connection with Tor's SOCKS server successfully. Any credentials the client provides are ignored. This allows Tor to work with SOCKS5 clients that can only support 'authenticated' connections. Also add a bunch of basic unit tests for SOCKS4/4a/5 support in buffers.c. --- doc/spec/socks-extensions.txt | 5 +- src/or/buffers.c | 64 ++++++--- src/or/config.c | 3 +- src/or/config.h | 2 + src/or/or.h | 2 + src/test/test.c | 237 ++++++++++++++++++++++++++++++++++ 6 files changed, 294 insertions(+), 19 deletions(-) diff --git a/doc/spec/socks-extensions.txt b/doc/spec/socks-extensions.txt index 62d86acd9f..52d6834552 100644 --- a/doc/spec/socks-extensions.txt +++ b/doc/spec/socks-extensions.txt @@ -31,8 +31,9 @@ Tor's extensions to the SOCKS protocol SOCKS5: - The (SOCKS5) "UDP ASSOCIATE" command is not supported. - IPv6 is not supported in CONNECT commands. - - Only the "NO AUTHENTICATION" (SOCKS5) authentication method [00] is - supported. + - The "NO AUTHENTICATION" (SOCKS5) authentication method [00] and the + "USERNAME/PASSWORD" (SOCKS5) authentication method [02] are both + supported. Any credentials passed in the latter are ignored. 2. Name lookup diff --git a/src/or/buffers.c b/src/or/buffers.c index 11c4fc8b9b..534f31c17a 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1633,40 +1633,74 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, uint8_t socksver; enum {socks4, socks4a} socks4_prot = socks4a; char *next, *startaddr; + unsigned char usernamelen, passlen; struct in_addr in; socksver = *data; switch (socksver) { /* which version of socks? */ + case 1: /* socks5: username/password authentication request */ + + usernamelen = (unsigned char)*(buf->head->data + 1); + if (buf->datalen < 2u + usernamelen) + return 0; + buf_pullup(buf, 2u + usernamelen + 1, 0); + passlen = (unsigned char)*(buf->head->data + 2u + usernamelen); + if (buf->datalen < 2u + usernamelen + 1u + passlen) + return 0; + if (buf->datalen > 2u + usernamelen + 1u + passlen) { + log_warn(LD_APP, + "socks5: Malformed username/password. Rejecting."); + return -1; + } + req->replylen = 2; /* 2 bytes of response */ + req->reply[0] = 5; + req->reply[1] = 0; /* authentication successful */ + buf_clear(buf); + log_debug(LD_APP, + "socks5: Accepted username/password without checking."); + return 0; + case 5: /* socks5 */ if (req->socks_version != 5) { /* we need to negotiate a method */ unsigned char nummethods = (unsigned char)*(data+1); + int r=0; tor_assert(!req->socks_version); if (datalen < 2u+nummethods) { *want_length_out = 2u+nummethods; return 0; } - if (!nummethods || !memchr(data+2, 0, nummethods)) { - log_warn(LD_APP, - "socks5: offered methods don't include 'no auth'. " - "Rejecting."); - req->replylen = 2; /* 2 bytes of response */ - req->reply[0] = 5; - req->reply[1] = '\xFF'; /* reject all methods */ + buf_pullup(buf, 2u+nummethods, 0); + if (!nummethods) return -1; + req->replylen = 2; /* 2 bytes of response */ + req->reply[0] = 5; /* socks5 reply */ + if (memchr(buf->head->data+2, SOCKS_NO_AUTH, nummethods)) { + req->reply[1] = SOCKS_NO_AUTH; /* tell client to use "none" auth + method */ + req->socks_version = 5; /* remember we've already negotiated auth */ + log_debug(LD_APP,"socks5: accepted method 0 (no authentication)"); + r=0; + }else if (memchr(buf->head->data+2, SOCKS_USER_PASS,nummethods)) { + req->reply[1] = SOCKS_USER_PASS; /* tell client to use "user/pass" + auth method */ + req->socks_version = 5; /* remember we've already negotiated auth */ + log_debug(LD_APP,"socks5: accepted method 2 (username/password)"); + r=0; + } else { + log_warn(LD_APP, + "socks5: offered methods don't include 'no auth' or " + "username/password. Rejecting."); + req->reply[1] = '\xFF'; /* reject all methods */ + r=-1; } /* remove packet from buf. also remove any other extraneous * bytes, to support broken socks clients. */ *drain_out = -1; - req->replylen = 2; /* 2 bytes of response */ - req->reply[0] = 5; /* socks5 reply */ - req->reply[1] = 0; /* tell client to use "none" auth method */ - req->socks_version = 5; /* remember we've already negotiated auth */ - log_debug(LD_APP,"socks5: accepted method 0"); - return 0; + return r; } /* we know the method; read in the request */ log_debug(LD_APP,"socks5: checking request"); @@ -1761,8 +1795,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, } tor_assert(0); case 4: /* socks4 */ - /* http://archive.socks.permeo.com/protocol/socks4.protocol */ - /* http://archive.socks.permeo.com/protocol/socks4a.protocol */ + /* http://ss5.sourceforge.net/socks4.protocol.txt */ + /* http://ss5.sourceforge.net/socks4A.protocol.txt */ req->socks_version = 4; if (datalen < SOCKS4_NETWORK_LEN) {/* basic info available? */ diff --git a/src/or/config.c b/src/or/config.c index cd6b0b0c3c..d2dbdaa5dd 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -542,7 +542,6 @@ static int options_transition_affects_workers(or_options_t *old_options, static int options_transition_affects_descriptor(or_options_t *old_options, or_options_t *new_options); static int check_nickname_list(const char *lst, const char *name, char **msg); -static void config_register_addressmaps(or_options_t *options); static int parse_bridge_line(const char *line, int validate_only); static int parse_dir_server_line(const char *line, @@ -4314,7 +4313,7 @@ get_torrc_fname(void) /** Adjust the address map based on the MapAddress elements in the * configuration options */ -static void +void config_register_addressmaps(or_options_t *options) { smartlist_t *elts; diff --git a/src/or/config.h b/src/or/config.h index bd5827b4e8..db871d472a 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -76,5 +76,7 @@ uint32_t get_effective_bwburst(or_options_t *options); or_options_t *options_new(void); #endif +void config_register_addressmaps(or_options_t *options); + #endif diff --git a/src/or/or.h b/src/or/or.h index c5349aed55..6ae62e8899 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3067,6 +3067,8 @@ static INLINE void or_state_mark_dirty(or_state_t *state, time_t when) #define MAX_SOCKS_REPLY_LEN 1024 #define MAX_SOCKS_ADDR_LEN 256 +#define SOCKS_NO_AUTH 0x00 +#define SOCKS_USER_PASS 0x02 /** Please open a TCP connection to this addr:port. */ #define SOCKS_COMMAND_CONNECT 0x01 diff --git a/src/test/test.c b/src/test/test.c index 9b0251b316..68f1aebf7c 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -197,6 +197,207 @@ free_pregenerated_keys(void) } } +/** Helper: Perform supported SOCKS 5 commands */ +static void +test_buffers_socks4_unsupported_commands_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 4 Send BIND [02] to IP address 2.2.2.2:4369 */ + cp = "\x04\x02\x11\x11\x02\x02\x02\x02\x00"; + write_to_buf(cp, 9, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == -1); + test_eq(4, socks->socks_version); + test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ + +done: + ; +} + +/** Helper: Perform supported SOCKS 5 commands */ +static void +test_buffers_socks4_supported_commands_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4369 */ + cp = "\x04\x01\x11\x11\x02\x02\x02\x02\x00"; + write_to_buf(cp, 9, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(4, socks->socks_version); + test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ + test_streq("2.2.2.2", socks->address); + test_eq(4369, socks->port); + + /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4369 with userid*/ + cp = "\x04\x01\x11\x11\x02\x02\x02\x02\x02me\x00"; + write_to_buf(cp, 12, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(4, socks->socks_version); + test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ + test_streq("2.2.2.2", socks->address); + test_eq(4369, socks->port); + + /* SOCKS 4a Send RESOLVE [F0] request for torproject.org:4369 */ + cp = "\x04\xF0\x01\x01\x00\x00\x00\x02\x02me\x00tor.org\x00"; + write_to_buf(cp, 20, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(4, socks->socks_version); + test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ + test_streq("tor.org", socks->address); + +done: + ; +} + +/** Helper: Perform supported SOCKS 5 commands */ +static void +test_buffers_socks5_unsupported_commands_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 5 Send unsupported BIND [02] command */ + cp = "\x05\x02\x00\x01\x02\x02\x02\x02\x01\x01"; + write_to_buf(cp, 10, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == -1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); /* XXX: shouldn't tor reply 'command + not supported' [07]? */ + + /* SOCKS 5 Send unsupported UDP_ASSOCIATE [03] command */ + cp = "\x05\x03\x00\x01\x02\x02\x02\x02\x01\x01"; + write_to_buf(cp, 10, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == -1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); /* XXX: shouldn't tor reply 'command + not supported' [07]? */ + +done: + ; +} + +/** Helper: Perform supported SOCKS 5 commands */ +static void +test_buffers_socks5_supported_commands_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */ + cp = "\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"; + write_to_buf(cp, 10, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("2.2.2.2", socks->address); + test_eq(4369, socks->port); + + /* SOCKS 5 Send CONNECT [01] to FQDN torproject.org:4369 */ + cp = "\x05\x01\x00\x03\x07tor.org\x11\x11"; + write_to_buf(cp, 14, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("tor.org", socks->address); + test_eq(4369, socks->port); + + /* SOCKS 5 Send RESOLVE [F0] request for torproject.org:4369 */ + cp = "\x05\xF0\x00\x03\x07tor.org"; + write_to_buf(cp, 14, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("tor.org", socks->address); + + /* SOCKS 5 Send RESOLVE_PTR [F1] for IP address 2.2.2.2 */ + cp = "\x05\xF1\x00\x01\x02\x02\x02\x02"; + write_to_buf(cp, 10, buf); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("2.2.2.2", socks->address); + +done: + ; +} + +/** Helper: Perform SOCKS 5 authentication */ +static void +test_buffers_socks5_no_authenticate_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /*SOCKS 5 No Authentication */ + cp = "\x05\x01\x00"; + write_to_buf(cp, 3, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(SOCKS_NO_AUTH, socks->reply[1]); + + /*SOCKS 5 Send username/password anyway - pretend to be broken */ + cp = "\x01\x02\x01\x01\x02\x01\x01"; + write_to_buf(cp, 7, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + +done: + ; +} + +/** Helper: Perform SOCKS 5 authentication */ +static void +test_buffers_socks5_authenticate_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 5 Negotiate username/password authentication */ + cp = "\x05\x01\x02"; + write_to_buf(cp, 3, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(SOCKS_USER_PASS, socks->reply[1]); + test_eq(5, socks->socks_version); + + /* SOCKS 5 Send username/password */ + cp = "\x01\x02me\x02me"; + write_to_buf(cp, 7, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); +done: + ; +} + /** Run unit tests for buffers.c */ static void test_buffers(void) @@ -206,6 +407,7 @@ test_buffers(void) buf_t *buf = NULL, *buf2 = NULL; const char *cp; + socks_request_t *socks; int j; size_t r; @@ -363,6 +565,41 @@ test_buffers(void) buf_free(buf); buf = NULL; + /* Test fetch_from_buf_socks() */ + buf = buf_new_with_capacity(256); + socks = tor_malloc_zero(sizeof(socks_request_t));; + config_register_addressmaps(get_options()); + + /* A SOCKS 5 client that only supports authentication */ + test_buffers_socks5_authenticate_helper(cp, buf, socks); + test_buffers_socks5_supported_commands_helper(cp, buf, socks); + test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); + + tor_free(socks); + buf_free(buf); + buf = NULL; + buf = buf_new_with_capacity(256); + socks = tor_malloc_zero(sizeof(socks_request_t));; + + /* A SOCKS 5 client that doesn't want authentication */ + test_buffers_socks5_no_authenticate_helper(cp, buf, socks); + test_buffers_socks5_supported_commands_helper(cp, buf, socks); + test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); + + tor_free(socks); + buf_free(buf); + buf = NULL; + buf = buf_new_with_capacity(256); + socks = tor_malloc_zero(sizeof(socks_request_t));; + + /* A SOCKS 4(a) client */ + test_buffers_socks4_supported_commands_helper(cp, buf, socks); + test_buffers_socks4_unsupported_commands_helper(cp, buf, socks); + + tor_free(socks); + buf_free(buf); + buf = NULL; + done: if (buf) buf_free(buf); From f85f52808cd29e50874b233e55b0a0fe98e425f3 Mon Sep 17 00:00:00 2001 From: Robert Hogan Date: Sun, 17 Oct 2010 14:14:51 +0100 Subject: [PATCH 02/14] bug1666 - Pass-through support for SOCKS5 authentication (2) Address Nick's comments: - Refactor against changes in buffers.c - Ensure we have negotiated a method before accepting authentication credentials --- src/or/buffers.c | 28 ++++++++++++++++++---------- src/test/test.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 534f31c17a..5cd76fa0d6 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1642,14 +1642,23 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, case 1: /* socks5: username/password authentication request */ - usernamelen = (unsigned char)*(buf->head->data + 1); - if (buf->datalen < 2u + usernamelen) + if (req->socks_version != 5) { + log_warn(LD_APP, + "socks5: Received authentication attempt before " + "authentication negotiated. Rejecting."); + return -1; + } + usernamelen = (unsigned char)*(data + 1); + if (datalen < 2u + usernamelen) { + *want_length_out = 2u+usernamelen; return 0; - buf_pullup(buf, 2u + usernamelen + 1, 0); - passlen = (unsigned char)*(buf->head->data + 2u + usernamelen); - if (buf->datalen < 2u + usernamelen + 1u + passlen) + } + passlen = (unsigned char)*(data + 2u + usernamelen); + if (datalen < 2u + usernamelen + 1u + passlen) { + *want_length_out = 2u+usernamelen; return 0; - if (buf->datalen > 2u + usernamelen + 1u + passlen) { + } + if (datalen > 2u + usernamelen + 1u + passlen) { log_warn(LD_APP, "socks5: Malformed username/password. Rejecting."); return -1; @@ -1657,9 +1666,9 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->replylen = 2; /* 2 bytes of response */ req->reply[0] = 5; req->reply[1] = 0; /* authentication successful */ - buf_clear(buf); log_debug(LD_APP, "socks5: Accepted username/password without checking."); + *drain_out = 2u + usernamelen + 1u + passlen; return 0; case 5: /* socks5 */ @@ -1672,18 +1681,17 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, *want_length_out = 2u+nummethods; return 0; } - buf_pullup(buf, 2u+nummethods, 0); if (!nummethods) return -1; req->replylen = 2; /* 2 bytes of response */ req->reply[0] = 5; /* socks5 reply */ - if (memchr(buf->head->data+2, SOCKS_NO_AUTH, nummethods)) { + if (memchr(data+2, SOCKS_NO_AUTH, nummethods)) { req->reply[1] = SOCKS_NO_AUTH; /* tell client to use "none" auth method */ req->socks_version = 5; /* remember we've already negotiated auth */ log_debug(LD_APP,"socks5: accepted method 0 (no authentication)"); r=0; - }else if (memchr(buf->head->data+2, SOCKS_USER_PASS,nummethods)) { + }else if (memchr(data+2, SOCKS_USER_PASS,nummethods)) { req->reply[1] = SOCKS_USER_PASS; /* tell client to use "user/pass" auth method */ req->socks_version = 5; /* remember we've already negotiated auth */ diff --git a/src/test/test.c b/src/test/test.c index 68f1aebf7c..8155c3f790 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -197,7 +197,7 @@ free_pregenerated_keys(void) } } -/** Helper: Perform supported SOCKS 5 commands */ +/** Helper: Perform supported SOCKS 4 commands */ static void test_buffers_socks4_unsupported_commands_helper(const char *cp, buf_t *buf, socks_request_t *socks) @@ -214,7 +214,7 @@ done: ; } -/** Helper: Perform supported SOCKS 5 commands */ +/** Helper: Perform supported SOCKS 4 commands */ static void test_buffers_socks4_supported_commands_helper(const char *cp, buf_t *buf, socks_request_t *socks) @@ -398,6 +398,26 @@ done: ; } +/** Helper: Perform SOCKS 5 authentication before method negotiated */ +static void +test_buffers_socks5_auth_before_negotiation_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 5 Send username/password */ + cp = "\x01\x02me\x02me"; + write_to_buf(cp, 7, buf); + test_assert(fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks) == -1); + test_eq(0, socks->socks_version); + test_eq(0, socks->replylen); + test_eq(0, socks->reply[0]); + test_eq(0, socks->reply[1]); + +done: + ; +} + /** Run unit tests for buffers.c */ static void test_buffers(void) @@ -570,6 +590,15 @@ test_buffers(void) socks = tor_malloc_zero(sizeof(socks_request_t));; config_register_addressmaps(get_options()); + /* Sending auth credentials before we've negotiated a method */ + test_buffers_socks5_auth_before_negotiation_helper(cp, buf, socks); + + tor_free(socks); + buf_free(buf); + buf = NULL; + buf = buf_new_with_capacity(256); + socks = tor_malloc_zero(sizeof(socks_request_t));; + /* A SOCKS 5 client that only supports authentication */ test_buffers_socks5_authenticate_helper(cp, buf, socks); test_buffers_socks5_supported_commands_helper(cp, buf, socks); From 8a2f7ef66ac3bee0d6885ec3ca16937fed0703bb Mon Sep 17 00:00:00 2001 From: Robert Hogan Date: Sun, 17 Oct 2010 14:22:24 +0100 Subject: [PATCH 03/14] bug1666 - Pass-through support for SOCKS5 authentication(3) Add changes file. --- changes/bug1666 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug1666 diff --git a/changes/bug1666 b/changes/bug1666 new file mode 100644 index 0000000000..9fd790d4e4 --- /dev/null +++ b/changes/bug1666 @@ -0,0 +1,4 @@ + o Minor features: + - Accept attempts to include a password authenticator in the handshake, as + supported by SOCKS5. This handles SOCKS clients that don't know how to + omit the password when authenticating. Resolves bug 1666. From 02c2d9a4aa2a7ce339e87be9c0c0dc23a6881c14 Mon Sep 17 00:00:00 2001 From: Robert Hogan Date: Tue, 14 Dec 2010 19:59:42 +0000 Subject: [PATCH 04/14] bug1666 - Pass-through support for SOCKS5 authentication(4) Implement nickm's suggestion that we tolerate SOCKS5 clients that send authentication credentials and SOCKS commands all in one go. --- src/or/buffers.c | 5 ----- src/test/test.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 5cd76fa0d6..445376f60e 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1658,11 +1658,6 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, *want_length_out = 2u+usernamelen; return 0; } - if (datalen > 2u + usernamelen + 1u + passlen) { - log_warn(LD_APP, - "socks5: Malformed username/password. Rejecting."); - return -1; - } req->replylen = 2; /* 2 bytes of response */ req->reply[0] = 5; req->reply[1] = 0; /* authentication successful */ diff --git a/src/test/test.c b/src/test/test.c index 8155c3f790..bac73f2709 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -398,6 +398,46 @@ done: ; } +/** Helper: Perform SOCKS 5 authentication and send data all in one go */ +static void +test_buffers_socks5_authenticate_with_data_helper(const char *cp, buf_t *buf, + socks_request_t *socks) +{ + /* SOCKS 5 Negotiate username/password authentication */ + cp = "\x05\x01\x02"; + write_to_buf(cp, 3, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(SOCKS_USER_PASS, socks->reply[1]); + test_eq(5, socks->socks_version); + + /* SOCKS 5 Send username/password */ + /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */ + cp = "\x01\x02me\x02me\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"; + write_to_buf(cp, 17, buf); + test_assert(!fetch_from_buf_socks(buf, socks, + get_options()->TestSocks, + get_options()->SafeSocks)); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("2.2.2.2", socks->address); + test_eq(4369, socks->port); +done: + ; +} + /** Helper: Perform SOCKS 5 authentication before method negotiated */ static void test_buffers_socks5_auth_before_negotiation_helper(const char *cp, buf_t *buf, @@ -610,6 +650,15 @@ test_buffers(void) buf = buf_new_with_capacity(256); socks = tor_malloc_zero(sizeof(socks_request_t));; + /* A SOCKS 5 client that sends credentials and data in one go */ + test_buffers_socks5_authenticate_with_data_helper(cp, buf, socks); + + tor_free(socks); + buf_free(buf); + buf = NULL; + buf = buf_new_with_capacity(256); + socks = tor_malloc_zero(sizeof(socks_request_t));; + /* A SOCKS 5 client that doesn't want authentication */ test_buffers_socks5_no_authenticate_helper(cp, buf, socks); test_buffers_socks5_supported_commands_helper(cp, buf, socks); From 1ed615ded7db0765e8355687bda8b00fdc643e3e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 11:45:15 -0400 Subject: [PATCH 05/14] Correct byte-counting in socks auth parsing code --- src/or/buffers.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 445376f60e..4b8532af09 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1648,14 +1648,19 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, "authentication negotiated. Rejecting."); return -1; } + /* Format is: authversion [1 byte] == 1 + usernamelen [1 byte] + username [usernamelen bytes] + passlen [1 byte] + password [passlen bytes] */ usernamelen = (unsigned char)*(data + 1); - if (datalen < 2u + usernamelen) { - *want_length_out = 2u+usernamelen; + if (datalen < 2u + usernamelen + 1u) { + *want_length_out = 2u + usernamelen + 1u; return 0; } passlen = (unsigned char)*(data + 2u + usernamelen); if (datalen < 2u + usernamelen + 1u + passlen) { - *want_length_out = 2u+usernamelen; + *want_length_out = 2u + usernamelen + 1u + passlen; return 0; } req->replylen = 2; /* 2 bytes of response */ From 9b3f48c0744bd72cb8b25ac7857476181b29481a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 11:47:35 -0400 Subject: [PATCH 06/14] Fix 'make check-spaces' --- src/or/buffers.c | 2 +- src/test/test.c | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 4b8532af09..3490d6dec6 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1691,7 +1691,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->socks_version = 5; /* remember we've already negotiated auth */ log_debug(LD_APP,"socks5: accepted method 0 (no authentication)"); r=0; - }else if (memchr(data+2, SOCKS_USER_PASS,nummethods)) { + } else if (memchr(data+2, SOCKS_USER_PASS,nummethods)) { req->reply[1] = SOCKS_USER_PASS; /* tell client to use "user/pass" auth method */ req->socks_version = 5; /* remember we've already negotiated auth */ diff --git a/src/test/test.c b/src/test/test.c index bac73f2709..d0e24d1b91 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -210,7 +210,7 @@ test_buffers_socks4_unsupported_commands_helper(const char *cp, buf_t *buf, test_eq(4, socks->socks_version); test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ -done: + done: ; } @@ -248,7 +248,7 @@ test_buffers_socks4_supported_commands_helper(const char *cp, buf_t *buf, test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ test_streq("tor.org", socks->address); -done: + done: ; } @@ -279,7 +279,7 @@ test_buffers_socks5_unsupported_commands_helper(const char *cp, buf_t *buf, test_eq(0, socks->reply[1]); /* XXX: shouldn't tor reply 'command not supported' [07]? */ -done: + done: ; } @@ -334,7 +334,7 @@ test_buffers_socks5_supported_commands_helper(const char *cp, buf_t *buf, test_eq(0, socks->reply[1]); test_streq("2.2.2.2", socks->address); -done: + done: ; } @@ -364,7 +364,7 @@ test_buffers_socks5_no_authenticate_helper(const char *cp, buf_t *buf, test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); -done: + done: ; } @@ -394,7 +394,7 @@ test_buffers_socks5_authenticate_helper(const char *cp, buf_t *buf, test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); -done: + done: ; } @@ -434,7 +434,7 @@ test_buffers_socks5_authenticate_with_data_helper(const char *cp, buf_t *buf, test_eq(0, socks->reply[1]); test_streq("2.2.2.2", socks->address); test_eq(4369, socks->port); -done: + done: ; } @@ -454,7 +454,7 @@ test_buffers_socks5_auth_before_negotiation_helper(const char *cp, buf_t *buf, test_eq(0, socks->reply[0]); test_eq(0, socks->reply[1]); -done: + done: ; } From aec396d9d0c2db18d44c9ad850bedbc01d50e40a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 12:00:00 -0400 Subject: [PATCH 07/14] Be more strict about when to accept socks auth message In the code as it stood, we would accept any number of socks5 username/password authentication messages, regardless of whether we had actually negotiated username/password authentication. Instead, we should only accept one, and only if we have really negotiated username/password authentication. This patch also makes some fields of socks_request_t into uint8_t, for safety. --- src/or/buffers.c | 36 +++++++++++++++++++++--------------- src/or/or.h | 12 +++++++++--- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 3490d6dec6..bbd60c4357 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1636,18 +1636,12 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, unsigned char usernamelen, passlen; struct in_addr in; - socksver = *data; - - switch (socksver) { /* which version of socks? */ - - case 1: /* socks5: username/password authentication request */ - - if (req->socks_version != 5) { - log_warn(LD_APP, - "socks5: Received authentication attempt before " - "authentication negotiated. Rejecting."); - return -1; - } + if (req->socks_version == 5 && !req->got_auth) { + /* See if we have received authentication. Strictly speaking, we should + also check whether we actually negotiated username/password + authentication. But some broken clients will send us authentication + even if we negotiated SOCKS_NO_AUTH. */ + if (*data == 1) { /* username/pass version 1 */ /* Format is: authversion [1 byte] == 1 usernamelen [1 byte] username [usernamelen bytes] @@ -1669,8 +1663,19 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, log_debug(LD_APP, "socks5: Accepted username/password without checking."); *drain_out = 2u + usernamelen + 1u + passlen; + req->got_auth = 1; return 0; + } else if (req->auth_type == SOCKS_USER_PASS) { + /* unknown version byte */ + log_warn(LD_APP, "Socks5 username/password version %d not recognized; " + "rejecting.", (int)*data); + return -1; + } + } + socksver = *data; + + switch (socksver) { /* which version of socks? */ case 5: /* socks5 */ if (req->socks_version != 5) { /* we need to negotiate a method */ @@ -1691,7 +1696,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->socks_version = 5; /* remember we've already negotiated auth */ log_debug(LD_APP,"socks5: accepted method 0 (no authentication)"); r=0; - } else if (memchr(data+2, SOCKS_USER_PASS,nummethods)) { + } else if (memchr(data+2, SOCKS_USER_PASS, nummethods)) { + req->auth_type = SOCKS_USER_PASS; req->reply[1] = SOCKS_USER_PASS; /* tell client to use "user/pass" auth method */ req->socks_version = 5; /* remember we've already negotiated auth */ @@ -1911,7 +1917,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, case 'H': /* head */ case 'P': /* put/post */ case 'C': /* connect */ - strlcpy(req->reply, + strlcpy((char*)req->reply, "HTTP/1.0 501 Tor is not an HTTP Proxy\r\n" "Content-Type: text/html; charset=iso-8859-1\r\n\r\n" "\n" @@ -1937,7 +1943,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, "\n" "\n" , MAX_SOCKS_REPLY_LEN); - req->replylen = strlen(req->reply)+1; + req->replylen = strlen((char*)req->reply)+1; /* fall through */ default: /* version is not socks4 or socks5 */ log_warn(LD_APP, diff --git a/src/or/or.h b/src/or/or.h index 6ae62e8899..bc80e3944c 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3088,10 +3088,15 @@ struct socks_request_t { /** Which version of SOCKS did the client use? One of "0, 4, 5" -- where * 0 means that no socks handshake ever took place, and this is just a * stub connection (e.g. see connection_ap_make_link()). */ - char socks_version; - int command; /**< What is this stream's goal? One from the above list. */ + uint8_t socks_version; + /** If using socks5 authentication, which authentication type did we + * negotiate? currently we support 0 (no authentication) and 2 + * (username/password). */ + uint8_t auth_type; + /** What is this stream's goal? One of the SOCKS_COMMAND_* values */ + uint8_t command; size_t replylen; /**< Length of reply. */ - char reply[MAX_SOCKS_REPLY_LEN]; /**< Write an entry into this string if + uint8_t reply[MAX_SOCKS_REPLY_LEN]; /**< Write an entry into this string if * we want to specify our own socks reply, * rather than using the default socks4 or * socks5 socks reply. We use this for the @@ -3103,6 +3108,7 @@ struct socks_request_t { unsigned int has_finished : 1; /**< Has the SOCKS handshake finished? Used to * make sure we send back a socks reply for * every connection. */ + unsigned int got_auth : 1; /**< Have we received any authentication data? */ }; /********************************* circuitbuild.c **********************/ From 204bce7e3ca0f60cfec1d8be700848309f605abd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 12:16:09 -0400 Subject: [PATCH 08/14] If we negotiate authentication, require it. --- src/or/buffers.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/or/buffers.c b/src/or/buffers.c index bbd60c4357..7b212a7d33 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1716,6 +1716,11 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, return r; } + if (req->auth_type != SOCKS_NO_AUTH && !req->got_auth) { + log_warn(LD_APP, + "socks5: negotiated authentication, but none provided"); + return -1; + } /* we know the method; read in the request */ log_debug(LD_APP,"socks5: checking request"); if (datalen < 8) {/* basic info plus >=2 for addr plus 2 for port */ From 2e6604f42ee614156ceeb29bdcab5c78cc1d84ba Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 12:31:17 -0400 Subject: [PATCH 09/14] Record username/password data in socks_request_t This change also requires us to add and use a pair of allocator/deallocator functions for socks_request_t, instead of using tor_malloc_zero/tor_free directly. --- src/or/buffers.c | 43 +++++++++++++++++++++++++++++++++++++++---- src/or/buffers.h | 2 ++ src/or/connection.c | 8 +++----- src/or/or.h | 12 ++++++++++++ src/test/test.c | 20 ++++++++++---------- 5 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 7b212a7d33..0e2dbbeb04 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1475,6 +1475,25 @@ log_unsafe_socks_warning(int socks_protocol, const char *address, socks_protocol, address, (int)port); } +/** Return a new socks_request_t. */ +socks_request_t * +socks_request_new(void) +{ + return tor_malloc_zero(sizeof(socks_request_t)); +} + +/** Free all storage held in the socks_request_t req. */ +void +socks_request_free(socks_request_t *req) +{ + if (!req) + return; + tor_free(req->username); + tor_free(req->password); + memset(req, 0xCC, sizeof(socks_request_t)); + tor_free(req); +} + /** There is a (possibly incomplete) socks handshake on buf, of one * of the forms * - socks4: "socksheader username\\0" @@ -1631,7 +1650,6 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, tor_addr_t destaddr; uint32_t destip; uint8_t socksver; - enum {socks4, socks4a} socks4_prot = socks4a; char *next, *startaddr; unsigned char usernamelen, passlen; struct in_addr in; @@ -1662,6 +1680,14 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->reply[1] = 0; /* authentication successful */ log_debug(LD_APP, "socks5: Accepted username/password without checking."); + if (usernamelen) { + req->username = tor_memdup(data+2u, usernamelen); + req->usernamelen = usernamelen; + } + if (passlen) { + req->password = tor_memdup(data+3u+usernamelen, passlen); + req->passwordlen = passlen; + } *drain_out = 2u + usernamelen + 1u + passlen; req->got_auth = 1; return 0; @@ -1813,7 +1839,9 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, return -1; } tor_assert(0); - case 4: /* socks4 */ + case 4: { /* socks4 */ + enum {socks4, socks4a} socks4_prot = socks4a; + const char *authstart, *authend; /* http://ss5.sourceforge.net/socks4.protocol.txt */ /* http://ss5.sourceforge.net/socks4A.protocol.txt */ @@ -1854,7 +1882,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, socks4_prot = socks4; } - next = memchr(data+SOCKS4_NETWORK_LEN, 0, + authstart = data + SOCKS4_NETWORK_LEN; + next = memchr(authstart, 0, datalen-SOCKS4_NETWORK_LEN); if (!next) { if (datalen >= 1024) { @@ -1865,6 +1894,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, *want_length_out = datalen+1024; /* ???? */ return 0; } + authend = next; tor_assert(next < data+datalen); startaddr = NULL; @@ -1914,10 +1944,15 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->port, escaped(req->address)); return -1; } + if (authend != authstart) { + req->got_auth = 1; + req->usernamelen = authend - authstart; + req->username = tor_memdup(authstart, authend - authstart); + } /* next points to the final \0 on inbuf */ *drain_out = next - data + 1; return 1; - + } case 'G': /* get */ case 'H': /* head */ case 'P': /* put/post */ diff --git a/src/or/buffers.h b/src/or/buffers.h index 35c1dd2ea5..fad509e4a1 100644 --- a/src/or/buffers.h +++ b/src/or/buffers.h @@ -41,6 +41,8 @@ int fetch_from_buf_http(buf_t *buf, char **headers_out, size_t max_headerlen, char **body_out, size_t *body_used, size_t max_bodylen, int force_complete); +socks_request_t *socks_request_new(void); +void socks_request_free(socks_request_t *req); int fetch_from_buf_socks(buf_t *buf, socks_request_t *req, int log_sockstype, int safe_socks); int fetch_from_buf_socks_client(buf_t *buf, int state, char **reason); diff --git a/src/or/connection.c b/src/or/connection.c index 6f9ae20a78..2a4a6f7626 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -246,7 +246,7 @@ edge_connection_new(int type, int socket_family) tor_assert(type == CONN_TYPE_EXIT || type == CONN_TYPE_AP); connection_init(time(NULL), TO_CONN(edge_conn), type, socket_family); if (type == CONN_TYPE_AP) - edge_conn->socks_request = tor_malloc_zero(sizeof(socks_request_t)); + edge_conn->socks_request = socks_request_new(); return edge_conn; } @@ -440,10 +440,8 @@ _connection_free(connection_t *conn) if (CONN_IS_EDGE(conn)) { edge_connection_t *edge_conn = TO_EDGE_CONN(conn); tor_free(edge_conn->chosen_exit_name); - if (edge_conn->socks_request) { - memset(edge_conn->socks_request, 0xcc, sizeof(socks_request_t)); - tor_free(edge_conn->socks_request); - } + if (edge_conn->socks_request) + socks_request_free(edge_conn->socks_request); rend_data_free(edge_conn->rend_data); } diff --git a/src/or/or.h b/src/or/or.h index bc80e3944c..3c764440ea 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3109,6 +3109,18 @@ struct socks_request_t { * make sure we send back a socks reply for * every connection. */ unsigned int got_auth : 1; /**< Have we received any authentication data? */ + + /** Number of bytes in username; 0 if username is NULL */ + uint8_t usernamelen; + /** Number of bytes in password; 0 if password is NULL */ + uint8_t passwordlen; + /** The negotiated username value if any (for socks5), or the entire + * authentication string (for socks4). This value is NOT nul-terminated; + * see usernamelen for its length. */ + char *username; + /** The negotiated password value if any (for socks5). This value is NOT + * nul-terminated; see usernamelen for its length. */ + char *password; }; /********************************* circuitbuild.c **********************/ diff --git a/src/test/test.c b/src/test/test.c index d0e24d1b91..38729cf2aa 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -627,54 +627,54 @@ test_buffers(void) /* Test fetch_from_buf_socks() */ buf = buf_new_with_capacity(256); - socks = tor_malloc_zero(sizeof(socks_request_t));; + socks = socks_request_new(); config_register_addressmaps(get_options()); /* Sending auth credentials before we've negotiated a method */ test_buffers_socks5_auth_before_negotiation_helper(cp, buf, socks); - tor_free(socks); + socks_request_free(socks); buf_free(buf); buf = NULL; buf = buf_new_with_capacity(256); - socks = tor_malloc_zero(sizeof(socks_request_t));; + socks = socks_request_new(); /* A SOCKS 5 client that only supports authentication */ test_buffers_socks5_authenticate_helper(cp, buf, socks); test_buffers_socks5_supported_commands_helper(cp, buf, socks); test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); - tor_free(socks); + socks_request_free(socks); buf_free(buf); buf = NULL; buf = buf_new_with_capacity(256); - socks = tor_malloc_zero(sizeof(socks_request_t));; + socks = socks_request_new(); /* A SOCKS 5 client that sends credentials and data in one go */ test_buffers_socks5_authenticate_with_data_helper(cp, buf, socks); - tor_free(socks); + socks_request_free(socks); buf_free(buf); buf = NULL; buf = buf_new_with_capacity(256); - socks = tor_malloc_zero(sizeof(socks_request_t));; + socks = socks_request_new(); /* A SOCKS 5 client that doesn't want authentication */ test_buffers_socks5_no_authenticate_helper(cp, buf, socks); test_buffers_socks5_supported_commands_helper(cp, buf, socks); test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); - tor_free(socks); + socks_request_free(socks); buf_free(buf); buf = NULL; buf = buf_new_with_capacity(256); - socks = tor_malloc_zero(sizeof(socks_request_t));; + socks = socks_request_new(); /* A SOCKS 4(a) client */ test_buffers_socks4_supported_commands_helper(cp, buf, socks); test_buffers_socks4_unsupported_commands_helper(cp, buf, socks); - tor_free(socks); + socks_request_free(socks); buf_free(buf); buf = NULL; From ee42fe8fbb85504ceef9335e2854c35e6163a8c3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 17:29:33 -0400 Subject: [PATCH 10/14] Don't drain extra data when parsing socks auth methods We added this back in 0649fa14 in 2006, to deal with the case where the client unconditionally sent us authentication data. Hopefully, that's not needed any longer, since we now can actually parse authentication data. --- src/or/buffers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 0e2dbbeb04..44a492addc 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1736,9 +1736,9 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->reply[1] = '\xFF'; /* reject all methods */ r=-1; } - /* remove packet from buf. also remove any other extraneous - * bytes, to support broken socks clients. */ - *drain_out = -1; + /* Remove packet from buf. Some SOCKS clients will have sent extra + * junk at this point; let's hope it's an authentication message. */ + *drain_out = 2u + nummethods; return r; } From 5d43a1572094868e598541f9ce686546c910f071 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 17:30:55 -0400 Subject: [PATCH 11/14] Refactor socks unit tests into a suite of their own --- src/test/test.c | 364 +++++++++++++++++++++++++++++------------------- 1 file changed, 220 insertions(+), 144 deletions(-) diff --git a/src/test/test.c b/src/test/test.c index 38729cf2aa..64916eab21 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -197,14 +197,59 @@ free_pregenerated_keys(void) } } -/** Helper: Perform supported SOCKS 4 commands */ -static void -test_buffers_socks4_unsupported_commands_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +typedef struct socks_test_data_t { + socks_request_t *req; + buf_t *buf; +} socks_test_data_t; + +static void * +socks_test_setup(const struct testcase_t *testcase) { + socks_test_data_t *data = tor_malloc(sizeof(socks_test_data_t)); + (void)testcase; + data->buf = buf_new_with_capacity(256); + data->req = socks_request_new(); + config_register_addressmaps(get_options()); + return data; +} +static int +socks_test_cleanup(const struct testcase_t *testcase, void *ptr) +{ + socks_test_data_t *data = ptr; + (void)testcase; + buf_free(data->buf); + socks_request_free(data->req); + tor_free(data); + return 1; +} + +const struct testcase_setup_t socks_setup = { + socks_test_setup, socks_test_cleanup +}; + +#define SOCKS_TEST_INIT() \ + socks_test_data_t *testdata = ptr; \ + buf_t *buf = testdata->buf; \ + socks_request_t *socks = testdata->req; +#define ADD_DATA(buf, s) \ + write_to_buf(s, sizeof(s)-1, buf) + +static void +socks_request_clear(socks_request_t *socks) +{ + tor_free(socks->username); + tor_free(socks->password); + memset(socks, 0, sizeof(socks_request_t)); +} + +/** Perform unsupported SOCKS 4 commands */ +static void +test_socks_4_unsupported_commands(void *ptr) +{ + SOCKS_TEST_INIT(); + /* SOCKS 4 Send BIND [02] to IP address 2.2.2.2:4369 */ - cp = "\x04\x02\x11\x11\x02\x02\x02\x02\x00"; - write_to_buf(cp, 9, buf); + ADD_DATA(buf, "\x04\x02\x11\x11\x02\x02\x02\x02\x00"); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == -1); test_eq(4, socks->socks_version); @@ -214,138 +259,186 @@ test_buffers_socks4_unsupported_commands_helper(const char *cp, buf_t *buf, ; } -/** Helper: Perform supported SOCKS 4 commands */ +/** Perform supported SOCKS 4 commands */ static void -test_buffers_socks4_supported_commands_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_4_supported_commands(void *ptr) { - /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4369 */ - cp = "\x04\x01\x11\x11\x02\x02\x02\x02\x00"; - write_to_buf(cp, 9, buf); + SOCKS_TEST_INIT(); + + test_eq(0, buf_datalen(buf)); + + /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4370 */ + ADD_DATA(buf, "\x04\x01\x11\x12\x02\x02\x02\x03\x00"); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(4, socks->socks_version); test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ - test_streq("2.2.2.2", socks->address); - test_eq(4369, socks->port); + test_eq(SOCKS_COMMAND_CONNECT, socks->command); + test_streq("2.2.2.3", socks->address); + test_eq(4370, socks->port); + test_assert(socks->got_auth == 0); + test_assert(! socks->username); + + test_eq(0, buf_datalen(buf)); + socks_request_clear(socks); /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4369 with userid*/ - cp = "\x04\x01\x11\x11\x02\x02\x02\x02\x02me\x00"; - write_to_buf(cp, 12, buf); + ADD_DATA(buf, "\x04\x01\x11\x12\x02\x02\x02\x04me\x00"); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(4, socks->socks_version); test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ - test_streq("2.2.2.2", socks->address); - test_eq(4369, socks->port); + test_eq(SOCKS_COMMAND_CONNECT, socks->command); + test_streq("2.2.2.4", socks->address); + test_eq(4370, socks->port); + test_assert(socks->got_auth == 1); + test_assert(socks->username); + test_eq(2, socks->usernamelen); + test_memeq("me", socks->username, 2); - /* SOCKS 4a Send RESOLVE [F0] request for torproject.org:4369 */ - cp = "\x04\xF0\x01\x01\x00\x00\x00\x02\x02me\x00tor.org\x00"; - write_to_buf(cp, 20, buf); + test_eq(0, buf_datalen(buf)); + socks_request_clear(socks); + + /* SOCKS 4a Send RESOLVE [F0] request for torproject.org */ + ADD_DATA(buf, "\x04\xF0\x01\x01\x00\x00\x00\x02me\x00torproject.org\x00"); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks)); + get_options()->SafeSocks) == 1); test_eq(4, socks->socks_version); test_eq(0, socks->replylen); /* XXX: shouldn't tor reply? */ - test_streq("tor.org", socks->address); + test_streq("torproject.org", socks->address); + + test_eq(0, buf_datalen(buf)); done: ; } -/** Helper: Perform supported SOCKS 5 commands */ +/** Perform unsupported SOCKS 5 commands */ static void -test_buffers_socks5_unsupported_commands_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_unsupported_commands(void *ptr) { + SOCKS_TEST_INIT(); + /* SOCKS 5 Send unsupported BIND [02] command */ - cp = "\x05\x02\x00\x01\x02\x02\x02\x02\x01\x01"; - write_to_buf(cp, 10, buf); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks) == -1); + ADD_DATA(buf, "\x05\x02\x00\x01"); + + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 0); + test_eq(0, buf_datalen(buf)); test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); - test_eq(0, socks->reply[1]); /* XXX: shouldn't tor reply 'command - not supported' [07]? */ + test_eq(0, socks->reply[1]); + ADD_DATA(buf, "\x05\x02\x00\x01\x02\x02\x02\x01\x01\x01"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), -1); + /* XXX: shouldn't tor reply 'command not supported' [07]? */ + + buf_clear(buf); + socks_request_clear(socks); /* SOCKS 5 Send unsupported UDP_ASSOCIATE [03] command */ - cp = "\x05\x03\x00\x01\x02\x02\x02\x02\x01\x01"; - write_to_buf(cp, 10, buf); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks) == -1); + ADD_DATA(buf, "\x05\x03\x00\x01\x02"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 0); test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); - test_eq(0, socks->reply[1]); /* XXX: shouldn't tor reply 'command - not supported' [07]? */ + test_eq(0, socks->reply[1]); + ADD_DATA(buf, "\x05\x03\x00\x01\x02\x02\x02\x01\x01\x01"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), -1); + /* XXX: shouldn't tor reply 'command not supported' [07]? */ done: ; } -/** Helper: Perform supported SOCKS 5 commands */ +/** Perform supported SOCKS 5 commands */ static void -test_buffers_socks5_supported_commands_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_supported_commands(void *ptr) { + SOCKS_TEST_INIT(); + /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */ - cp = "\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"; - write_to_buf(cp, 10, buf); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks) == 1); + ADD_DATA(buf, "\x05\x01\x00"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 0); test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); + + ADD_DATA(buf, "\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 1); test_streq("2.2.2.2", socks->address); test_eq(4369, socks->port); + test_eq(0, buf_datalen(buf)); + socks_request_clear(socks); + /* SOCKS 5 Send CONNECT [01] to FQDN torproject.org:4369 */ - cp = "\x05\x01\x00\x03\x07tor.org\x11\x11"; - write_to_buf(cp, 14, buf); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks)); + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\x01\x00\x03\x0Etorproject.org\x11\x11"); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 0); + test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks), 1); + test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); - test_streq("tor.org", socks->address); + test_streq("torproject.org", socks->address); test_eq(4369, socks->port); - /* SOCKS 5 Send RESOLVE [F0] request for torproject.org:4369 */ - cp = "\x05\xF0\x00\x03\x07tor.org"; - write_to_buf(cp, 14, buf); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks)); - test_eq(5, socks->socks_version); - test_eq(2, socks->replylen); - test_eq(5, socks->reply[0]); - test_eq(0, socks->reply[1]); - test_streq("tor.org", socks->address); + test_eq(0, buf_datalen(buf)); + socks_request_clear(socks); - /* SOCKS 5 Send RESOLVE_PTR [F1] for IP address 2.2.2.2 */ - cp = "\x05\xF1\x00\x01\x02\x02\x02\x02"; - write_to_buf(cp, 10, buf); + /* SOCKS 5 Send RESOLVE [F0] request for torproject.org:4369 */ + ADD_DATA(buf, "\x05\x01\x00"); + ADD_DATA(buf, "\x05\xF0\x00\x03\x0Etorproject.org\x01\x02"); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 0); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); - test_streq("2.2.2.2", socks->address); + test_streq("torproject.org", socks->address); + + test_eq(0, buf_datalen(buf)); + 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"); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 0); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, + get_options()->SafeSocks) == 1); + test_eq(5, socks->socks_version); + test_eq(2, socks->replylen); + test_eq(5, socks->reply[0]); + test_eq(0, socks->reply[1]); + test_streq("2.2.2.5", socks->address); + + test_eq(0, buf_datalen(buf)); done: ; } -/** Helper: Perform SOCKS 5 authentication */ +/** Perform SOCKS 5 authentication */ static void -test_buffers_socks5_no_authenticate_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_no_authenticate(void *ptr) { + SOCKS_TEST_INIT(); + /*SOCKS 5 No Authentication */ - cp = "\x05\x01\x00"; - write_to_buf(cp, 3, buf); + ADD_DATA(buf,"\x05\x01\x00"); test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -353,9 +446,10 @@ test_buffers_socks5_no_authenticate_helper(const char *cp, buf_t *buf, test_eq(5, socks->reply[0]); test_eq(SOCKS_NO_AUTH, socks->reply[1]); + test_eq(0, buf_datalen(buf)); + /*SOCKS 5 Send username/password anyway - pretend to be broken */ - cp = "\x01\x02\x01\x01\x02\x01\x01"; - write_to_buf(cp, 7, buf); + ADD_DATA(buf,"\x01\x02\x01\x01\x02\x01\x01"); test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -364,18 +458,25 @@ test_buffers_socks5_no_authenticate_helper(const char *cp, buf_t *buf, test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); + test_eq(2, socks->usernamelen); + test_eq(2, socks->passwordlen); + + test_memeq("\x01\x01", socks->username, 2); + test_memeq("\x01\x01", socks->password, 2); + done: ; } -/** Helper: Perform SOCKS 5 authentication */ +/** Perform SOCKS 5 authentication */ static void -test_buffers_socks5_authenticate_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_authenticate(void *ptr) { + SOCKS_TEST_INIT(); + /* SOCKS 5 Negotiate username/password authentication */ - cp = "\x05\x01\x02"; - write_to_buf(cp, 3, buf); + ADD_DATA(buf, "\x05\x01\x02"); + test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -384,9 +485,10 @@ test_buffers_socks5_authenticate_helper(const char *cp, buf_t *buf, test_eq(SOCKS_USER_PASS, socks->reply[1]); test_eq(5, socks->socks_version); + test_eq(0, buf_datalen(buf)); + /* SOCKS 5 Send username/password */ - cp = "\x01\x02me\x02me"; - write_to_buf(cp, 7, buf); + ADD_DATA(buf, "\x01\x02me\x08mypasswd"); test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -394,18 +496,26 @@ test_buffers_socks5_authenticate_helper(const char *cp, buf_t *buf, test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); + + test_eq(2, socks->usernamelen); + test_eq(8, socks->passwordlen); + + test_memeq("me", socks->username, 2); + test_memeq("mypasswd", socks->password, 8); + done: ; } -/** Helper: Perform SOCKS 5 authentication and send data all in one go */ +/** Perform SOCKS 5 authentication and send data all in one go */ static void -test_buffers_socks5_authenticate_with_data_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_authenticate_with_data(void *ptr) { + SOCKS_TEST_INIT(); + /* SOCKS 5 Negotiate username/password authentication */ - cp = "\x05\x01\x02"; - write_to_buf(cp, 3, buf); + ADD_DATA(buf, "\x05\x01\x02"); + test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -414,10 +524,11 @@ test_buffers_socks5_authenticate_with_data_helper(const char *cp, buf_t *buf, test_eq(SOCKS_USER_PASS, socks->reply[1]); test_eq(5, socks->socks_version); + test_eq(0, buf_datalen(buf)); + /* SOCKS 5 Send username/password */ /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */ - cp = "\x01\x02me\x02me\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"; - write_to_buf(cp, 17, buf); + ADD_DATA(buf, "\x01\x02me\x02me\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); test_assert(!fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks)); @@ -434,18 +545,19 @@ test_buffers_socks5_authenticate_with_data_helper(const char *cp, buf_t *buf, test_eq(0, socks->reply[1]); test_streq("2.2.2.2", socks->address); test_eq(4369, socks->port); + done: ; } -/** Helper: Perform SOCKS 5 authentication before method negotiated */ +/** Perform SOCKS 5 authentication before method negotiated */ static void -test_buffers_socks5_auth_before_negotiation_helper(const char *cp, buf_t *buf, - socks_request_t *socks) +test_socks_5_auth_before_negotiation(void *ptr) { + SOCKS_TEST_INIT(); + /* SOCKS 5 Send username/password */ - cp = "\x01\x02me\x02me"; - write_to_buf(cp, 7, buf); + ADD_DATA(buf, "\x01\x02me\x02me"); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == -1); @@ -467,7 +579,6 @@ test_buffers(void) buf_t *buf = NULL, *buf2 = NULL; const char *cp; - socks_request_t *socks; int j; size_t r; @@ -625,59 +736,6 @@ test_buffers(void) buf_free(buf); buf = NULL; - /* Test fetch_from_buf_socks() */ - buf = buf_new_with_capacity(256); - socks = socks_request_new(); - config_register_addressmaps(get_options()); - - /* Sending auth credentials before we've negotiated a method */ - test_buffers_socks5_auth_before_negotiation_helper(cp, buf, socks); - - socks_request_free(socks); - buf_free(buf); - buf = NULL; - buf = buf_new_with_capacity(256); - socks = socks_request_new(); - - /* A SOCKS 5 client that only supports authentication */ - test_buffers_socks5_authenticate_helper(cp, buf, socks); - test_buffers_socks5_supported_commands_helper(cp, buf, socks); - test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); - - socks_request_free(socks); - buf_free(buf); - buf = NULL; - buf = buf_new_with_capacity(256); - socks = socks_request_new(); - - /* A SOCKS 5 client that sends credentials and data in one go */ - test_buffers_socks5_authenticate_with_data_helper(cp, buf, socks); - - socks_request_free(socks); - buf_free(buf); - buf = NULL; - buf = buf_new_with_capacity(256); - socks = socks_request_new(); - - /* A SOCKS 5 client that doesn't want authentication */ - test_buffers_socks5_no_authenticate_helper(cp, buf, socks); - test_buffers_socks5_supported_commands_helper(cp, buf, socks); - test_buffers_socks5_unsupported_commands_helper(cp, buf, socks); - - socks_request_free(socks); - buf_free(buf); - buf = NULL; - buf = buf_new_with_capacity(256); - socks = socks_request_new(); - - /* A SOCKS 4(a) client */ - test_buffers_socks4_supported_commands_helper(cp, buf, socks); - test_buffers_socks4_unsupported_commands_helper(cp, buf, socks); - - socks_request_free(socks); - buf_free(buf); - buf = NULL; - done: if (buf) buf_free(buf); @@ -1564,6 +1622,23 @@ static struct testcase_t test_array[] = { END_OF_TESTCASES }; +#define SOCKSENT(name) \ + { #name, test_socks_##name, TT_FORK, &socks_setup, NULL } + +static struct testcase_t socks_tests[] = { + SOCKSENT(4_unsupported_commands), + SOCKSENT(4_supported_commands), + + SOCKSENT(5_unsupported_commands), + SOCKSENT(5_supported_commands), + SOCKSENT(5_no_authenticate), + SOCKSENT(5_auth_before_negotiation), + SOCKSENT(5_authenticate), + SOCKSENT(5_authenticate_with_data), + + END_OF_TESTCASES +}; + extern struct testcase_t addr_tests[]; extern struct testcase_t crypto_tests[]; extern struct testcase_t container_tests[]; @@ -1573,6 +1648,7 @@ extern struct testcase_t microdesc_tests[]; static struct testgroup_t testgroups[] = { { "", test_array }, + { "socks/", socks_tests }, { "addr/", addr_tests }, { "crypto/", crypto_tests }, { "container/", container_tests }, From 05c424f4b830e98595b33397b58504462dbda8db Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Jun 2011 17:44:29 -0400 Subject: [PATCH 12/14] Refactor fetch_from_buf_socks() to be greedy Previously, fetch_from_buf_socks() might return 0 if there was still data on the buffer and a subsequent call to fetch_from_buf_socks() would return 1. This was making some of the socks5 unit tests harder to write, and could potentially have caused misbehavior with some overly verbose SOCKS implementations. Now, fetch_from_buf_socks() does as much processing as it can, and returns 0 only if it really needs more data. This brings it into line with the evbuffer socks implementation. --- src/or/buffers.c | 13 ++++++------- src/or/connection_edge.c | 23 ++++++++++------------- src/test/test.c | 23 ++++++++--------------- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 44a492addc..1310b4215b 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1544,9 +1544,7 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req, else if (n_drain > 0) buf_remove_from_front(buf, n_drain); - } while (res == 0 && buf->head && - buf->datalen > buf->head->datalen && - want_length < buf->head->datalen); + } while (res == 0 && buf->head && want_length < buf->datalen); return res; } @@ -1690,6 +1688,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, } *drain_out = 2u + usernamelen + 1u + passlen; req->got_auth = 1; + *want_length_out = 7; /* Minimal socks5 sommand. */ return 0; } else if (req->auth_type == SOCKS_USER_PASS) { /* unknown version byte */ @@ -1749,8 +1748,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, } /* we know the method; read in the request */ log_debug(LD_APP,"socks5: checking request"); - if (datalen < 8) {/* basic info plus >=2 for addr plus 2 for port */ - *want_length_out = 8; + if (datalen < 7) {/* basic info plus >=1 for addr plus 2 for port */ + *want_length_out = 7; return 0; /* not yet */ } req->command = (unsigned char) *(data+1); @@ -1891,7 +1890,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, return -1; } log_debug(LD_APP,"socks4: Username not here yet."); - *want_length_out = datalen+1024; /* ???? */ + *want_length_out = datalen+1024; /* More than we need, but safe */ return 0; } authend = next; @@ -1919,7 +1918,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, return -1; } log_debug(LD_APP,"socks4: Destaddr not all here yet."); - *want_length_out = datalen + 1024; + *want_length_out = datalen + 1024; /* More than we need, but safe */ return 0; } if (MAX_SOCKS_ADDR_LEN <= next-startaddr) { diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 4e988bd04c..bff73d4a9e 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1883,6 +1883,7 @@ connection_ap_handshake_process_socks(edge_connection_t *conn) socks_request_t *socks; int sockshere; or_options_t *options = get_options(); + int had_reply = 0; tor_assert(conn); tor_assert(conn->_base.type == CONN_TYPE_AP); @@ -1900,22 +1901,18 @@ connection_ap_handshake_process_socks(edge_connection_t *conn) sockshere = fetch_from_buf_socks(conn->_base.inbuf, socks, options->TestSocks, options->SafeSocks); }; + + if (socks->replylen) { + had_reply = 1; + connection_write_to_buf(socks->reply, socks->replylen, TO_CONN(conn)); + socks->replylen = 0; + } + if (sockshere == 0) { - if (socks->replylen) { - connection_write_to_buf(socks->reply, socks->replylen, TO_CONN(conn)); - /* zero it out so we can do another round of negotiation */ - socks->replylen = 0; - } else { - log_debug(LD_APP,"socks handshake not all here yet."); - } + log_debug(LD_APP,"socks handshake not all here yet."); return 0; } else if (sockshere == -1) { - if (socks->replylen) { /* we should send reply back */ - log_debug(LD_APP,"reply is already set for us. Using it."); - connection_ap_handshake_socks_reply(conn, socks->reply, socks->replylen, - END_STREAM_REASON_SOCKSPROTOCOL); - - } else { + if (!had_reply) { log_warn(LD_APP,"Fetching socks handshake failed. Closing."); connection_ap_handshake_socks_reply(conn, NULL, 0, END_STREAM_REASON_SOCKSPROTOCOL); diff --git a/src/test/test.c b/src/test/test.c index 64916eab21..01fb30cd0c 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -381,8 +381,6 @@ test_socks_5_supported_commands(void *ptr) /* SOCKS 5 Send CONNECT [01] to FQDN torproject.org:4369 */ ADD_DATA(buf, "\x05\x01\x00"); ADD_DATA(buf, "\x05\x01\x00\x03\x0Etorproject.org\x11\x11"); - test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks), 0); test_eq(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks), 1); @@ -399,8 +397,6 @@ test_socks_5_supported_commands(void *ptr) /* SOCKS 5 Send RESOLVE [F0] request for torproject.org:4369 */ ADD_DATA(buf, "\x05\x01\x00"); ADD_DATA(buf, "\x05\xF0\x00\x03\x0Etorproject.org\x01\x02"); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks) == 0); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(5, socks->socks_version); @@ -415,8 +411,6 @@ test_socks_5_supported_commands(void *ptr) /* 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"); - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks) == 0); test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(5, socks->socks_version); @@ -528,24 +522,23 @@ test_socks_5_authenticate_with_data(void *ptr) /* SOCKS 5 Send username/password */ /* SOCKS 5 Send CONNECT [01] to IP address 2.2.2.2:4369 */ - ADD_DATA(buf, "\x01\x02me\x02me\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); - test_assert(!fetch_from_buf_socks(buf, socks, + ADD_DATA(buf, "\x01\x02me\x03you\x05\x01\x00\x01\x02\x02\x02\x02\x11\x11"); + test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, - get_options()->SafeSocks)); - test_eq(5, socks->socks_version); - test_eq(2, socks->replylen); - test_eq(5, socks->reply[0]); - test_eq(0, socks->reply[1]); - - test_assert(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks) == 1); test_eq(5, socks->socks_version); test_eq(2, socks->replylen); test_eq(5, socks->reply[0]); test_eq(0, socks->reply[1]); + test_streq("2.2.2.2", socks->address); test_eq(4369, socks->port); + test_eq(2, socks->usernamelen); + test_eq(3, socks->passwordlen); + test_memeq("me", socks->username, 2); + test_memeq("you", socks->password, 3); + done: ; } From 4ce302c8e5f19e0ac4417f2a75e206b64a1fc63d Mon Sep 17 00:00:00 2001 From: Linus Nordberg Date: Mon, 11 Jul 2011 20:51:26 +0200 Subject: [PATCH 13/14] Fix cut'n'paste bug in comment. --- src/or/or.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/or.h b/src/or/or.h index 3c764440ea..e9fab82f18 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3119,7 +3119,7 @@ struct socks_request_t { * see usernamelen for its length. */ char *username; /** The negotiated password value if any (for socks5). This value is NOT - * nul-terminated; see usernamelen for its length. */ + * nul-terminated; see passwordlen for its length. */ char *password; }; From 16c5a62a66098274dde726c8e02110238866fe0b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 Jul 2011 10:51:31 -0400 Subject: [PATCH 14/14] Add more error checks to socks parsing code Suggested by Linus to avoid uninitialized reads or infinite loops if it turns out our code is buggier than we had thought. --- src/or/buffers.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 1310b4215b..b7567bf24c 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1544,7 +1544,8 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req, else if (n_drain > 0) buf_remove_from_front(buf, n_drain); - } while (res == 0 && buf->head && want_length < buf->datalen); + } while (res == 0 && buf->head && want_length < buf->datalen && + buf->datalen >= 2); return res; } @@ -1595,12 +1596,14 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req, * will need more data than we currently have. */ /* Loop while we have more data that we haven't given parse_socks() yet. */ - while (evbuffer_get_length(buf) > datalen) { + do { int free_data = 0; + const size_t last_wanted = want_length; n_drain = 0; data = NULL; datalen = inspect_evbuffer(buf, &data, want_length, &free_data, NULL); + want_length = 0; res = parse_socks(data, datalen, req, log_sockstype, safe_socks, &n_drain, &want_length); @@ -1612,20 +1615,16 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req, else if (n_drain > 0) evbuffer_drain(buf, n_drain); - if (res) /* If res is nonzero, parse_socks() made up its mind. */ - return res; - - /* If parse_socks says that we want less data than we actually tried to - give it, we've got some kind of weird situation; just exit the loop for - now. - */ - if (want_length <= datalen) + if (res == 0 && n_drain == 0 && want_length <= last_wanted) { + /* If we drained nothing, and we didn't ask for more than last time, + * we're stuck in a loop. That's bad. It shouldn't be possible, but + * let's make sure. */ + log_warn(LD_BUG, "We seem to be caught in a parse loop; breaking out"); break; - /* Otherwise, it wants more data than we gave it. If we can provide more - * data than we gave it, we'll try to do so in the next iteration of the - * loop. If we can't, the while loop will exit. It's okay if it asked for - * more than we have total; maybe it doesn't really need so much. */ - } + } + + buflen = evbuffer_get_length(buf); + } while (res == 0 && want_length <= buflen && buflen >= 2); return res; } @@ -1652,6 +1651,12 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, unsigned char usernamelen, passlen; struct in_addr in; + if (datalen < 2) { + /* We always need at least 2 bytes. */ + *want_length_out = 2; + return 0; + } + if (req->socks_version == 5 && !req->got_auth) { /* See if we have received authentication. Strictly speaking, we should also check whether we actually negotiated username/password