[tor-commits] [tor/master] Fix buf_t advancement in fetch_buf_from_socks

[nickm at torproject.org]

commit 58cb2ed24381d29458927c68e56aca0cae4b0f2e
Author: rl1987 <rl1987 at sdf.lonestar.org>
Date:   Wed May 23 11:44:43 2018 +0200

    Fix buf_t advancement in fetch_buf_from_socks
    
    We pullup 512 bytes of input to make sure that at least one SOCKS
    message ends up in head of linked list
---
 src/or/proto_socks.c  | 24 +++++++++++-------------
 src/test/test_socks.c |  4 ++--
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/or/proto_socks.c b/src/or/proto_socks.c
index 0c48d597e..9f6073d2a 100644
--- a/src/or/proto_socks.c
+++ b/src/or/proto_socks.c
@@ -32,8 +32,7 @@ static socks_result_t parse_socks(const char *data,
                                   socks_request_t *req,
                                   int log_sockstype,
                                   int safe_socks,
-                                  size_t *drain_out,
-                                  size_t *want_length_out);
+                                  size_t *drain_out);
 static int parse_socks_client(const uint8_t *data, size_t datalen,
                               int state, char **reason,
                               ssize_t *drain_out);
@@ -125,7 +124,7 @@ parse_socks4_request(const uint8_t *raw_data, socks_request_t *req,
     goto end;
   } else if (parsed == -2) {
     res = SOCKS_RESULT_TRUNCATED;
-    if (datalen > 1024) { // XXX
+    if (datalen >= MAX_SOCKS_MESSAGE_LEN) {
       log_warn(LD_APP, "socks4: parsing failed - invalid request.");
       res = SOCKS_RESULT_INVALID;
     }
@@ -273,7 +272,7 @@ parse_socks5_methods_request(const uint8_t *raw_data, socks_request_t *req,
     goto end;
   } else if (parsed == -2) {
     res = SOCKS_RESULT_TRUNCATED;
-    if (datalen > 1024) { // XXX
+    if (datalen > MAX_SOCKS_MESSAGE_LEN) {
       log_warn(LD_APP, "socks5: parsing failed - invalid version "
                        "id/method selection message.");
       res = SOCKS_RESULT_INVALID;
@@ -720,9 +719,9 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
   int res = 0;
   size_t datalen = buf_datalen(buf);
   size_t n_drain;
-  size_t want_length = 128;
   const char *head = NULL;
   socks_result_t socks_res;
+  size_t n_pullup;
 
   if (buf_datalen(buf) < 2) { /* version and another byte */
     res = 0;
@@ -731,13 +730,12 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
 
   do {
     n_drain = 0;
-    buf_pullup(buf, MAX(want_length, buf_datalen(buf)),
-               &head, &datalen);
+    n_pullup = MIN(MAX_SOCKS_MESSAGE_LEN, buf_datalen(buf));
+    buf_pullup(buf, n_pullup, &head, &datalen);
     tor_assert(head && datalen >= 2);
-    want_length = 0;
 
     socks_res = parse_socks(head, datalen, req, log_sockstype,
-                            safe_socks, &n_drain, &want_length);
+                            safe_socks, &n_drain);
 
     if (socks_res == SOCKS_RESULT_INVALID)
       buf_clear(buf);
@@ -752,6 +750,9 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
         res = 1;
         break;
       case SOCKS_RESULT_TRUNCATED:
+        if (datalen == n_pullup)
+          return 0;
+        /* FALLTHRU */
       case SOCKS_RESULT_MORE_EXPECTED:
         res = 0;
         break;
@@ -812,14 +813,12 @@ static const char SOCKS_PROXY_IS_NOT_AN_HTTP_PROXY_MSG[] =
  * we'd like to see in the input buffer, if they're available. */
 static int
 parse_socks(const char *data, size_t datalen, socks_request_t *req,
-            int log_sockstype, int safe_socks, size_t *drain_out,
-            size_t *want_length_out)
+            int log_sockstype, int safe_socks, size_t *drain_out)
 {
   uint8_t socksver;
 
   if (datalen < 2) {
     /* We always need at least 2 bytes. */
-    *want_length_out = 2;
     return 0;
   }
 
@@ -827,7 +826,6 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
 
   if (socksver == 5 || socksver == 4 ||
       socksver == 1) { // XXX: RFC 1929
-    *want_length_out = 128; // TODO remove this arg later
     return handle_socks_message((const uint8_t *)data, datalen, req,
                                 log_sockstype, safe_socks, drain_out);
   }
diff --git a/src/test/test_socks.c b/src/test/test_socks.c
index 9645ea32a..e1c5db6ee 100644
--- a/src/test/test_socks.c
+++ b/src/test/test_socks.c
@@ -142,7 +142,7 @@ test_socks_4_bad_arguments(void *ptr)
                                  get_options()->SafeSocks),
             OP_EQ, -1);
   buf_clear(buf);
-  expect_log_msg_containing("Port or DestIP is zero."); // !!!
+  expect_log_msg_containing("Port or DestIP is zero.");
   mock_clean_saved_logs();
 
   /* Try with 0 port */
@@ -164,7 +164,7 @@ test_socks_4_bad_arguments(void *ptr)
   tt_int_op(fetch_from_buf_socks(buf, socks, 1, 0),
             OP_EQ, -1);
   buf_clear(buf);
-  expect_log_msg_containing("user name too long; rejecting.");
+  expect_log_msg_containing("socks4: parsing failed - invalid request.");
   mock_clean_saved_logs();
 
   /* Try with 2000-byte hostname */