[tor/master] Be more strict about when to accept socks auth message

13 Jul 2011

commit aec396d9d0c2db18d44c9ad850bedbc01d50e40a
Author: Nick Mathewson <nickm@torproject.org>
Date:   Wed Jun 29 12:00:00 2011 -0400

    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 3490d6d..bbd60c4 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"
 "<html>\n"
@@ -1937,7 +1943,7 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
 "</body>\n"
 "</html>\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 6ae62e8..bc80e39 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 <b>reply</b>. */
-  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 **********************/

    

nickm＠torproject.org

tags

participants (1)