[tor-commits] [tor/master] bug1666 - Pass-through support for SOCKS5 authentication (2)

[nickm at torproject.org]

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