[tor-commits] [tor/master] Socks5: handle truncated client requests correctly

[dgoulet at torproject.org]

commit c4fe66e342292b45f29e9fd242b66a0ca27a7758
Author: Nick Mathewson <nickm at torproject.org>
Date:   Mon Dec 14 10:14:03 2020 -0500

    Socks5: handle truncated client requests correctly
    
    Previously, our code would send back an error if the socks5 request
    parser said anything but DONE.  But there are other non-error cases,
    like TRUNCATED: we shouldn't send back errors for them.
    
    This patch lowers the responsibility for setting the error message
    into the parsing code, since the actual type of the error message
    will depend on what problem was encountered.
    
    Fixes bug 40190; bugfix on 0.3.5.1-alpha.
---
 changes/bug40190             | 4 ++++
 src/core/proto/proto_socks.c | 7 ++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/changes/bug40190 b/changes/bug40190
new file mode 100644
index 0000000000..0f3d6941dc
--- /dev/null
+++ b/changes/bug40190
@@ -0,0 +1,4 @@
+  o Minor bugfixes (SOCKS5):
+    - Handle partial socks5 messages correctly.  Previously, our code would
+      send an incorrect error message if it got a socks5 request that wasn't
+      complete. Fixes bug 40190; bugfix on 0.3.5.1-alpha.
diff --git a/src/core/proto/proto_socks.c b/src/core/proto/proto_socks.c
index c7bf13b9f4..5a7d7ac9be 100644
--- a/src/core/proto/proto_socks.c
+++ b/src/core/proto/proto_socks.c
@@ -545,6 +545,7 @@ parse_socks5_client_request(const uint8_t *raw_data, socks_request_t *req,
   if (parsed == -1) {
     log_warn(LD_APP, "socks5: parsing failed - invalid client request");
     res = SOCKS_RESULT_INVALID;
+    socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR);
     goto end;
   } else if (parsed == -2) {
     res = SOCKS_RESULT_TRUNCATED;
@@ -556,6 +557,7 @@ parse_socks5_client_request(const uint8_t *raw_data, socks_request_t *req,
 
   if (socks5_client_request_get_version(trunnel_req) != 5) {
     res = SOCKS_RESULT_INVALID;
+    socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR);
     goto end;
   }
 
@@ -590,6 +592,7 @@ parse_socks5_client_request(const uint8_t *raw_data, socks_request_t *req,
       tor_addr_to_str(req->address, &destaddr, sizeof(req->address), 1);
     } break;
     default: {
+      socks_request_set_socks5_error(req, SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED);
       res = -1;
     } break;
   }
@@ -770,8 +773,10 @@ handle_socks_message(const uint8_t *raw_data, size_t datalen,
     } else {
       res = parse_socks5_client_request(raw_data, req,
                                         datalen, drain_out);
-      if (res != SOCKS_RESULT_DONE) {
+      if (BUG(res == SOCKS_RESULT_INVALID && req->replylen == 0)) {
         socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR);
+      }
+      if (res != SOCKS_RESULT_DONE) {
         goto end;
       }