commit f85f52808cd29e50874b233e55b0a0fe98e425f3
Author: Robert Hogan <robert(a)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);