Merge remote-tracking branch 'isis-github/bug25409'

changes/bug25409

@@ -0,0 +1,12 @@
+  o Removed features:
+    - The PortForwarding and PortForwardingHelper features have been
+      removed. The reasoning is, given that implementations of NAT traversal
+      protocols within common consumer grade routers are frequently buggy, and
+      that the target audience for a NAT punching feature is a perhaps
+      less-technically-inclined relay operator, when the helper fails to setup
+      traversal the problems are usually deep, ugly, and very router specific,
+      making them horrendously impossible for technical support to reliable
+      assist with, and thus resulting in frustration all around.  Unfortunately,
+      relay operators who would like to run relays behind NATs will need to
+      become more familiar with the port forwarding configurations on their
+      local router.  Closes 25409.

changes/ticket25409

@@ -0,0 +1,6 @@
+  o Code simplification and refactoring:
+      We remove the PortForwsrding and PortForwardingHelper options, related
+      functions, and the port_forwarding tests. These options were used by
+      the now-deprecated Vidalia to help ordinary users become Tor relays or
+      bridges. Closes ticket 25409. Patch by Neel Chauhan.
+

doc/tor.1.txt

@@ -805,10 +805,9 @@ GENERAL OPTIONS
 
 [[NoExec]] **NoExec** **0**|**1**::
     If this option is set to 1, then Tor will never launch another
-    executable, regardless of the settings of PortForwardingHelper,
-    ClientTransportPlugin, or ServerTransportPlugin.  Once this
-    option has been set to 1, it cannot be set back to 0 without
-    restarting Tor. (Default: 0)
+    executable, regardless of the settings of ClientTransportPlugin
+    or ServerTransportPlugin.  Once this option has been set to 1,
+    it cannot be set back to 0 without restarting Tor. (Default: 0)
 
 [[Schedulers]] **Schedulers** **KIST**|**KISTLite**|**Vanilla**::
     Specify the scheduler type that tor should use. The scheduler is
@@ -2087,18 +2086,6 @@ is non-zero):
     For obvious reasons, NoAdvertise and NoListen are mutually exclusive, and
     IPv4Only and IPv6Only are mutually exclusive.
 
-[[PortForwarding]] **PortForwarding** **0**|**1**::
-    Attempt to automatically forward the DirPort and ORPort on a NAT router
-    connecting this Tor server to the Internet. If set, Tor will try both
-    NAT-PMP (common on Apple routers) and UPnP (common on routers from other
-    manufacturers). (Default: 0)
-
-[[PortForwardingHelper]] **PortForwardingHelper** __filename__|__pathname__::
-    If PortForwarding is set, use this executable to configure the forwarding.
-    If set to a filename, the system path will be searched for the executable.
-    If set to a path, only the specified path will be executed.
-    (Default: tor-fw-helper)
-
 [[PublishServerDescriptor]] **PublishServerDescriptor** **0**|**1**|**v3**|**bridge**,**...**::
     This option specifies which descriptors Tor will publish when acting as
     a relay. You can

src/common/util.c

@@ -5111,30 +5111,6 @@ stream_status_to_string(enum stream_status stream_status)
   }
 }
 
-/* DOCDOC */
-static void
-log_portfw_spawn_error_message(const char *buf,
-                               const char *executable, int *child_status)
-{
-  /* Parse error message */
-  int retval, child_state, saved_errno;
-  retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
-                      &child_state, &saved_errno);
-  if (retval == 2) {
-    log_warn(LD_GENERAL,
-             "Failed to start child process \"%s\" in state %d: %s",
-             executable, child_state, strerror(saved_errno));
-    if (child_status)
-      *child_status = 1;
-  } else {
-    /* Failed to parse message from child process, log it as a
-       warning */
-    log_warn(LD_GENERAL,
-             "Unexpected message from port forwarding helper \"%s\": %s",
-             executable, buf);
-  }
-}
-
 #ifdef _WIN32
 
 /** Return a smartlist containing lines outputted from
@@ -5254,42 +5230,6 @@ tor_get_lines_from_handle, (int fd, enum stream_status *stream_status_out))
   return lines;
 }
 
-/** Read from fd, and send lines to log at the specified log level.
- * Returns 1 if stream is closed normally, -1 if there is a error reading, and
- * 0 otherwise. Handles lines from tor-fw-helper and
- * tor_spawn_background() specially.
- */
-static int
-log_from_pipe(int fd, int severity, const char *executable,
-              int *child_status)
-{
-  char buf[256];
-  enum stream_status r;
-
-  for (;;) {
-    r = get_string_from_pipe(fd, buf, sizeof(buf) - 1);
-
-    if (r == IO_STREAM_CLOSED) {
-      return 1;
-    } else if (r == IO_STREAM_EAGAIN) {
-      return 0;
-    } else if (r == IO_STREAM_TERM) {
-      return -1;
-    }
-
-    tor_assert(r == IO_STREAM_OKAY);
-
-    /* Check if buf starts with SPAWN_ERROR_MESSAGE */
-    if (strcmpstart(buf, SPAWN_ERROR_MESSAGE) == 0) {
-      log_portfw_spawn_error_message(buf, executable, child_status);
-    } else {
-      log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf);
-    }
-  }
-
-  /* We should never get here */
-  return -1;
-}
 #endif /* defined(_WIN32) */
 
 /** Reads from <b>fd</b> and stores input in <b>buf_out</b> making
@@ -5332,294 +5272,6 @@ get_string_from_pipe(int fd, char *buf_out, size_t count)
   return IO_STREAM_OKAY;
 }
 
-/** Parse a <b>line</b> from tor-fw-helper and issue an appropriate
- *  log message to our user. */
-static void
-handle_fw_helper_line(const char *executable, const char *line)
-{
-  smartlist_t *tokens = smartlist_new();
-  char *message = NULL;
-  char *message_for_log = NULL;
-  const char *external_port = NULL;
-  const char *internal_port = NULL;
-  const char *result = NULL;
-  int port = 0;
-  int success = 0;
-
-  if (strcmpstart(line, SPAWN_ERROR_MESSAGE) == 0) {
-    /* We need to check for SPAWN_ERROR_MESSAGE again here, since it's
-     * possible that it got sent after we tried to read it in log_from_pipe.
-     *
-     * XXX Ideally, we should be using one of stdout/stderr for the real
-     * output, and one for the output of the startup code.  We used to do that
-     * before cd05f35d2c.
-     */
-    int child_status;
-    log_portfw_spawn_error_message(line, executable, &child_status);
-    goto done;
-  }
-
-  smartlist_split_string(tokens, line, NULL,
-                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);
-
-  if (smartlist_len(tokens) < 5)
-    goto err;
-
-  if (strcmp(smartlist_get(tokens, 0), "tor-fw-helper") ||
-      strcmp(smartlist_get(tokens, 1), "tcp-forward"))
-    goto err;
-
-  external_port = smartlist_get(tokens, 2);
-  internal_port = smartlist_get(tokens, 3);
-  result = smartlist_get(tokens, 4);
-
-  if (smartlist_len(tokens) > 5) {
-    /* If there are more than 5 tokens, they are part of [<message>].
-       Let's use a second smartlist to form the whole message;
-       strncat loops suck. */
-    int i;
-    int message_words_n = smartlist_len(tokens) - 5;
-    smartlist_t *message_sl = smartlist_new();
-    for (i = 0; i < message_words_n; i++)
-      smartlist_add(message_sl, smartlist_get(tokens, 5+i));
-
-    tor_assert(smartlist_len(message_sl) > 0);
-    message = smartlist_join_strings(message_sl, " ", 0, NULL);
-
-    /* wrap the message in log-friendly wrapping */
-    tor_asprintf(&message_for_log, " ('%s')", message);
-
-    smartlist_free(message_sl);
-  }
-
-  port = atoi(external_port);
-  if (port < 1 || port > 65535)
-    goto err;
-
-  port = atoi(internal_port);
-  if (port < 1 || port > 65535)
-    goto err;
-
-  if (!strcmp(result, "SUCCESS"))
-    success = 1;
-  else if (!strcmp(result, "FAIL"))
-    success = 0;
-  else
-    goto err;
-
-  if (!success) {
-    log_warn(LD_GENERAL, "Tor was unable to forward TCP port '%s' to '%s'%s. "
-             "Please make sure that your router supports port "
-             "forwarding protocols (like NAT-PMP). Note that if '%s' is "
-             "your ORPort, your relay will be unable to receive inbound "
-             "traffic.", external_port, internal_port,
-             message_for_log ? message_for_log : "",
-             internal_port);
-  } else {
-    log_info(LD_GENERAL,
-             "Tor successfully forwarded TCP port '%s' to '%s'%s.",
-             external_port, internal_port,
-             message_for_log ? message_for_log : "");
-  }
-
-  goto done;
-
- err:
-  log_warn(LD_GENERAL, "tor-fw-helper sent us a string we could not "
-           "parse (%s).", line);
-
- done:
-  SMARTLIST_FOREACH(tokens, char *, cp, tor_free(cp));
-  smartlist_free(tokens);
-  tor_free(message);
-  tor_free(message_for_log);
-}
-
-/** Read what tor-fw-helper has to say in its stdout and handle it
- *  appropriately */
-static int
-handle_fw_helper_output(const char *executable,
-                        process_handle_t *process_handle)
-{
-  smartlist_t *fw_helper_output = NULL;
-  enum stream_status stream_status = 0;
-
-  fw_helper_output =
-    tor_get_lines_from_handle(tor_process_get_stdout_pipe(process_handle),
-                              &stream_status);
-  if (!fw_helper_output) { /* didn't get any output from tor-fw-helper */
-    /* if EAGAIN we should retry in the future */
-    return (stream_status == IO_STREAM_EAGAIN) ? 0 : -1;
-  }
-
-  /* Handle the lines we got: */
-  SMARTLIST_FOREACH_BEGIN(fw_helper_output, char *, line) {
-    handle_fw_helper_line(executable, line);
-    tor_free(line);
-  } SMARTLIST_FOREACH_END(line);
-
-  smartlist_free(fw_helper_output);
-
-  return 0;
-}
-
-/** Spawn tor-fw-helper and ask it to forward the ports in
- *  <b>ports_to_forward</b>. <b>ports_to_forward</b> contains strings
- *  of the form "<external port>:<internal port>", which is the format
- *  that tor-fw-helper expects. */
-void
-tor_check_port_forwarding(const char *filename,
-                          smartlist_t *ports_to_forward,
-                          time_t now)
-{
-/* When fw-helper succeeds, how long do we wait until running it again */
-#define TIME_TO_EXEC_FWHELPER_SUCCESS 300
-/* When fw-helper failed to start, how long do we wait until running it again
- */
-#define TIME_TO_EXEC_FWHELPER_FAIL 60
-
-  /* Static variables are initialized to zero, so child_handle.status=0
-   * which corresponds to it not running on startup */
-  static process_handle_t *child_handle=NULL;
-
-  static time_t time_to_run_helper = 0;
-  int stderr_status, retval;
-  int stdout_status = 0;
-
-  tor_assert(filename);
-
-  /* Start the child, if it is not already running */
-  if ((!child_handle || child_handle->status != PROCESS_STATUS_RUNNING) &&
-      time_to_run_helper < now) {
-    /*tor-fw-helper cli looks like this: tor_fw_helper -p :5555 -p 4555:1111 */
-    const char **argv; /* cli arguments */
-    int args_n, status;
-    int argv_index = 0; /* index inside 'argv' */
-
-    tor_assert(smartlist_len(ports_to_forward) > 0);
-
-    /* check for overflow during 'argv' allocation:
-       (len(ports_to_forward)*2 + 2)*sizeof(char*) > SIZE_MAX ==
-       len(ports_to_forward) > (((SIZE_MAX/sizeof(char*)) - 2)/2) */
-    if ((size_t) smartlist_len(ports_to_forward) >
-        (((SIZE_MAX/sizeof(char*)) - 2)/2)) {
-      log_warn(LD_GENERAL,
-               "Overflow during argv allocation. This shouldn't happen.");
-      return;
-    }
-    /* check for overflow during 'argv_index' increase:
-       ((len(ports_to_forward)*2 + 2) > INT_MAX) ==
-       len(ports_to_forward) > (INT_MAX - 2)/2 */
-    if (smartlist_len(ports_to_forward) > (INT_MAX - 2)/2) {
-      log_warn(LD_GENERAL,
-               "Overflow during argv_index increase. This shouldn't happen.");
-      return;
-    }
-
-    /* Calculate number of cli arguments: one for the filename, two
-       for each smartlist element (one for "-p" and one for the
-       ports), and one for the final NULL. */
-    args_n = 1 + 2*smartlist_len(ports_to_forward) + 1;
-    argv = tor_calloc(args_n, sizeof(char *));
-
-    argv[argv_index++] = filename;
-    SMARTLIST_FOREACH_BEGIN(ports_to_forward, const char *, port) {
-      argv[argv_index++] = "-p";
-      argv[argv_index++] = port;
-    } SMARTLIST_FOREACH_END(port);
-    argv[argv_index] = NULL;
-
-    /* Assume tor-fw-helper will succeed, start it later*/
-    time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS;
-
-    if (child_handle) {
-      tor_process_handle_destroy(child_handle, 1);
-      child_handle = NULL;
-    }
-
-#ifdef _WIN32
-    /* Passing NULL as lpApplicationName makes Windows search for the .exe */
-    status = tor_spawn_background(NULL, argv, NULL, &child_handle);
-#else
-    status = tor_spawn_background(filename, argv, NULL, &child_handle);
-#endif /* defined(_WIN32) */
-
-    tor_free_((void*)argv);
-    argv=NULL;
-
-    if (PROCESS_STATUS_ERROR == status) {
-      log_warn(LD_GENERAL, "Failed to start port forwarding helper %s",
-              filename);
-      time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
-      return;
-    }
-
-    log_info(LD_GENERAL,
-             "Started port forwarding helper (%s) with pid '%d'",
-             filename, tor_process_get_pid(child_handle));
-  }
-
-  /* If child is running, read from its stdout and stderr) */
-  if (child_handle && PROCESS_STATUS_RUNNING == child_handle->status) {
-    /* Read from stdout/stderr and log result */
-    retval = 0;
-#ifdef _WIN32
-    stderr_status = log_from_handle(child_handle->stderr_pipe, LOG_INFO);
-#else
-    stderr_status = log_from_pipe(child_handle->stderr_pipe,
-                                  LOG_INFO, filename, &retval);
-#endif /* defined(_WIN32) */
-    if (handle_fw_helper_output(filename, child_handle) < 0) {
-      log_warn(LD_GENERAL, "Failed to handle fw helper output.");
-      stdout_status = -1;
-      retval = -1;
-    }
-
-    if (retval) {
-      /* There was a problem in the child process */
-      time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
-    }
-
-    /* Combine the two statuses in order of severity */
-    if (-1 == stdout_status || -1 == stderr_status)
-      /* There was a failure */
-      retval = -1;
-#ifdef _WIN32
-    else if (!child_handle || tor_get_exit_code(child_handle, 0, NULL) !=
-             PROCESS_EXIT_RUNNING) {
-      /* process has exited or there was an error */
-      /* TODO: Do something with the process return value */
-      /* TODO: What if the process output something since
-       * between log_from_handle and tor_get_exit_code? */
-      retval = 1;
-    }
-#else /* !(defined(_WIN32)) */
-    else if (1 == stdout_status || 1 == stderr_status)
-      /* stdout or stderr was closed, the process probably
-       * exited. It will be reaped by waitpid() in main.c */
-      /* TODO: Do something with the process return value */
-      retval = 1;
-#endif /* defined(_WIN32) */
-    else
-      /* Both are fine */
-      retval = 0;
-
-    /* If either pipe indicates a failure, act on it */
-    if (0 != retval) {
-      if (1 == retval) {
-        log_info(LD_GENERAL, "Port forwarding helper terminated");
-        child_handle->status = PROCESS_STATUS_NOTRUNNING;
-      } else {
-        log_warn(LD_GENERAL, "Failed to read from port forwarding helper");
-        child_handle->status = PROCESS_STATUS_ERROR;
-      }
-
-      /* TODO: The child might not actually be finished (maybe it failed or
-         closed stdout/stderr), so maybe we shouldn't start another? */
-    }
-  }
-}
-
 /** Initialize the insecure RNG <b>rng</b> from a seed value <b>seed</b>. */
 void
 tor_init_weak_random(tor_weak_rng_t *rng, unsigned seed)

src/common/util.h

@@ -414,11 +414,6 @@ void start_daemon(void);
 void finish_daemon(const char *desired_cwd);
 int write_pidfile(const char *filename);
 
-/* Port forwarding */
-void tor_check_port_forwarding(const char *filename,
-                               struct smartlist_t *ports_to_forward,
-                               time_t now);
-
 void tor_disable_spawning_background_processes(void);
 
 typedef struct process_handle_t process_handle_t;
@@ -457,9 +452,7 @@ void set_environment_variable_in_smartlist(struct smartlist_t *env_vars,
                                            void (*free_old)(void*),
                                            int free_p);
 
-/* Values of process_handle_t.status. PROCESS_STATUS_NOTRUNNING must be
- * 0 because tor_check_port_forwarding depends on this being the initial
- * statue of the static instance of process_handle_t */
+/* Values of process_handle_t.status. */
 #define PROCESS_STATUS_NOTRUNNING 0
 #define PROCESS_STATUS_RUNNING 1
 #define PROCESS_STATUS_ERROR -1

src/or/config.c

@@ -492,8 +492,8 @@ static config_var_t option_vars_[] = {
   V(TestingSigningKeySlop,           INTERVAL, "1 day"),
 
   V(OptimisticData,              AUTOBOOL, "auto"),
-  V(PortForwarding,              BOOL,     "0"),
-  V(PortForwardingHelper,        FILENAME, "tor-fw-helper"),
+  OBSOLETE("PortForwarding"),
+  OBSOLETE("PortForwardingHelper"),
   OBSOLETE("PreferTunneledDirConns"),
   V(ProtocolWarnings,            BOOL,     "0"),
   V(PublishServerDescriptor,     CSV,      "1"),
@@ -3879,15 +3879,6 @@ options_validate(or_options_t *old_options, or_options_t *options,
   if (options->KeepalivePeriod < 1)
     REJECT("KeepalivePeriod option must be positive.");
 
-  if (options->PortForwarding && options->Sandbox) {
-    REJECT("PortForwarding is not compatible with Sandbox; at most one can "
-           "be set");
-  }
-  if (options->PortForwarding && options->NoExec) {
-    COMPLAIN("Both PortForwarding and NoExec are set; PortForwarding will "
-             "be ignored.");
-  }
-
   if (ensure_bandwidth_cap(&options->BandwidthRate,
                            "BandwidthRate", msg) < 0)
     return -1;

src/or/main.c

@@ -1335,7 +1335,6 @@ CALLBACK(retry_listeners);
 CALLBACK(expire_old_ciruits_serverside);
 CALLBACK(check_dns_honesty);
 CALLBACK(write_bridge_ns);
-CALLBACK(check_fw_helper_app);
 CALLBACK(heartbeat);
 CALLBACK(clean_consdiffmgr);
 CALLBACK(reset_padding_counts);
@@ -1371,7 +1370,6 @@ static periodic_event_item_t periodic_events[] = {
   CALLBACK(expire_old_ciruits_serverside),
   CALLBACK(check_dns_honesty),
   CALLBACK(write_bridge_ns),
-  CALLBACK(check_fw_helper_app),
   CALLBACK(heartbeat),
   CALLBACK(clean_consdiffmgr),
   CALLBACK(reset_padding_counts),
@@ -2185,33 +2183,6 @@ write_bridge_ns_callback(time_t now, const or_options_t *options)
   return PERIODIC_EVENT_NO_UPDATE;
 }
 
-/**
- * Periodic callback: poke the tor-fw-helper app if we're using one.
- */
-static int
-check_fw_helper_app_callback(time_t now, const or_options_t *options)
-{
-  if (net_is_disabled() ||
-      ! server_mode(options) ||
-      ! options->PortForwarding ||
-      options->NoExec) {
-    return PERIODIC_EVENT_NO_UPDATE;
-  }
-  /* 11. check the port forwarding app */
-
-#define PORT_FORWARDING_CHECK_INTERVAL 5
-  smartlist_t *ports_to_forward = get_list_of_ports_to_forward();
-  if (ports_to_forward) {
-    tor_check_port_forwarding(options->PortForwardingHelper,
-                              ports_to_forward,
-                              now);
-
-    SMARTLIST_FOREACH(ports_to_forward, char *, cp, tor_free(cp));
-    smartlist_free(ports_to_forward);
-  }
-  return PORT_FORWARDING_CHECK_INTERVAL;
-}
-
 static int heartbeat_callback_first_time = 1;
 
 /**

src/or/or.h

@@ -4200,10 +4200,6 @@ typedef struct {
                                         * testing our DNS server. */
   int EnforceDistinctSubnets; /**< If true, don't allow multiple routers in the
                                * same network zone in the same circuit. */
-  int PortForwarding; /**< If true, use NAT-PMP or UPnP to automatically
-                       * forward the DirPort and ORPort on the NAT device */
-  char *PortForwardingHelper; /** < Filename or full path of the port
-                                  forwarding helper executable */
   int AllowNonRFC953Hostnames; /**< If true, we allow connections to hostnames
                                 * with weird characters. */
   /** If true, we try resolving hostnames with weird characters. */

src/test/test_options.c

@@ -2421,37 +2421,6 @@ test_options_validate__circuits(void *ignored)
   tor_free(msg);
 }
 
-static void
-test_options_validate__port_forwarding(void *ignored)
-{
-  (void)ignored;
-  int ret;
-  char *msg;
-  options_test_data_t *tdata = NULL;
-
-  free_options_test_data(tdata);
-  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
-                                "PortForwarding 1\nSandbox 1\n");
-  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(msg, OP_EQ, "PortForwarding is not compatible with Sandbox;"
-            " at most one can be set");
-  tor_free(msg);
-
-  free_options_test_data(tdata);
-  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
-                                "PortForwarding 1\nSandbox 0\n");
-  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_int_op(ret, OP_EQ, 0);
-  tt_assert(!msg);
-  tor_free(msg);
-
- done:
-  free_options_test_data(tdata);
-  policies_free_all();
-  tor_free(msg);
-}
-
 static void
 test_options_validate__tor2web(void *ignored)
 {
@@ -4261,7 +4230,6 @@ struct testcase_t options_tests[] = {
   LOCAL_VALIDATE_TEST(path_bias),
   LOCAL_VALIDATE_TEST(bandwidth),
   LOCAL_VALIDATE_TEST(circuits),
-  LOCAL_VALIDATE_TEST(port_forwarding),
   LOCAL_VALIDATE_TEST(tor2web),
   LOCAL_VALIDATE_TEST(rend),
   LOCAL_VALIDATE_TEST(single_onion),

src/tools/tor-fw-helper/README

@@ -1,10 +0,0 @@
-
-We no longer recommend the use of this tool.  Instead, please use the
-pure-Go version of tor-fw-helper available at
-     https://gitweb.torproject.org/tor-fw-helper.git
-
-Why?
-
-The C code here was fine, but frankly: we don't trust the underlying
-libraries. They don't seem to have been written with network security
-in mind, and we have very little faith in their safety.