Merge branch 'socks_trunnel4_squashed_merged'

changes/ticket3569_part1

+  o Code simplification and refactoring:
+    - Rework Tor SOCKS server code to use Trunnel and benefit from
+      autogenerated functions for parsing and generating SOCKS wire
+      format. New implementation is cleaner, more maintainable and
+      should be less prone to heartbleed-style vulnerabilities.
+      Implements a significant fraction of ticket 3569.

scripts/codegen/fuzzing_include_am.py

@@ -12,6 +12,7 @@ FUZZERS = """
 	http-connect
 	iptsv2
 	microdesc
+	socks
 	vrs
 """
 

src/core/or/socks_request_st.h

@@ -70,6 +70,8 @@ struct socks_request_t {
   /** The negotiated password value if any (for socks5). This value is NOT
    * nul-terminated; see passwordlen for its length. */
   char *password;
+
+  uint8_t socks5_atyp; /* SOCKS5 address type */
 };
 
 #endif

src/test/fuzz/fuzz_socks.c

+/* Copyright (c) 2016-2018, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#include "orconfig.h"
+
+#define BUFFERS_PRIVATE
+#include "core/or/or.h"
+
+#include "lib/container/buffers.h"
+#include "lib/err/backtrace.h"
+#include "lib/log/log.h"
+#include "core/proto/proto_socks.h"
+#include "feature/client/addressmap.h"
+
+#include "test/fuzz/fuzzing.h"
+
+int
+fuzz_init(void)
+{
+  addressmap_init();
+  return 0;
+}
+
+int
+fuzz_cleanup(void)
+{
+  addressmap_free_all();
+  return 0;
+}
+
+int
+fuzz_main(const uint8_t *stdin_buf, size_t data_size)
+{
+  buf_t *buffer = buf_new_with_data((char*)stdin_buf, data_size);
+  if (!buffer) {
+    tor_assert(data_size==0);
+    buffer = buf_new();
+  }
+
+  socks_request_t *request = socks_request_new();
+
+  int r = fetch_from_buf_socks(buffer, request, 0, 0);
+  log_info(LD_GENERAL, "Socks request status: %d", r);
+
+  /* Reset. */
+  buf_free(buffer);
+  socks_request_free(request);
+
+  return 0;
+}

src/test/fuzz/include.am

@@ -17,7 +17,7 @@ FUZZING_LIBS = \
 	@TOR_ZSTD_LIBS@
 
 oss-fuzz-prereqs: \
-	$(TOR_INTERNAL_TESTING_LIBS)
+    $(TOR_INTERNAL_TESTING_LIBS)
 
 noinst_HEADERS += \
 	src/test/fuzz/fuzzing.h
@@ -120,6 +120,14 @@ src_test_fuzz_fuzz_microdesc_CFLAGS = $(FUZZING_CFLAGS)
 src_test_fuzz_fuzz_microdesc_LDFLAGS = $(FUZZING_LDFLAG)
 src_test_fuzz_fuzz_microdesc_LDADD = $(FUZZING_LIBS)
 
+src_test_fuzz_fuzz_socks_SOURCES = \
+	src/test/fuzz/fuzzing_common.c \
+	src/test/fuzz/fuzz_socks.c
+src_test_fuzz_fuzz_socks_CPPFLAGS = $(FUZZING_CPPFLAGS)
+src_test_fuzz_fuzz_socks_CFLAGS = $(FUZZING_CFLAGS)
+src_test_fuzz_fuzz_socks_LDFLAGS = $(FUZZING_LDFLAG)
+src_test_fuzz_fuzz_socks_LDADD = $(FUZZING_LIBS)
+
 src_test_fuzz_fuzz_vrs_SOURCES = \
 	src/test/fuzz/fuzzing_common.c \
 	src/test/fuzz/fuzz_vrs.c
@@ -140,6 +148,7 @@ FUZZERS = \
 	src/test/fuzz/fuzz-http-connect \
 	src/test/fuzz/fuzz-iptsv2 \
 	src/test/fuzz/fuzz-microdesc \
+	src/test/fuzz/fuzz-socks \
 	src/test/fuzz/fuzz-vrs
 
 # ===== libfuzzer
@@ -222,6 +231,13 @@ src_test_fuzz_lf_fuzz_microdesc_CFLAGS = $(LIBFUZZER_CFLAGS)
 src_test_fuzz_lf_fuzz_microdesc_LDFLAGS = $(LIBFUZZER_LDFLAG)
 src_test_fuzz_lf_fuzz_microdesc_LDADD = $(LIBFUZZER_LIBS)
 
+src_test_fuzz_lf_fuzz_socks_SOURCES = \
+	$(src_test_fuzz_fuzz_socks_SOURCES)
+src_test_fuzz_lf_fuzz_socks_CPPFLAGS = $(LIBFUZZER_CPPFLAGS)
+src_test_fuzz_lf_fuzz_socks_CFLAGS = $(LIBFUZZER_CFLAGS)
+src_test_fuzz_lf_fuzz_socks_LDFLAGS = $(LIBFUZZER_LDFLAG)
+src_test_fuzz_lf_fuzz_socks_LDADD = $(LIBFUZZER_LIBS)
+
 src_test_fuzz_lf_fuzz_vrs_SOURCES = \
 	$(src_test_fuzz_fuzz_vrs_SOURCES)
 src_test_fuzz_lf_fuzz_vrs_CPPFLAGS = $(LIBFUZZER_CPPFLAGS)
@@ -241,6 +257,7 @@ LIBFUZZER_FUZZERS = \
 	src/test/fuzz/lf-fuzz-http-connect \
 	src/test/fuzz/lf-fuzz-iptsv2 \
 	src/test/fuzz/lf-fuzz-microdesc \
+	src/test/fuzz/lf-fuzz-socks \
 	src/test/fuzz/lf-fuzz-vrs
 
 else
@@ -305,6 +322,11 @@ src_test_fuzz_liboss_fuzz_microdesc_a_SOURCES = \
 src_test_fuzz_liboss_fuzz_microdesc_a_CPPFLAGS = $(LIBOSS_FUZZ_CPPFLAGS)
 src_test_fuzz_liboss_fuzz_microdesc_a_CFLAGS = $(LIBOSS_FUZZ_CFLAGS)
 
+src_test_fuzz_liboss_fuzz_socks_a_SOURCES = \
+	$(src_test_fuzz_fuzz_socks_SOURCES)
+src_test_fuzz_liboss_fuzz_socks_a_CPPFLAGS = $(LIBOSS_FUZZ_CPPFLAGS)
+src_test_fuzz_liboss_fuzz_socks_a_CFLAGS = $(LIBOSS_FUZZ_CFLAGS)
+
 src_test_fuzz_liboss_fuzz_vrs_a_SOURCES = \
 	$(src_test_fuzz_fuzz_vrs_SOURCES)
 src_test_fuzz_liboss_fuzz_vrs_a_CPPFLAGS = $(LIBOSS_FUZZ_CPPFLAGS)
@@ -322,6 +344,7 @@ OSS_FUZZ_FUZZERS = \
 	src/test/fuzz/liboss-fuzz-http-connect.a \
 	src/test/fuzz/liboss-fuzz-iptsv2.a \
 	src/test/fuzz/liboss-fuzz-microdesc.a \
+	src/test/fuzz/liboss-fuzz-socks.a \
 	src/test/fuzz/liboss-fuzz-vrs.a
 
 else

src/test/test_socks.c

@@ -84,7 +84,7 @@ test_socks_4_supported_commands(void *ptr)
 
   tt_int_op(0,OP_EQ, buf_datalen(buf));
 
-  /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4370 */
+  /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.3:4370 */
   ADD_DATA(buf, "\x04\x01\x11\x12\x02\x02\x02\x03\x00");
   tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
                                  get_options()->SafeSocks),
@@ -100,7 +100,7 @@ test_socks_4_supported_commands(void *ptr)
   tt_int_op(0,OP_EQ, buf_datalen(buf));
   socks_request_clear(socks);
 
-  /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.2:4369 with userid*/
+  /* SOCKS 4 Send CONNECT [01] to IP address 2.2.2.4:4369 with userid*/
   ADD_DATA(buf, "\x04\x01\x11\x12\x02\x02\x02\x04me\x00");
   tt_int_op(fetch_from_buf_socks(buf, socks, 1, 0),
             OP_EQ, 1);
@@ -166,7 +166,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 */
@@ -194,7 +194,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("Destaddr too long.");
+  expect_log_msg_containing("parsing failed - invalid request.");
   mock_clean_saved_logs();
 
   /* Socks4, bogus hostname */
@@ -648,7 +648,8 @@ test_socks_5_malformed_commands(void *ptr)
   tt_int_op(5,OP_EQ,socks->socks_version);
   tt_int_op(10,OP_EQ,socks->replylen);
   tt_int_op(5,OP_EQ,socks->reply[0]);
-  tt_int_op(SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED,OP_EQ,socks->reply[1]);
+  /* trunnel parsing will fail with -1 */
+  tt_int_op(SOCKS5_GENERAL_ERROR,OP_EQ,socks->reply[1]);
   tt_int_op(1,OP_EQ,socks->reply[3]);
 
  done:

src/trunnel/include.am

@@ -10,7 +10,8 @@ TRUNNELINPUTS = \
 	src/trunnel/ed25519_cert.trunnel \
 	src/trunnel/link_handshake.trunnel \
 	src/trunnel/pwbox.trunnel \
-	src/trunnel/channelpadding_negotiation.trunnel
+	src/trunnel/channelpadding_negotiation.trunnel \
+	src/trunner/socks5.trunnel
 
 TRUNNELSOURCES = \
 	src/ext/trunnel/trunnel.c \
@@ -21,7 +22,8 @@ TRUNNELSOURCES = \
 	src/trunnel/hs/cell_establish_intro.c	\
 	src/trunnel/hs/cell_introduce1.c \
 	src/trunnel/hs/cell_rendezvous.c \
-	src/trunnel/channelpadding_negotiation.c
+	src/trunnel/channelpadding_negotiation.c \
+	src/trunnel/socks5.c
 
 TRUNNELHEADERS = \
 	src/ext/trunnel/trunnel.h		\
@@ -34,7 +36,8 @@ TRUNNELHEADERS = \
 	src/trunnel/hs/cell_establish_intro.h	\
 	src/trunnel/hs/cell_introduce1.h \
 	src/trunnel/hs/cell_rendezvous.h \
-	src/trunnel/channelpadding_negotiation.h
+	src/trunnel/channelpadding_negotiation.h \
+	src/trunnel/socks5.h
 
 src_trunnel_libor_trunnel_a_SOURCES = $(TRUNNELSOURCES)
 src_trunnel_libor_trunnel_a_CPPFLAGS = \

src/trunnel/socks5.trunnel

+// Example: here's a quickie implementation of the messages in the
+// socks5 protocol.
+
+struct socks5_client_version {
+   u8 version IN [5];
+   u8 n_methods;
+   u8 methods[n_methods];
+}
+
+struct socks5_server_method {
+   u8 version IN [5];
+   u8 method;
+}
+
+const CMD_CONNECT = 1;
+const CMD_BIND = 2;
+const CMD_UDP_ASSOCIATE = 3;
+// This is a tor extension
+const CMD_RESOLVE = 0xF0;
+const CMD_RESOLVE_PTR = 0xF1;
+
+const ATYPE_IPV4 = 1;
+const ATYPE_IPV6 = 4;
+const ATYPE_DOMAINNAME = 3;
+
+struct domainname {
+   u8 len;
+   char name[len];
+}
+
+struct socks5_client_request {
+   u8 version IN [5];
+   u8 command IN [CMD_CONNECT, CMD_BIND, CMD_UDP_ASSOCIATE, CMD_RESOLVE, CMD_RESOLVE_PTR];
+   u8 reserved IN [0];
+   u8 atype;
+   union dest_addr[atype] {
+     ATYPE_IPV4: u32 ipv4;
+     ATYPE_IPV6: u8 ipv6[16];
+     ATYPE_DOMAINNAME: struct domainname domainname;
+     default: fail;
+   };
+   u16 dest_port;
+}
+
+struct socks5_server_reply {
+   u8 version IN [5];
+   u8 reply;
+   u8 reserved IN [0];
+   u8 atype;
+   union bind_addr[atype] {
+     ATYPE_IPV4: u32 ipv4;
+     ATYPE_IPV6: u8 ipv6[16];
+     ATYPE_DOMAINNAME: struct domainname domainname;
+     default: fail;
+   };
+   u16 bind_port;
+}
+
+struct socks5_client_userpass_auth {
+   u8 version IN [1];
+   u8 username_len;
+   char username[username_len];
+   u8 passwd_len;
+   char passwd[passwd_len];
+}
+
+struct socks5_server_userpass_auth {
+   u8 version IN [1];
+   u8 status;
+}
+
+// Oh why not.  Here's socks4 and socks4a.
+
+struct socks4_client_request {
+   u8 version IN [4];
+   u8 command IN [CMD_CONNECT,CMD_BIND,CMD_RESOLVE,CMD_RESOLVE_PTR];
+   u16 port;
+   u32 addr;
+   nulterm username;
+   union socks4a_addr[addr] {
+      1..255:
+               nulterm hostname;
+      default:
+               ;
+   };
+}
+
+struct socks4_server_reply {
+   u8 version IN [4];
+   u8 status;
+   u16 port;
+   u32 addr;
+}
+