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 **********************/