commit f85f52808cd29e50874b233e55b0a0fe98e425f3 Author: Robert Hogan robert@roberthogan.net Date: Sun Oct 17 14:14:51 2010 +0100
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 534f31c..5cd76fa 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 68f1aeb..8155c3f 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);